RFC: emit option-typed fields as optional properties in TypeScript clients#174
RFC: emit option-typed fields as optional properties in TypeScript clients#174hardbyte wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f7297141d4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| description: doc_to_ts_comments(&field.description, field.deprecation_note.as_deref(), 4), | ||
| type_: type_ref_to_ts_ref(&field.type_ref, schema, implemented_types), | ||
| optional: !field.required, | ||
| optional: !field.required || is_option_type, |
There was a problem hiding this comment.
Preserve requiredness for custom-deserialized options
When an input field is Option<T> but uses a custom serde adapter such as #[serde(with = "...")] / #[serde(deserialize_with = "...")] without #[serde(default)], serde does not reliably apply the standard Option missing-field path; common adapters reject an omitted key, and the schema already records that as field.required == true. This line overrides that requiredness purely from the reflected type name, so generated TypeScript clients can omit a field that the server will reject at deserialization time. Please keep required authoritative for option fields whose serde deserialization is not known to be the standard/default path.
Useful? React with 👍 / 👎.
|
📖 Documentation Preview: https://reflectapi-docs-preview-pr-174.partly.workers.dev Updated automatically from commit 0bb866b |
|
I'm fine with this, I don't recall the former discussion :) I know serde special cases I don't know why it works without the #[derive(Debug, serde::Serialize, serde::Deserialize)]
struct S {
#[serde(default)] // take this in and out and see the change
x: reflectapi::Option<String>,
y: Option<String>,
}
fn main() -> Result<(), Box<dyn std::error::Error>> {
let s = serde_json::from_value::<S>(serde_json::json!({}))?;
dbg!(&s);
Ok(())
} |
…ce properties serde accepts a missing key for option-typed fields unconditionally: missing_field special-cases deserialize_option, so a missing std::option::Option deserializes to None even without #[serde(default)], and reflectapi::Option behaves the same. The generated TypeScript interfaces nevertheless required these keys unless skip_serializing_if was present, forcing callers to pass explicit nulls that the server never needed. Mark option-typed fields optional (field?: T | null) unconditionally in field_to_ts_field, mirroring the Python backend's resolve_field_optionality. Adds a test sample covering all four annotation combinations of std and reflectapi options.
f729714 to
8698687
Compare
…s with custom serde codecs serde's missing_field path cannot route through serialize_with/deserialize_with, so an option-typed field with a custom codec and no #[serde(default)] rejects a missing key — the optional-promotion must not apply to it. The derive now records a custom_codec flag on schema fields whenever a serde with-family attribute is present (direction-agnostic so input/output reflections stay identical for type consolidation), and the TypeScript backend skips the option promotion for such fields. Extends the test sample with a deserialize_with field that stays required.
…layer Field optionality was re-derived independently by each backend from (required, type name, custom_codec) heuristics, and the copies drifted: Python knew option-typed fields are omittable but TypeScript did not, and neither knew custom serde codecs reject missing keys until patched separately. Introduce resolve_field_wire_contract in the compiler-side schema layer, resolving a field into three orthogonal facts: key presence, value nullability, and whether absence is distinct from null. The TypeScript and Python backends now render from this single resolution; Python's resolve_field_optionality becomes a thin mapping from the contract to Python syntax. This also closes the pre-existing Python gap where option fields with deserialize_with were generated as omittable keys the server rejects. First concrete step toward the SemanticSchema-as-backend-contract direction (#129): backend-independent meaning computed once in the shared layer, backend-specific rendering kept local.
…d of a custom_codec flag The raw schema should record facts; interpretation belongs to the compiler layer. The single direction-agnostic custom_codec flag was a pre-interpreted blend of two facts, and a lossy one: missing-key acceptance is purely a deserialize-side question, so a field with only a custom serializer was wrongly demoted to a required key. Record the presence of the serde attributes literally (serialize_with, deserialize_with; with sets both) and key the wire-contract carve-out on deserialize_with alone. Also adds both flags to Field's Hash impl, which the previous flag had missed.
- resolve_field_wire_contract now takes a FieldWireFacts input naming exactly the facts resolution depends on (type name, deserialize_with, folded required), removing the redundant required parameter and the synthetic Field construction in the Python flattened-enum path. - Rename the contract's nullable to declared_nullable and document that whether null appears on the wire is direction-dependent. - Adopt absent_distinct_from_null: Python's is_partial sites and the ReflectapiPartialModel import detection resolve through the contract instead of string-comparing the reflectapi::Option type name. - Update the Field::required quadrant doc in reflectapi-schema to state the deserialize-side leniency the generated clients now reflect. - Extend the test sample to cover serialize_with and with end to end: a custom serializer alone leaves the key optional, with stays required, and the schema snapshot pins that with sets both flags. The sample now reflects both input and output typespaces. - Expand the changelog entry: Python deserialize_with fix, new schema Field flags, and the shared resolution pass. - Make the presence module pub(crate) so its types are nameable, drop a speculative lint reference from its docs, and comment why Python renders absent-but-not-nullable fields with a None default.
Addresses #76; first concrete step toward #129. Opening this as a proposal — the issue thread has earlier maintainer pushback (@avkonst, @andyyu2004), so this PR implements one direction and lays out the alternatives; happy to rework toward another option if you disagree with the premise.
New evidence since the Feb 2025 discussion
The earlier position was "not doing
serde(default)means the server expects it, so required is correct." I tested this against serde directly, and it does not hold for plain option fields — serde accepts a missing key for option-typed fields unconditionally, unless a custom serde deserializer is involved:#[serde(default)]std_option: Option<String>Ok(None)—missing_fieldspecial-casesdeserialize_optionreflect_option: reflectapi::Option<String>Ok(None)(withdefaultit becomesUndefined)opt: Option<String>+#[serde(deserialize_with = ...)]Err("missing field")—missing_fieldcan't route through a custom deserializerRunnable example demonstrating the serde behaviour
Cargo.toml:src/main.rs:Output (
cargo run):Two things worth noting:
reflectapi::Optionwithoutserde(default): a missing key still deserializes fine, but collapses toNoneinstead ofUndefined. Mechanically:reflectapi::Option::deserializecallsdeserializer.deserialize_option(...), and serde'sMissingFieldDeserializeronly supportsdeserialize_option→visit_none(), which the visitor maps toNone.Undefinedis unreachable through deserialization — it only ever arises fromDefault::default()via#[serde(default)]. This is the footgun @andyyu2004 found, and why a derive-time lint (alternative B) is still worth doing separately.deserialize_withcase is why the optional-promotion in this PR has a carve-out (see below).So a client can always omit plain option keys without a protocol error, and the generated
field: T \| null(no?) is stricter than the server's actual contract.Worth keeping in mind: the input/output
requiredasymmetry mirrors a real serde asymmetry. Deserialization is lenient (a missing option key becomesNoneregardless of annotations), while serialization is deterministic (Noneis emitted as an explicitnullunlessskip_serializing_ifsuppresses it). "Optional" is genuinely true on the input side and genuinely false on the output side for the same unannotated field — and a single consolidated interface has to pick one.Implemented direction (A): option ⇒ optional, resolved once in the shared schema layer
The semantic decomposes into three orthogonal facts per field — key presence, value nullability, and whether absence is distinct from null. The four required×nullable quadrants are already documented on
Field::requiredinreflectapi-schema; what was missing is that the model lived in a doc comment while each backend re-derived it independently — which is exactly how #167 (Python had the rule in one render path, not four others), #76 (TS never had it), and the deserializer gap below happened.This PR therefore implements the optionality rule as a shared wire-contract resolution in the compiler-side schema layer (
codegen/schema/presence.rs, with the full truth table in its doc comment), per the #129 direction — backend-independent meaning computed once, backend-specific rendering kept local:resolve_field_wire_contract(field, effective_required) → { key, nullable, absent_distinct_from_null }key == Optionalto?(nullability is already carried by the type mapping)resolve_field_optionalityis now a thin mapping from the contract to= Nonedefaults and| NonehintsFor the issue's repro this produces exactly the expected shape:
Custom-deserializer carve-out (from Codex's review, verified real): an option field with
#[serde(with/deserialize_with)]and nodefaultrejects a missing key —missing_fieldcannot route through a custom deserializer — so promoting it to optional would let clients omit a key the server 400s on. The derive records the raw facts on schema fields:serialize_withanddeserialize_withbooleans named literally after the serde attributes whose presence they record (additive, mirroring howhiddenwas added; recorded identically on input/output reflections so type consolidation is unaffected). The shared contract keys the carve-out ondeserialize_withalone — a custom serializer never affects whether a key may be absent, so an option field with onlyserialize_withstill renders optional. Because the rule now lives in one place, this also fixes the pre-existing Python gap: option fields withdeserialize_withwere generated as omittable keys the server rejects.Known trade-off: consolidated types are shared between input and output typespaces, so output-side consumers also see
?on option fields that the server in fact always sends (when there's noskip_serializing_if). Consumers already must null-check these fields (T | null), so the practical cost is== null-style checks instead of=== null.Alternatives considered
(B) Derive-time assert requiring
#[serde(default)]onreflectapi::Optionfields (@avkonst's suggestion in the thread). Orthogonal and still worth doing — withoutdefault, a missing key deserializes toNonerather thanUndefined, silently collapsing the three-state type to two states (see the runnable example above). But it's a breaking change for existing code and doesn't fix thestd::option::Optionergonomics at all. Natural follow-up on top of this PR.(C) Direction-aware optionality with type splitting —
?only in input typespaces, keep output precise, splitting every consolidated type containing option fields into input/output variants. Most type-theoretically accurate, but generates two interfaces (name churn for consumers) wherever today there is one. If ever wanted, the shared contract is now the single place to add direction-awareness.(C-lite) Direction-aware without splitting — keep
?for input-only and shared types, drop it only for output-only types, where the server demonstrably always emits the key. The machinery exists (schema.is_input_type/is_output_typesurvive consolidation), and with the shared resolver it's now a one-place change:field?: T | nullfield?: T | nullfield?: T | nullfield: T | nullfield?: T | nullfield?: T | nullThe cost is shape stability: whether a type is "output-only" depends on which endpoints exist, not on the type itself. Adding one endpoint that accepts an existing response type as input silently flips that type from
field: T | nulltofield?: T | nullacross every consumer codebase. (A) is stable under that kind of API evolution; (C-lite) trades stability for precision on output-only types. Happy to implement this instead if you prefer the trade.(D) Status quo + documentation ("just add
serde(default)"). Leaves generated types stricter than the wire contract, keeps backends inconsistent with each other, and the ceremony multiplies across every request struct with optional fields.Not covered here (pre-existing, separate): flattened
Option<Struct>fields without annotations still render viaNullToEmptyObjectrather thanPartial, in both TS and Python.Tests & validation
test_struct_with_option_fields_without_serde_defaultwith the issue's exact repro plus adeserialize_withfield (schema/TS/Rust/Python snapshots) — the four plain option fields render optional in both TS and Python, the custom-deserializer field stays required in both.resolve_field_wire_contractcovering the truth table (plain/option/reflectapi-option/custom serializer/custom deserializer × required).reflectapi = "0.17.6", including the custom-deserializer failure mode.tsc --strictvalidation: the pre-fix interface rejects{}(verified via@ts-expect-error), the post-fix interface accepts{}and explicit values.field: T | null→field?: T | nulladdition. Existing Python snapshots are byte-identical (the shared contract reproduces Python's previous behavior exactly, minus the deserializer gap). 195/195 demo tests pass; fmt/clippy/build clean; full suite verified locally withCI=1(typechecking enabled).reflectapi::Optionenum — the new sample is the first to put areflectapi::Optionfield through the Rust typecheck pass.