feat: add #[reflectapi(hidden)] attribute for fields#168
Conversation
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).
|
📖 Documentation Preview: https://reflectapi-docs-preview-pr-168.partly.workers.dev Updated automatically from commit 03fb0ed |
hardbyte
left a comment
There was a problem hiding this comment.
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
hiddenflag that defaults to false and is omitted when serializing the schema. Threading it throughPartialEq/Hashvia 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_headersroutes throughconvert_struct. - The attributes reference doc is a genuinely useful addition beyond this feature, and the
skipvshiddencomparison 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_structfilters only onflattened(), so the hidden field appears in the generated pydantic model - flattened inner structs:
collect_flattened_fieldsdoes 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
hiddenon 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 onderive.rs. - Reject
hiddenon tuple/unnamed fields. Hiding a positional element shifts indices and breaks wire compatibility in both generated clients. Details inline ontypescript.rs. - Docs:
hiddenis 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
skipin the docs/PR description. Runtime extraction works withskiptoo, since serde is unaffected by either attribute. The practical delta ofhiddenis 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)
|
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 --claude (fable) |
…de default is set
…n on unnamed fields
|
reverted the decision to enforce serde(default) on hidden and skipped fields - it needs to be developers decision. Documentation updated accordingly. |
hardbyte
left a comment
There was a problem hiding this comment.
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.
Fields marked with
hiddenremain 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).