Conversation
📝 WalkthroughWalkthroughThis PR introduces three architectural documentation files outlining a comprehensive redesign of the gts-rust macro system, transitioning from a monolithic Changes
Possibly related PRs
Poem
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
docs/001-macro-proposal.md (2)
126-174: Clear before/after migration example with helpful summary table.The side-by-side comparison makes the transition immediately understandable. The table at lines 167-174 provides excellent quick-reference guidance for each change.
One minor observation: The "After" example (line 149) explicitly includes
JsonSchemain the derives, but according to the ADR (lines 235-236 in the ADR file),JsonSchemais auto-added if not present. Consider adding a note clarifying thatJsonSchemacan be omitted and will be added automatically.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/001-macro-proposal.md` around lines 126 - 174, Add a brief clarifying sentence to the "After" example explaining that JsonSchema in the derive list is optional because the macro will auto-add JsonSchema when omitted per the ADR; reference the example struct BaseEventV1 and the derive attributes (Serialize, Deserialize, JsonSchema, GtsSchema) and mention the ADR lines about auto-adding JsonSchema so readers know they can omit JsonSchema and rely on the macro to inject it.
380-418: Well-planned modular architecture for extensibility.The proposed module structure (lines 380-390) is a significant improvement over the current ~1,843-line single file. The two concrete examples (lines 392-417) demonstrate how the architecture accommodates future spec features:
- Adding a new field-level attribute requires changes to only 4 focused modules
- Schema traits (§9.7) can be added via the same parse-validate-generate pipeline
However, the code blocks at lines 400 and 418 are missing language specifiers. While these are pseudo-structure examples rather than actual code, adding language hints would improve rendering.
Minor: Add language specifiers to code blocks
-``` +```text gts-macros/src/ lib.rs Entry points (current + updated macro) gts_schema_derive.rs #[derive(GtsSchema)] orchestrationand
-``` +```json "x-gts-traits-schema": { "properties": { "topicRef": { "x-gts-ref": "gts.x.core.events.topic.v1~" },As per coding guidelines, this addresses the markdownlint warnings for missing language specifiers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/001-macro-proposal.md` around lines 380 - 418, Add missing Markdown language specifiers to the two fenced code blocks showing the directory layout and the JSON trait example: change the first fence to use "text" (for the gts-macros/src/ listing) and the second fence to use "json" (for the "x-gts-traits-schema"/"x-gts-traits" example). Locate the blocks that contain the "gts-macros/src/" directory listing and the JSON snippet starting with "x-gts-traits-schema" and update their opening triple-backtick fences to ```text and ```json respectively so markdownlint warnings are resolved and syntax rendering improves.docs/001-macro-alignment-implementation-plan.md (1)
60-82: Comprehensive compile-fail test coverage.The 15 compile-fail tests cover all error cases for the new macro. Each test has a clear validation purpose and spec reference. However, the code block at line 78 is missing a language specifier.
Minor: Add language specifier
-| `unknown_gts_attr` | `#[gts(nonexistent)]` | Fail-fast on typos | +| `unknown_gts_attr` | `#[gts(nonexistent)]` (markdown table, no code block) | Fail-fast on typos |Actually, reviewing the context, this appears to be within a markdown table, not a code block. The static analysis warning at line 78 may be a false positive or referring to a different issue. No change needed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/001-macro-alignment-implementation-plan.md` around lines 60 - 82, The reviewer flagged a missing code-block language specifier around the table entry near `nested_direct_serialize_cfg_attr`; inspect the markdown row containing `nested_direct_serialize` / `nested_direct_serialize_cfg_attr` to confirm whether it is truly a fenced code block or just table text; if it's just table content, leave it unchanged and mark the static-analysis warning as a false positive (or add a comment in the PR), but if there is an accidental fenced code block, add the appropriate language specifier (e.g., ```rust) to that fence to silence the warning.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@docs/001-macro-alignment-implementation-plan.md`:
- Around line 60-82: The reviewer flagged a missing code-block language
specifier around the table entry near `nested_direct_serialize_cfg_attr`;
inspect the markdown row containing `nested_direct_serialize` /
`nested_direct_serialize_cfg_attr` to confirm whether it is truly a fenced code
block or just table text; if it's just table content, leave it unchanged and
mark the static-analysis warning as a false positive (or add a comment in the
PR), but if there is an accidental fenced code block, add the appropriate
language specifier (e.g., ```rust) to that fence to silence the warning.
In `@docs/001-macro-proposal.md`:
- Around line 126-174: Add a brief clarifying sentence to the "After" example
explaining that JsonSchema in the derive list is optional because the macro will
auto-add JsonSchema when omitted per the ADR; reference the example struct
BaseEventV1 and the derive attributes (Serialize, Deserialize, JsonSchema,
GtsSchema) and mention the ADR lines about auto-adding JsonSchema so readers
know they can omit JsonSchema and rely on the macro to inject it.
- Around line 380-418: Add missing Markdown language specifiers to the two
fenced code blocks showing the directory layout and the JSON trait example:
change the first fence to use "text" (for the gts-macros/src/ listing) and the
second fence to use "json" (for the "x-gts-traits-schema"/"x-gts-traits"
example). Locate the blocks that contain the "gts-macros/src/" directory listing
and the JSON snippet starting with "x-gts-traits-schema" and update their
opening triple-backtick fences to ```text and ```json respectively so
markdownlint warnings are resolved and syntax rendering improves.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f4d4cf41-1e27-4748-96ae-3af4dd6c9af5
📒 Files selected for processing (4)
.gts-specdocs/001-macro-alignment-adr.mddocs/001-macro-alignment-implementation-plan.mddocs/001-macro-proposal.md
|
|
||
| Categories 4 and 5 require GTS identity fields. But the spec examples include schemas that produce instances with **no GTS identity field at all**: | ||
|
|
||
| - `gts.x.commerce.orders.order.v1.0~` — the Order schema has `id: uuid` (a plain business identifier, not a `GtsInstanceId`) and no `type` field |
There was a problem hiding this comment.
Well, the intention was that all the GTS instances have GTS type encoded into the object field, either as well-known ID, or type field. So I'd say it's rather a bug in the the gts.x.commerce.orders.order.v1.0~ example... Because otherwise, if a want to change my REST API to become, say, RPC (or MCP) API and my service has 2 flavors of orders with 2 schemas, how would I know what schema to be used?
I'd enforce GTS type to be always "deducible" from the objects.
| Categories 4 and 5 require GTS identity fields. But the spec examples include schemas that produce instances with **no GTS identity field at all**: | ||
|
|
||
| - `gts.x.commerce.orders.order.v1.0~` — the Order schema has `id: uuid` (a plain business identifier, not a `GtsInstanceId`) and no `type` field | ||
| - `gts.x.core.idp.contact.v1.0~` — the Contact schema has `id: uuid` and no `type` field |
| }; | ||
| ``` | ||
|
|
||
| **Conclusion**: The `type` field is a runtime value set by the user — it depends on which concrete child type the instance represents. It is not something the macro can or should auto-populate from the struct's own `SCHEMA_ID`. |
There was a problem hiding this comment.
I'm not sure that conclusion reflects original intention. The intention was to generate the type automatically when we create an instance, or at least to minimize error surface not to allow humans to create an instance and put random/irrelevant type there. For example using macros, dylint, and such
| | `orders.order.v1.0~` | UUID | (none) | Plain data entity | | ||
| | `idp.contact.v1.0~` | UUID | (none) | Plain data entity | | ||
|
|
||
| **Conclusion**: There is no single "correct" identity pattern. The choice is domain-specific and implementation-defined ([spec §11.1](https://github.com/GlobalTypeSystem/gts-spec#111-global-rules-schema-vs-instance-normalization-and-document-categories)). The macro should support these patterns, not mandate one. |
There was a problem hiding this comment.
Correct. The id can be:
- uuid or whatever other string
or - GTS ID (for well-known instances)
| 1. **Validates the schema ID format** against GTS identifier rules | ||
| 2. **Validates version consistency** between the struct name suffix (e.g., `V1`) and the schema ID version (e.g., `v1~`) | ||
| 3. **Validates parent-child inheritance** — segment count, parent schema ID match | ||
| 4. **Validates the `properties` list** — every listed property must exist as a struct field |
There was a problem hiding this comment.
The properties list was added intentionally to be able to serialize a subset of the fields only, I don't mind if we can use per-field annotation instead
| - Implement the `GtsSchema` trait (`SCHEMA_ID`, `GENERIC_FIELD`, schema composition methods) | ||
| - Generate `gts_schema_id()`, `gts_base_schema_id()`, `gts_make_instance_id()` | ||
| - Generate `gts_schema_with_refs_as_string()` and similar convenience methods | ||
| - Store `dir_path` and `description` as associated constants for CLI schema generation |
There was a problem hiding this comment.
I believe we also need this:
- validate id/type fields — base structs must have either an
id: GtsInstanceIdor a type field (type/gts_type/etc.) of typeGtsSchemaId, but not both
| - Excludes the field from the generated JSON Schema properties | ||
| - Does not affect serde behavior (use `#[serde(skip)]` for that) | ||
|
|
||
| These attributes are **all optional**. A struct without any `#[gts(type_field)]` or `#[gts(instance_id)]` annotation is valid — it represents a data entity like `order.v1.0~` or `contact.v1.0~` from the spec. |
There was a problem hiding this comment.
let's make at least one mandatory
|
|
||
| ```rust | ||
| #[derive(Debug, Clone, GtsSchema)] | ||
| #[gts( |
There was a problem hiding this comment.
What is going to happen with the 'dir_path' ?
| } | ||
| ``` | ||
|
|
||
| **Migration**: The `properties` parameter is removed. Fields previously omitted from `properties` should add `#[gts(skip)]`. The `dir_path` parameter is retained as it controls file output location for the CLI. |
There was a problem hiding this comment.
I can't see it in examples above. Any changes here?
| #[gts( | ||
| schema_id = "gts.x.core.events.type.v1~x.core.audit.event.v1~", | ||
| description = "...", | ||
| extends = BaseEventV1, |
There was a problem hiding this comment.
The base = true was used to quickly find all the base types in IDE, PR, repo and such
Can we support extends = None or so?
|
|
||
| ### 4.4 Let users control serde | ||
|
|
||
| The current macro silently adds `Serialize`/`Deserialize` derives for base structs and silently removes them for nested structs. This is surprising behavior that fights against Rust conventions. |
There was a problem hiding this comment.
It's not surprising, it's a key thing.
-
We do not want to allow to serialize only nested type, otherwise humans will make mistakes.
We want to serialize only audit event schema as whole, so it means we need to serialize the base type with respect of all the nested objects serializer... -
We do not want to have unserializable GTS entities, because we need to register them all in Type Registry
| #[gts( | ||
| schema_id = "...", | ||
| extends = BaseEventV1, | ||
| allow_direct_serde, // opt-out: allow Serialize/Deserialize |
There was a problem hiding this comment.
I would prohibit it because people will make mistakes, we never (?) need nested objects as separate json
|
|
||
| ## 11. Open Questions | ||
|
|
||
| 1. **CLI impact**: The CLI currently scans source files for `#[struct_to_gts_schema]` annotations to extract metadata (schema ID, properties, description). The new `#[derive(GtsSchema)]` + `#[gts(...)]` pattern requires updating the CLI parser. The removal of `properties` means the CLI must parse struct fields directly (respecting `#[gts(skip)]` and `#[serde(skip)]` attributes). This is deferred for a separate design discussion. |
There was a problem hiding this comment.
Yes, the CLI must be updated respectively
2 participants
This PR contains a proposal to evolve
#[struct_to_gts_schema] into a derive-based#[derive(GtsSchema)]` macro. The goal is to bring the macro into closer alignment with the GTS spec v0.8.0, particularly around identity field requirements.The full proposal is at
docs/001-macro-proposal.md. Some key changes:x-gts-ref: "/$idvsgts.*- the spec distinguishes self-reference from cross-reference. The current macro generatesgts.*for allGtsSchemaIdfields. The new macro would use field annotations such as#[gts(type_field)]or#[gts(instance_id)]to enable this functionalityproperties = "field1,field2"parameterextends = Parentreplacesbase = True / base = Parent- Clearer naming and ifextendsis omitted, the struct is considered root by defaultAll schema output would be structurally identical.
The working implementation is at https://github.com/asmith987/gts-rust/tree/gts-macro-implementation
Relevant docs in the PR:
Summary by CodeRabbit