Skip to content

feat: Add support for reflectapi(required) which will force an input to be required in the generated clients, even if it is optional/has a default. This allows for introduction of new required properties without breaking existing deployed clients.#121

Open
JosiahBull wants to merge 1 commit intomainfrom
jo/exploratory-add-support-for-forcing-optional-field-to-be-required

Conversation

@JosiahBull
Copy link
Copy Markdown
Contributor

No description provided.

…t to be required in the generated clients, even if it is optional/has a default. This allows for introduction of new required properties without breaking existing deployed clients.
@JosiahBull JosiahBull requested a review from andyyu2004 March 23, 2026 04:11
@JosiahBull JosiahBull marked this pull request as ready for review March 23, 2026 04:11
@andyyu2004
Copy link
Copy Markdown
Contributor

I don't really understand this. What's wrong with not specifying a default?
When does it make sense to have default & required?

Copy link
Copy Markdown

@claude claude bot left a comment

Choose a reason for hiding this comment

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

LGTM — clean, well-tested addition following existing patterns.

Extended reasoning...

Overview

This PR adds a #[reflectapi(required)] field attribute to the derive macro. Changes touch three small files in reflectapi-derive (new REQUIRED symbol, parsing support, and one-line logic change in derive.rs) plus a new test with five snapshot files covering the schema, TypeScript, Rust, OpenAPI, and Python outputs.

Security risks

None. This is a compile-time proc-macro attribute with no runtime security surface.

Level of scrutiny

Low. The implementation is a minimal, additive change — one new boolean field in ParsedFieldAttributes, one new symbol constant, one else if branch in the parser, and a single || added to the requiredness logic for inputs. The change is isolated to input field handling and does not affect output behavior.

Other factors

The snapshots fully validate the end-to-end behavior across all code-generation targets. The logic attrs.required || field.attrs.default().is_none() correctly preserves existing behavior (fields without a serde default are still required) while allowing opt-in forcing of requiredness. No bugs were found by the automated system, and there are no outstanding reviewer comments.

@JosiahBull
Copy link
Copy Markdown
Contributor Author

JosiahBull commented Mar 23, 2026

So I want to add a new field onto a request, but there are existing clients out in the world.

To be used for adding a new field to a request, sort of like Postgres NOT VALID constraint. Best for endpoints with out of band consumers, e.g. tier1.vehicles.search - our primary users are in the monorepo and will need the new field, but there are retools/ad-hoc scripts in use by half the company that need backwards compatibility.

Would also be useful for users who haven't refreshed their browsers in a while.

@andyyu2004
Copy link
Copy Markdown
Contributor

andyyu2004 commented Mar 23, 2026

I understand the use case now, thanks. It feels a little strange, I'll sit on it a bit. The patch is quite simple though which is good.

I'm almost leaning towards something like reflectapi(force_required) to make its purpose clearer.

@avkonst
Copy link
Copy Markdown
Collaborator

avkonst commented Mar 23, 2026

I agree with Andy here.

@andyyu2004
Copy link
Copy Markdown
Contributor

As in you're fine with force_required?

@avkonst
Copy link
Copy Markdown
Collaborator

avkonst commented Mar 23, 2026

I agree with you that the change almost like not very strongly justified (use case is quite weak, imo).
I also agree that if we accept it I agree with your proposal for force_required name.
But I would rethink accepting it, as I just realized we already have support for it with the type transformer attribute. @JosiahBull have you tried adding the transformer attribute to the field which drops Option in the spec? This should do the job, you can even name it like the following:

#[derive(reflectapi::Input, reflectapi::Output, serde::Deserialize, serde::Serialize)]
struct TestStructWithTransformFallback {
    #[reflectapi(
        input_transform = "reflectapi-ex::force_required",
        output_transform = "reflectapi-ex::force_required"
    )]
    _f: Option<u8>,
}
#[test]
fn test_reflectapi_struct_with_transform_fallback() {
    assert_snapshot!(TestStructWithTransformFallback);
}

@JosiahBull
Copy link
Copy Markdown
Contributor Author

I agree with you that the change almost like not very strongly justified (use case is quite weak, imo).

For the quality ranks, how can we add a new required field to an existing API, while meeting this criteria:

Persisted state backward & forward compatibility (eg. with help of downsizer) assurance

For level 3/4 without this?

The only method I see is creating an entirely new API version, which takes months for consumers to migrate. With the help of the type system we could add a new required field without breaking existing deployed clients, but still force build errors in the monorepo via the type system - encouraging rapid adoption.

I am not familiar with input_transform and output_transform, seems like it will do the job if it allows editing of the schema directly. Similarly could do with a wrapper type and custom deser. implementation.

@JosiahBull
Copy link
Copy Markdown
Contributor Author

The only path being a new version might be intentional, but I think it's a very high effort option for sometimes very small additions which we could accommodate in a backwards compatible fashion.

@avkonst
Copy link
Copy Markdown
Collaborator

avkonst commented Mar 24, 2026

Jo, use the transformers. It will help you to achieve exactly what you need. I am happy to see our reflectapi-ex crate helper force_required implemented. It is quite simple to do.

In regards to the justification for the usecase. No need to convince me :) I got it you need it, and so reflectapi with help of transformers let's you do so cleanly.

In regards to forward and backward compatibility. It would be typically achieved via these steps:
1 new optional field is added (or we assume existing optional field is already in place), and let's say we want to turn it to the required
2 all of the clients are changed to submit the field to the backend (deployed and rolled out)
3 the backend is changed to have the field required rather than optional
4. clients migrate to the new generated api
And yes, I agree with the force_required transformer would make it easier to do the step 2 with better guarantees.

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.

3 participants