fix(gts-macros): make GTS type/id field optional and fix deserialization

[asmith987]

Description

The struct_to_gts_schema macro required base structs to have a GTS type or ID field, even when the field had no runtime purpose. The schema ID is already accessible via Self::gts_schema_id() and Self::SCHEMA_ID through the GtsSchema trait. This forced users into manual serde workarounds to avoid deserialization failures.

This change makes the id/type fully optional on base structs. When a GTS field is present, it must be listed in properties, otherwise the macro emits a clear compile error guiding the user to either add it to properties or remove it from the struct.

What changed

• Removed the mandatory type/id field requirement. Base structs now support three patterns. No ID/type field, ID field listed in properties or GTS type field listed in properties.
• Added new validation that catches GTS-typed fields not listed in properties, preventing dead fields that confuse serde.
• Scoped existing validation to properties. Only fields within properties are validated, so fields like id: Uuid are treated as regular fields and would not trigger a GTS field validation.

Why this change was necessary

• Users have to define a dead-code field and manually work around serde:

  #[derive(Debug, Clone)]
  #[struct_to_gts_schema(
     dir_path = "schemas",
     base = true,
     schema_id = "gts.cf.core.errors.quota_failure.v1~",
     description = "Quota failure with one or more violations",
     properties = "violations"
   )]
   pub struct QuotaFailureV1 {
       #[allow(dead_code)]
       #[serde(skip_serializing, default = "dummy_gts_schema_id")]
       gts_type: gts::GtsSchemaId,
       pub violations: Vec<QuotaViolation>,
   }

 // This required writing a standalone dummy_gts_schema_id() function, and using
 //skip_serializing alone still broke deserialization unless default was also specified.
 //Now the same struct can simply be:
 #[derive(Debug, Clone)]
  #[struct_to_gts_schema(
     dir_path = "schemas",
     base = true,
     schema_id = "gts.cf.core.errors.quota_failure.v1~",
     description = "Quota failure with one or more violations",
     properties = "violations"
   )]
 pub struct QuotaFailureV1 {
     pub violations: Vec<QuotaViolation>,
 }

Resolves #72

Type of Change

• Bug fix
• Documentation update

Testing

• Round trip serialization/deserialization tests for base structs with/without id/type fields.
• Verified id: Uuid is not misidentified as a GTS ID Field
• New compile-fail test to verify a GTS-typed field not in properties produces actionable error
• Updated stderr expectations for 7 existing compile-fail tests to reflect improved error surfacing

Checklist

• Tests pass (cargo test)
• Code is formatted (cargo fmt)
• No clippy warnings (cargo clippy)
• Documentation is updated

Summary by CodeRabbit

• New Features

  • Base structs now allow three options: no ID/type, ID in properties, or GTS Type in properties.

• Bug Fixes

  • Validation now enforces that any ID or GTS Type fields present must be listed in properties; structs cannot have both ID and GTS Type in properties.

• Documentation

  • README expanded with clear examples, error scenarios, and guidance for field placement and schema ID access.

• Tests

  • Added and updated tests to cover the new validation rules and serialization behaviors.

[coderabbitai]

📝 Walkthrough

Walkthrough

Refactors struct_to_gts_schema validation to a properties-driven model: base structs may optionally include ID or GTS Type fields, but only fields listed in the macro's properties are treated as GTS fields. Tests and error messages updated; new compile-time check enforces that GTS fields present on a struct are listed in properties.

Changes

Cohort / File(s)	Summary
Documentation & Examples gts-macros/README.md	Rewrote base-struct field validation: optional ID/type presence, three example patterns, rule that any GTS field must be listed in properties, and updated examples and notes about serde/rename.
Core Macro Logic gts-macros/src/lib.rs	Refactored validation to derive property_names from args.properties, updated validator signatures to accept property list, added validate_gts_fields_in_properties helper, and integrated properties-based checks into macro flow.
Removed/Updated Compile-fail Tests gts-macros/tests/compile_fail/base_struct_missing_*.rs, base_struct_wrong_*.rs, associated .stderr files	Deleted old tests that enforced mandatory ID/type fields; adjusted/removed expected error outputs to match new optional-presence rules.
New Compile-fail Test gts-macros/tests/compile_fail/gts_field_not_in_properties.rs, gts_field_not_in_properties.stderr	Added test asserting that a GTS field (e.g., gts_type/id) present on a struct but not listed in properties triggers a compile error.
Updated Error Expectations gts-macros/tests/compile_fail/*.stderr (various files)	Modified error message expectations to reflect property-driven validation, parent/schema_id/version consistency checks, and trait-bound diagnostics.
Integration Tests / New Bases gts-macros/tests/inheritance_tests.rs	Added QuotaFailureV1, RateLimitErrorV1, expanded tests for optional ID/type behaviors, serialization/deserialization shapes, and property-driven inheritance scenarios.

Sequence Diagram(s)

sequenceDiagram
    participant UserMacro as "struct_to_gts_schema (macro)"
    participant PropParser as "Properties Parser"
    participant FieldValidator as "Field Type Validator"
    participant GtsValidator as "GTS Fields-in-Properties Validator"

    UserMacro->>PropParser: parse `properties` arg -> property_names
    PropParser-->>UserMacro: property_names

    UserMacro->>FieldValidator: validate_field_types(fields, property_names)
    FieldValidator-->>UserMacro: field-type validation result

    UserMacro->>GtsValidator: validate_gts_fields_in_properties(struct, base, property_names)
    GtsValidator-->>UserMacro: ensure any GTS fields are listed in properties or emit compile error

    alt GTS fields listed in properties
        UserMacro->>UserMacro: treat listed fields as GTS id/type (apply GTS rules)
    else GTS fields present but not listed
        UserMacro-->>UserMacro: emit compile-time error
    end

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Possibly related PRs

• feat(macros): add compile-time GTS ID validation to struct_to_gts_schema (#47) #51: Modifies struct_to_gts_schema macro logic similarly; relevant for overlapping validation and schema-id parsing changes.
• Issue #72 (linked): Discusses removing or adjusting the injected gts_type field and serde behavior; closely related to changes that make GTS fields optional and properties-driven.

Suggested reviewers

• MikeFalcon77

Poem

"I hop and nibble schema dots,
properties lined in tidy slots.
ID or type — pick one or none,
the meadow's rules now let you run.
🐇✨"

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'fix(gts-macros): make GTS type/id field optional and fix deserialization' clearly and specifically describes the main change: making GTS type/id fields optional and fixing deserialization issues in the struct_to_gts_schema macro.
Linked Issues check	✅ Passed	The PR fully addresses issue #72: makes GTS type/id fields optional, implements automatic serde skip/default behavior for absent fields, and resolves deserialization failures via the GtsSchema trait's gts_schema_id() method.
Out of Scope Changes check	✅ Passed	All changes align with the stated objective: documentation updates clarify the new optional field model, code changes implement properties-driven validation, test removals eliminate obsolete validation rules, and new tests verify the optional field behavior and deserialization correctness.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@gts-macros/src/lib.rs`:
- Around line 628-631: The function add_serde_skip_for_gts_fields currently
pushes a fresh #[serde(skip, default)] onto field.attrs when a field is not in
property_names, which can create duplicate serde options if the field already
has a serde attribute; update the logic in add_serde_skip_for_gts_fields to
inspect existing attributes in field.attrs for an attribute with path "serde"
and parse its meta to determine if "skip" and/or "default" are already present,
then only add the missing option(s) (either by appending the missing tokens to
the existing serde attribute or by creating a single serde attribute that merges
existing and missing options) so that duplicate serde options are never produced
for the same field_name and serde attribute.

In `@gts/src/gts.rs`:
- Around line 477-481: The Default impls for GtsInstanceId (and the same for
GtsSchemaId) construct an invalid empty ID; remove these impls instead of
returning Self(GtsEntityId::new("")) so core ID types never expose invalid
defaults. Delete the impl Default blocks for GtsInstanceId and GtsSchemaId and
update any code/tests that relied on Default to supply IDs explicitly or use the
macro-generated field defaults/Option at the call site for skipped metadata
fields.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 075b3c7c-b3d3-46c3-a010-d41a1905c5b6

📥 Commits

Reviewing files that changed from the base of the PR and between cd8ed5b and c078e24.

📒 Files selected for processing (18)

• gts-macros/README.md
• gts-macros/src/lib.rs
• gts-macros/tests/compile_fail/base_parent_mismatch.stderr
• gts-macros/tests/compile_fail/base_parent_single_segment.stderr
• gts-macros/tests/compile_fail/base_struct_both_id_and_type.stderr
• gts-macros/tests/compile_fail/base_struct_missing_id.rs
• gts-macros/tests/compile_fail/base_struct_missing_id.stderr
• gts-macros/tests/compile_fail/base_struct_wrong_gts_type.rs
• gts-macros/tests/compile_fail/base_struct_wrong_gts_type.stderr
• gts-macros/tests/compile_fail/base_struct_wrong_id_type.rs
• gts-macros/tests/compile_fail/base_struct_wrong_id_type.stderr
• gts-macros/tests/compile_fail/base_true_multi_segment.stderr
• gts-macros/tests/compile_fail/non_gts_generic.stderr
• gts-macros/tests/compile_fail/version_mismatch_major.stderr
• gts-macros/tests/compile_fail/version_mismatch_minor_missing_schema.stderr
• gts-macros/tests/compile_fail/version_mismatch_minor_missing_struct.stderr
• gts-macros/tests/inheritance_tests.rs
• gts/src/gts.rs

💤 Files with no reviewable changes (7)

• gts-macros/tests/compile_fail/base_struct_wrong_gts_type.rs
• gts-macros/tests/compile_fail/base_struct_wrong_gts_type.stderr
• gts-macros/tests/compile_fail/base_struct_missing_id.rs
• gts-macros/tests/compile_fail/base_struct_wrong_id_type.stderr
• gts-macros/tests/compile_fail/base_struct_wrong_id_type.rs
• gts-macros/tests/compile_fail/base_parent_single_segment.stderr
• gts-macros/tests/compile_fail/base_struct_missing_id.stderr

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (2)

gts-macros/src/lib.rs (1)

238-243: Extract shared properties parsing into one helper.

properties parsing is duplicated in multiple places. Centralizing this into a helper reduces drift risk and keeps validation behavior consistent.

Also applies to: 607-611, 906-911

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@gts-macros/src/lib.rs` around lines 238 - 243, Extract the duplicated parsing
logic into a single helper function (e.g., fn parse_property_names(input: &str)
-> Vec<String>) that trims, splits on ',', filters out empty entries, and
returns Vec<String>; replace the inline code in places that currently do the
split/trim/filter (the block creating property_names from args.properties and
the similar blocks at the other occurrences) with calls to this helper (and
update callers to pass &str where needed), and keep validation behavior
identical by reusing the same helper wherever properties are parsed.

gts-macros/tests/inheritance_tests.rs (1)

169-169: Tighten regression coverage for the new id property inclusion.

Now that TopicV1 includes id in properties, please also assert id in the schema-field-path expectation to lock this behavior and prevent silent regressions.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@gts-macros/tests/inheritance_tests.rs` at line 169, Update the test that
checks schema-field-path expectations for TopicV1 to include an assertion for
the new "id" property: locate the test referencing TopicV1 and the variable used
for the expected schema field paths (e.g., the schema-field-path expectation
array or set) and add "id" to that expectation so the test fails if "id" is ever
removed; ensure the assertion references TopicV1 and the same expectation
variable name used in the test so the coverage is tightened for the new
property.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@gts-macros/src/lib.rs`:
- Around line 628-643: The current code in struct_to_gts_schema that checks
(is_gts_id_field || is_gts_type_field) and returns Err when field_name isn't in
property_names causes a hard compile error; change this to allow the shape
non-fatally by removing the Err return and instead skipping/ignoring that field
so code generation continues (i.e., do not treat GTS id/type fields missing from
properties as a fatal error). If you want to surface diagnostics, emit a
non-blocking warning via proc_macro_error::emit_warning (or equivalent)
referencing ident and input.ident, but do not abort macro expansion.

---

Nitpick comments:
In `@gts-macros/src/lib.rs`:
- Around line 238-243: Extract the duplicated parsing logic into a single helper
function (e.g., fn parse_property_names(input: &str) -> Vec<String>) that trims,
splits on ',', filters out empty entries, and returns Vec<String>; replace the
inline code in places that currently do the split/trim/filter (the block
creating property_names from args.properties and the similar blocks at the other
occurrences) with calls to this helper (and update callers to pass &str where
needed), and keep validation behavior identical by reusing the same helper
wherever properties are parsed.

In `@gts-macros/tests/inheritance_tests.rs`:
- Line 169: Update the test that checks schema-field-path expectations for
TopicV1 to include an assertion for the new "id" property: locate the test
referencing TopicV1 and the variable used for the expected schema field paths
(e.g., the schema-field-path expectation array or set) and add "id" to that
expectation so the test fails if "id" is ever removed; ensure the assertion
references TopicV1 and the same expectation variable name used in the test so
the coverage is tightened for the new property.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 2c8a7f5c-53b2-4094-847c-f67ae00bc749

📥 Commits

Reviewing files that changed from the base of the PR and between c078e24 and 5d0a0a1.

📒 Files selected for processing (19)

• gts-macros/README.md
• gts-macros/src/lib.rs
• gts-macros/tests/compile_fail/base_parent_mismatch.stderr
• gts-macros/tests/compile_fail/base_parent_single_segment.stderr
• gts-macros/tests/compile_fail/base_struct_both_id_and_type.stderr
• gts-macros/tests/compile_fail/base_struct_missing_id.rs
• gts-macros/tests/compile_fail/base_struct_missing_id.stderr
• gts-macros/tests/compile_fail/base_struct_wrong_gts_type.rs
• gts-macros/tests/compile_fail/base_struct_wrong_gts_type.stderr
• gts-macros/tests/compile_fail/base_struct_wrong_id_type.rs
• gts-macros/tests/compile_fail/base_struct_wrong_id_type.stderr
• gts-macros/tests/compile_fail/base_true_multi_segment.stderr
• gts-macros/tests/compile_fail/gts_field_not_in_properties.rs
• gts-macros/tests/compile_fail/gts_field_not_in_properties.stderr
• gts-macros/tests/compile_fail/non_gts_generic.stderr
• gts-macros/tests/compile_fail/version_mismatch_major.stderr
• gts-macros/tests/compile_fail/version_mismatch_minor_missing_schema.stderr
• gts-macros/tests/compile_fail/version_mismatch_minor_missing_struct.stderr
• gts-macros/tests/inheritance_tests.rs

💤 Files with no reviewable changes (7)

• gts-macros/tests/compile_fail/base_struct_wrong_id_type.stderr
• gts-macros/tests/compile_fail/base_struct_missing_id.rs
• gts-macros/tests/compile_fail/base_struct_wrong_gts_type.rs
• gts-macros/tests/compile_fail/base_struct_missing_id.stderr
• gts-macros/tests/compile_fail/base_struct_wrong_gts_type.stderr
• gts-macros/tests/compile_fail/base_struct_wrong_id_type.rs
• gts-macros/tests/compile_fail/base_parent_single_segment.stderr

🚧 Files skipped from review as they are similar to previous changes (1)

• gts-macros/tests/compile_fail/base_true_multi_segment.stderr

[coderabbitai]

Actionable comments posted: 1

♻️ Duplicate comments (1)

gts-macros/src/lib.rs (1)

628-643: ⚠️ Potential issue | 🟠 Major

Hard compile rejection regresses the optional GTS field migration path.

This branch aborts macro expansion when a GtsSchemaId/GtsInstanceId field exists but is not listed in properties, which conflicts with the stated optional-field behavior and can break existing base structs that intentionally keep these fields out of schema properties. Please make this non-fatal and keep the deserialization-safe omission path instead of returning an error here.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@gts-macros/src/lib.rs` around lines 628 - 643, The macro currently returns
Err when a field with is_gts_id_field or is_gts_type_field is missing from
property_names, which hard-fails expansion; change this to a non-fatal path so
omitted GtsSchemaId/GtsInstanceId fields are allowed: instead of returning Err
in the branch that references is_gts_id_field, is_gts_type_field,
property_names, field_name, and type_desc, skip emitting an error and either
continue processing the field (i.e., treat it as a non-property/optional field)
or emit a compile-time warning using the span (ident) rather than aborting;
ensure downstream code that expects optional omission still behaves the same.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@gts-macros/README.md`:
- Around line 261-266: The fenced error-output block containing the message
starting with "error: struct_to_gts_schema: Field `gts_type`..." is missing a
language tag; update the opening fence from ``` to ```text so the block is
marked as plain text (this will satisfy markdownlint MD040). Locate the block
showing the struct_to_gts_schema error and change its fence accordingly; no
other content changes are required (the block references
RateLimitErrorV1::gts_schema_id() and RateLimitErrorV1::SCHEMA_ID for context).

---

Duplicate comments:
In `@gts-macros/src/lib.rs`:
- Around line 628-643: The macro currently returns Err when a field with
is_gts_id_field or is_gts_type_field is missing from property_names, which
hard-fails expansion; change this to a non-fatal path so omitted
GtsSchemaId/GtsInstanceId fields are allowed: instead of returning Err in the
branch that references is_gts_id_field, is_gts_type_field, property_names,
field_name, and type_desc, skip emitting an error and either continue processing
the field (i.e., treat it as a non-property/optional field) or emit a
compile-time warning using the span (ident) rather than aborting; ensure
downstream code that expects optional omission still behaves the same.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 1fefb5f5-c4dc-43d8-bd3f-281109d306d4

📥 Commits

Reviewing files that changed from the base of the PR and between 5d0a0a1 and e905b30.

📒 Files selected for processing (19)

• gts-macros/README.md
• gts-macros/src/lib.rs
• gts-macros/tests/compile_fail/base_parent_mismatch.stderr
• gts-macros/tests/compile_fail/base_parent_single_segment.stderr
• gts-macros/tests/compile_fail/base_struct_both_id_and_type.stderr
• gts-macros/tests/compile_fail/base_struct_missing_id.rs
• gts-macros/tests/compile_fail/base_struct_missing_id.stderr
• gts-macros/tests/compile_fail/base_struct_wrong_gts_type.rs
• gts-macros/tests/compile_fail/base_struct_wrong_gts_type.stderr
• gts-macros/tests/compile_fail/base_struct_wrong_id_type.rs
• gts-macros/tests/compile_fail/base_struct_wrong_id_type.stderr
• gts-macros/tests/compile_fail/base_true_multi_segment.stderr
• gts-macros/tests/compile_fail/gts_field_not_in_properties.rs
• gts-macros/tests/compile_fail/gts_field_not_in_properties.stderr
• gts-macros/tests/compile_fail/non_gts_generic.stderr
• gts-macros/tests/compile_fail/version_mismatch_major.stderr
• gts-macros/tests/compile_fail/version_mismatch_minor_missing_schema.stderr
• gts-macros/tests/compile_fail/version_mismatch_minor_missing_struct.stderr
• gts-macros/tests/inheritance_tests.rs

💤 Files with no reviewable changes (7)

• gts-macros/tests/compile_fail/base_struct_missing_id.rs
• gts-macros/tests/compile_fail/base_struct_wrong_id_type.rs
• gts-macros/tests/compile_fail/base_parent_single_segment.stderr
• gts-macros/tests/compile_fail/base_struct_wrong_id_type.stderr
• gts-macros/tests/compile_fail/base_struct_wrong_gts_type.stderr
• gts-macros/tests/compile_fail/base_struct_missing_id.stderr
• gts-macros/tests/compile_fail/base_struct_wrong_gts_type.rs

🚧 Files skipped from review as they are similar to previous changes (4)

• gts-macros/tests/compile_fail/gts_field_not_in_properties.rs
• gts-macros/tests/compile_fail/version_mismatch_minor_missing_struct.stderr
• gts-macros/tests/compile_fail/base_true_multi_segment.stderr
• gts-macros/tests/compile_fail/gts_field_not_in_properties.stderr