Skip to content

feat: add #[reflectapi(hidden)] attribute for fields#168

Merged
hardbyte merged 8 commits into
mainfrom
andrey/exploratory-add-hidden-field-attribute
Jun 11, 2026
Merged

feat: add #[reflectapi(hidden)] attribute for fields#168
hardbyte merged 8 commits into
mainfrom
andrey/exploratory-add-hidden-field-attribute

Conversation

@avkonst

@avkonst avkonst commented Jun 10, 2026

Copy link
Copy Markdown
Collaborator

Fields marked with hidden remain in the schema (with "hidden": true) and are functional at runtime (e.g. header extraction by the axum adapter), but are excluded from generated clients, docs, and OpenAPI specs.

This is distinct from #[reflectapi(skip)] which fully excludes the field from the schema (for fields whose types don't implement reflectapi traits).

Fields marked with `hidden` remain in the schema (with `"hidden": true`)
and are functional at runtime (e.g. header extraction by the axum adapter),
but are excluded from generated clients, docs, and OpenAPI specs.

This is distinct from `#[reflectapi(skip)]` which fully excludes the field
from the schema (for fields whose types don't implement reflectapi traits).
@github-actions

github-actions Bot commented Jun 10, 2026

Copy link
Copy Markdown

📖 Documentation Preview: https://reflectapi-docs-preview-pr-168.partly.workers.dev

Updated automatically from commit 03fb0ed

@avkonst avkonst requested a review from hardbyte June 10, 2026 07:20

@hardbyte hardbyte left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This review was carried out with Claude; every finding below was reproduced empirically against the head commit (933b7f3) rather than just read from the diff.

What's good

  • The schema-level design is clean and backwards compatible: an additive hidden flag that defaults to false and is omitted when serializing the schema. Threading it through PartialEq / Hash via exhaustive destructuring means the compiler will catch future omissions when fields are added.
  • TypeScript, Rust client, and OpenAPI filtering are correct, including enum variants. OpenAPI header parameters get filtered for free because convert_headers routes through convert_struct.
  • The attributes reference doc is a genuinely useful addition beyond this feature, and the skip vs hidden comparison is well written.
  • Good drive-by fix of test_reflectapi_struct_one_basic_field_string_reflectapi_both_equally2, which was snapshotting the wrong type.

Blocking

1. Python codegen still leaks hidden fields in the common paths. The filters added in python.rs only cover the flatten-specific render functions. Verified leaks on this branch (details inline on the file):

  • plain structs (no flatten): render_struct filters only on flattened(), so the hidden field appears in the generated pydantic model
  • flattened inner structs: collect_flattened_fields does not filter
  • enum variants: no filtering at all (leaks for both internally and externally tagged enums)

2. The new test cannot catch this. assert_builder_snapshot! generates schema, TypeScript, Rust, and OpenAPI snapshots but not Python, so the only test for this feature never exercises the Python generator. That is why CI is green despite the above.

Design suggestion: filter once at the schema boundary

The root cause of blocking item 1 is architectural rather than a one-off oversight. As implemented, "hidden" is an invariant that every codegen backend (and every future backend) must independently remember at every field-iteration site; python.rs alone has 20+ such sites. I would suggest inverting it: add a strip_hidden() transformation in reflectapi-schema (similar in spirit to fold_transparent_types) and apply it once at each codegen entry point. Then no backend can leak by omission, all the per-backend filters can be deleted, and any tooling that wants to see hidden fields can read the raw schema. This would resolve all three Python leaks by construction.

Other suggestions

  • Guard hidden on required input fields at compile time. Without #[serde(default)] the contract is unrepresentable: clients cannot send the field but the server requires it, so every generated-client request fails at runtime. Details inline on derive.rs.
  • Reject hidden on tuple/unnamed fields. Hiding a positional element shifts indices and breaks wire compatibility in both generated clients. Details inline on typescript.rs.
  • Docs: hidden is not redaction. For output types serde still serializes the field onto the wire; it is only hidden from generated code and docs. Worth an explicit caveat. Details inline on the docs page.
  • Clarify the motivation vs skip in the docs/PR description. Runtime extraction works with skip too, since serde is unaffected by either attribute. The practical delta of hidden is that the field stays visible in the schema JSON, so it would help to state what consumes that, as it is the feature's main justification.

--claude (fable)

Comment thread reflectapi/src/codegen/python.rs Outdated
Comment thread reflectapi-demo/src/tests/basic.rs
Comment thread reflectapi-derive/src/derive.rs
Comment thread reflectapi/src/codegen/typescript.rs Outdated
Comment thread docs/src/getting-started/attributes.md Outdated
@hardbyte

Copy link
Copy Markdown
Contributor

One more architectural note: this PR connects directly to #129. "Excluded from generated clients" is shared, backend-independent meaning, and implementing it as render-time filters inside each backend is exactly the drift pattern that issue describes. The verified Python leaks are a concrete example of the failure mode: three backends filtered correctly, the one with the most raw-schema traversal sites missed several.

The strip_hidden() transformation suggested in the review is effectively a first increment of #129's direction: resolve visibility once in a shared stage before codegen, so no backend can leak by omission (including OpenAPI, which bypasses the semantic layer entirely today). When backends later migrate to the SemanticSchema contract, that strip just moves into normalization and the per-backend filters never need to exist.

--claude (fable)

@avkonst

avkonst commented Jun 11, 2026

Copy link
Copy Markdown
Collaborator Author

reverted the decision to enforce serde(default) on hidden and skipped fields - it needs to be developers decision. Documentation updated accordingly.

@hardbyte hardbyte left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All findings addressed: schema-level strip_hidden_fields covers every backend path, tuple fields are a compile error with trybuild coverage, and the serde(default) trade-off is now clearly documented as a deliberate choice. Python sample coverage lands separately in #170.

@hardbyte hardbyte merged commit d9c8f72 into main Jun 11, 2026
6 checks passed
@hardbyte hardbyte mentioned this pull request Jun 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants