Skip to content

RFC: emit option-typed fields as optional properties in TypeScript clients#174

Open
hardbyte wants to merge 5 commits into
mainfrom
fix/typescript-option-fields-optional
Open

RFC: emit option-typed fields as optional properties in TypeScript clients#174
hardbyte wants to merge 5 commits into
mainfrom
fix/typescript-option-fields-optional

Conversation

@hardbyte

@hardbyte hardbyte commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

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:

Field Missing key, no #[serde(default)]
std_option: Option<String> Ok(None)missing_field special-cases deserialize_option
reflect_option: reflectapi::Option<String> Ok(None) (with default it becomes Undefined)
opt: Option<String> + #[serde(deserialize_with = ...)] Err("missing field")missing_field can't route through a custom deserializer
Runnable example demonstrating the serde behaviour

Cargo.toml:

[package]
name = "serde-option-check"
version = "0.1.0"
edition = "2021"

[dependencies]
serde = { version = "1", features = ["derive"] }
serde_json = "1"
reflectapi = "0.17.6"

src/main.rs:

//! What does serde do with a MISSING key for option-typed fields,
//! without `#[serde(default)]`?

use serde::Deserialize;

#[derive(Debug, Deserialize)]
#[serde(deny_unknown_fields)]
struct StdOpt {
    std_option: Option<String>,
}

#[derive(Debug, Deserialize)]
#[serde(deny_unknown_fields)]
struct ReflectOpt {
    reflect_option: reflectapi::Option<String>,
}

#[derive(Debug, Deserialize)]
#[serde(deny_unknown_fields)]
struct ReflectOptDefault {
    #[serde(default)]
    reflect_option: reflectapi::Option<String>,
}

// Custom deserializer: missing key errors, because serde's missing-field
// machinery cannot route through deserialize_with.
fn de_opt<'de, D: serde::Deserializer<'de>>(d: D) -> Result<Option<String>, D::Error> {
    Option::<String>::deserialize(d)
}

#[derive(Debug, Deserialize)]
struct WithCustom {
    #[serde(deserialize_with = "de_opt")]
    opt: Option<String>,
}

fn main() {
    let r = serde_json::from_str::<StdOpt>("{}");
    println!("std Option, missing key, no default      : {r:?}");

    let r = serde_json::from_str::<StdOpt>(r#"{"std_option": null}"#);
    println!("std Option, explicit null                : {r:?}");

    let r = serde_json::from_str::<ReflectOpt>("{}");
    println!("reflectapi::Option, missing, no default  : {r:?}");

    let r = serde_json::from_str::<ReflectOpt>(r#"{"reflect_option": null}"#);
    println!("reflectapi::Option, explicit null        : {r:?}");

    let r = serde_json::from_str::<ReflectOptDefault>("{}");
    println!("reflectapi::Option, missing, with default: {r:?}");

    let r = serde_json::from_str::<WithCustom>("{}");
    println!("Option + deserialize_with, missing key   : {r:?}");

    let r = serde_json::from_str::<WithCustom>(r#"{"opt": null}"#);
    println!("Option + deserialize_with, explicit null : {r:?}");
}

Output (cargo run):

std Option, missing key, no default      : Ok(StdOpt { std_option: None })
std Option, explicit null                : Ok(StdOpt { std_option: None })
reflectapi::Option, missing, no default  : Ok(ReflectOpt { reflect_option: None })
reflectapi::Option, explicit null        : Ok(ReflectOpt { reflect_option: None })
reflectapi::Option, missing, with default: Ok(ReflectOptDefault { reflect_option: Undefined })
Option + deserialize_with, missing key   : Err(Error("missing field `opt`", line: 1, column: 2))
Option + deserialize_with, explicit null : Ok(WithCustom { opt: None })

Two things worth noting:

  1. reflectapi::Option without serde(default): a missing key still deserializes fine, but collapses to None instead of Undefined. Mechanically: reflectapi::Option::deserialize calls deserializer.deserialize_option(...), and serde's MissingFieldDeserializer only supports deserialize_optionvisit_none(), which the visitor maps to None. Undefined is unreachable through deserialization — it only ever arises from Default::default() via #[serde(default)]. This is the footgun @andyyu2004 found, and why a derive-time lint (alternative B) is still worth doing separately.
  2. The deserialize_with case 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 required asymmetry mirrors a real serde asymmetry. Deserialization is lenient (a missing option key becomes None regardless of annotations), while serialization is deterministic (None is emitted as an explicit null unless skip_serializing_if suppresses 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::required in reflectapi-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 }
  • TypeScript maps key == Optional to ? (nullability is already carried by the type mapping)
  • Python's resolve_field_optionality is now a thin mapping from the contract to = None defaults and | None hints
  • OpenAPI/Rust are untouched here; migrating them onto the same contract is the natural Architecture: move codegen backends toward SemanticSchema as the common backend contract #129 follow-up

For the issue's repro this produces exactly the expected shape:

export interface ARequest {
  reflect_option?: string | null | undefined;
  annotated_reflect_option?: string | null | undefined;
  std_option?: string | null;
  annotated_std_option?: string | null;
  custom_deserializer_option: string | null;  // deserialize_with — stays required
}

Custom-deserializer carve-out (from Codex's review, verified real): an option field with #[serde(with/deserialize_with)] and no default rejects a missing key — missing_field cannot 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_with and deserialize_with booleans named literally after the serde attributes whose presence they record (additive, mirroring how hidden was added; recorded identically on input/output reflections so type consolidation is unaffected). The shared contract keys the carve-out on deserialize_with alone — a custom serializer never affects whether a key may be absent, so an option field with only serialize_with still renders optional. Because the rule now lives in one place, this also fixes the pre-existing Python gap: option fields with deserialize_with were 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 no skip_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)] on reflectapi::Option fields (@avkonst's suggestion in the thread). Orthogonal and still worth doing — without default, a missing key deserializes to None rather than Undefined, 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 the std::option::Option ergonomics 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_type survive consolidation), and with the shared resolver it's now a one-place change:

    Type appears in (A) uniform (C-lite) Difference
    input typespace only field?: T | null field?: T | null none
    output typespace only field?: T | null field: T | null C-lite is more precise for consumers
    both (consolidated) field?: T | null field?: T | null none — one interface can't say "optional when writing, present when reading"

    The 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 | null to field?: T | null across 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 via NullToEmptyObject rather than Partial, in both TS and Python.

Tests & validation

  • New sample test_struct_with_option_fields_without_serde_default with the issue's exact repro plus a deserialize_with field (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.
  • Unit tests on resolve_field_wire_contract covering the truth table (plain/option/reflectapi-option/custom serializer/custom deserializer × required).
  • Empirical serde check (collapsed block above) run against the published reflectapi = "0.17.6", including the custom-deserializer failure mode.
  • Consumer-level tsc --strict validation: the pre-fix interface rejects {} (verified via @ts-expect-error), the post-fix interface accepts {} and explicit values.
  • 13 existing TS snapshots updated — every change is the same field: T | nullfield?: T | null addition. 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 with CI=1 (typechecking enabled).
  • The Rust dependency stub used for snapshot typechecking gains the reflectapi::Option enum — the new sample is the first to put a reflectapi::Option field through the Rust typecheck pass.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 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".

Comment thread reflectapi/src/codegen/typescript.rs Outdated
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,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge 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 👍 / 👎.

@github-actions

github-actions Bot commented Jun 11, 2026

Copy link
Copy Markdown

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

Updated automatically from commit 0bb866b

@andyyu2004

Copy link
Copy Markdown
Collaborator

I'm fine with this, I don't recall the former discussion :) I know serde special cases std::option::Option so that makes sense. I'm less clear on the behaviour with reflectapi::Option. I just did a quick experiment and without serde(default) on reflectapi::Option the behavior changes (you get None without and Undefined with).

I don't know why it works without the default, I think that needs some investigation and that's a bit of a footgun.

#[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.
@hardbyte hardbyte force-pushed the fix/typescript-option-fields-optional branch from f729714 to 8698687 Compare June 12, 2026 00:00
hardbyte added 4 commits June 12, 2026 14:48
…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.
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