Skip to content

Conversation

JMLX42
Copy link
Contributor

@JMLX42 JMLX42 commented Apr 25, 2025

Implements #1378

Some of the code in this MR was generated with Claude Code and the generated code reviewed by me:

To Do

  • Fix refs
    • implement a relevant test case
    • trigger the failing test case in the CI
    • implement a fix
  • Fix inline
    • implement a relevant test case
    • trigger the failing test case in the CI
    • implement a fix

@JMLX42 JMLX42 force-pushed the fix/non-nullable-option-oneof branch from 42faa80 to 8d9dfa6 Compare April 25, 2025 06:27
@JMLX42
Copy link
Contributor Author

JMLX42 commented Apr 25, 2025

Ah... 😅🫠

@JMLX42
Copy link
Contributor Author

JMLX42 commented Apr 25, 2025

As 2f2dbd3 demonstrates, the problem comes from Option<_> + nullable = false + default. I'll update the issue/title accordingly.

@JMLX42 JMLX42 changed the title test(utoipa-gen): test the schema generated for Option<_> + nullable … Option + nullable = false + default expands to a degenerate oneOf Apr 25, 2025
@JMLX42
Copy link
Contributor Author

JMLX42 commented Apr 25, 2025

Probable source of the problem:

let schema = if default.is_some() || nullable || title.is_some() {
composed_or_ref(quote_spanned! {type_path.span()=>
utoipa::openapi::schema::OneOfBuilder::new()
#nullable_item
.item(utoipa::openapi::schema::RefBuilder::new()
#description_stream
.ref_location_from_schema_name(#name_tokens)
)
#title_tokens
#default_tokens
})

@JMLX42
Copy link
Contributor Author

JMLX42 commented Apr 25, 2025

error[E0599]: no method named `default` found for struct `RefBuilder` in the current scope

And yet, I think in OAS 3.1, $ref + default is valid.

@JMLX42
Copy link
Contributor Author

JMLX42 commented Apr 25, 2025

OAS 3.0:

https://swagger.io/docs/specification/v3_0/using-ref/

Any sibling elements of a $ref are ignored. This is because $ref works by replacing itself and everything on its level with the definition it is pointing at.

OAS 3.1:

https://spec.openapis.org/oas/v3.1.0#schema-object

The Schema Object allows the definition of input and output data types. These types can be objects, but also primitives and arrays. This object is a superset of the JSON Schema Specification Draft 2020-12.

For more information about the properties, see JSON Schema Core and JSON Schema Validation.

Unless stated otherwise, the property definitions follow those of JSON Schema and do not add any additional semantics.

https://json-schema.org/draft/2020-12/json-schema-core#section-8.2.3.1

Note that this definition of how the results are determined means that other keywords can appear alongside of "$ref" in the same schema object.

And this StackOverflow answer:

https://stackoverflow.com/a/41752575

Your definition will work as expected when migrated to OpenAPI 3.1. This new version is fully compatible with JSON Schema 2020-12, which allows $ref siblings in schemas.

Thus, I think RefBuilder should accept siblings such as default or description.

@JMLX42 JMLX42 force-pushed the fix/non-nullable-option-oneof branch 2 times, most recently from f85b3a9 to 96a6cdf Compare April 25, 2025 12:58
@JMLX42
Copy link
Contributor Author

JMLX42 commented Apr 25, 2025

@juhaku ready for review!

Feedback welcome 🙏.

@JMLX42
Copy link
Contributor Author

JMLX42 commented Apr 27, 2025

@juhaku ready for review!

Actually I think my tests are incomplete. Please wait before merging.

@JMLX42
Copy link
Contributor Author

JMLX42 commented Apr 27, 2025

Actually I think my tests are incomplete. Please wait before merging.

There is still an issue with the inline branch:

let schema = if default.is_some()
|| nullable
|| title.is_some()
|| !description_tokens.is_empty()
{
quote_spanned! {type_path.span()=>
utoipa::openapi::schema::OneOfBuilder::new()
#nullable_item
.item(#items_tokens)
#title_tokens
#default_tokens
#description_stream
}
} else {
items_tokens
};

@JMLX42 JMLX42 force-pushed the fix/non-nullable-option-oneof branch from 96a6cdf to 14d036e Compare April 27, 2025 11:49
@JMLX42
Copy link
Contributor Author

JMLX42 commented Apr 27, 2025

I have tested the following:

                          let schema = if nullable {
                            quote_spanned! {type_path.span()=>
                                utoipa::openapi::schema::OneOfBuilder::new()
                                    #nullable_item
                                    .item(#items_tokens)
                                #title_tokens
                                #default_tokens
                                #description_stream
                            }
                        } else {
                            items_tokens
                        };

But then, default is missing from the generated schema. Which means it's missing from items_tokens.

Here is an example of generated code with inline = true + nullable = true:

object = object
    .property(
        "orthographic",
        utoipa::openapi::schema::OneOfBuilder::new()
            .item(
                utoipa::openapi::schema::ObjectBuilder::new()
                    .schema_type(utoipa::openapi::schema::Type::Null),
            )
            .item(
                <OrthographicCamera as utoipa::PartialSchema>::schema(),
            )
            .description(
                Some(
                    "An orthographic camera containing properties to create an orthographic\nprojection matrix. This property **MUST NOT** be defined when `perspective`\nis defined.",
                ),
            ),
    );

And with inline = true+nullable = false`:

object = object
    .property(
        "orthographic",
        <OrthographicCamera as utoipa::PartialSchema>::schema(),
    );

But PartialSchema::schema() returns RefOr<Schema>, which does not support the default property. Since we're inlining, it's probably a RefOr<Schema>::T(Schema).

@JMLX42
Copy link
Contributor Author

JMLX42 commented Apr 27, 2025

But PartialSchema::schema() returns RefOr, which does not support the default property. Since we're inlining, it's probably a RefOr::T(Schema).

When there is a title, a description or a default, the best option is probably to use a AllOf instead of a OneOf with a single item.

✔️ I have tested with Swagger UI, and it does merge the title, a description and default with the object.

✔️ I have successfully validated the OpenAPI schema.

✔️ OpenAPIGenerator also generates the expected code.

@JMLX42 JMLX42 force-pushed the fix/non-nullable-option-oneof branch 2 times, most recently from dd3133d to 03744c5 Compare April 27, 2025 12:52
@JMLX42 JMLX42 force-pushed the fix/non-nullable-option-oneof branch from 03744c5 to fe595a2 Compare April 27, 2025 13:01
@JMLX42
Copy link
Contributor Author

JMLX42 commented Apr 27, 2025

@juhaku I have asked ChatGPT (o4-mini-high) to generate the following summary for your review:


🐛 The Problem (Issue #1378)

When you have an Option<T> field with both

  • #[schema(nullable = false)]
  • and a #[schema(default = …)]

the generator was emitting a “degenerate” oneOf (i.e. a single‐member oneOf that just wraps your schema plus default) instead of a cleaner composition.


🔧 Key Changes

  1. Split out the non-nullable + default case in utoipa-gen/src/component.rs

    • Before: any combination of default.is_some() || nullable || title.is_some() went through a OneOfBuilder, even when nullable == false and you only wanted to attach a default.
    • After:
      • If nullable == true, still use OneOfBuilder (to allow null + the real type).
      • Otherwise, if you only have default/title/description, use an AllOfBuilder that merges:
        1. the item schema
        2. an inline object holding your { default: …, title: …, description: … }
  2. Support $ref siblings in utoipa/src/openapi/schema.rs

    • OAS 3.1 (and JSON Schema 2020-12) allow you to put default, title, etc. alongside a $ref.
    • This adds a default(Option<Value>) and title(Option<String>) field+builder method on RefBuilder, so non-inlined Option<T> + default (nullable=false) now emits:
      "optional_ref": {
        "$ref": "#/components/schemas/RefType",
        "default": { /**/ },
        "title": ""
      }
  3. New / updated tests & snapshots under utoipa-gen/tests/

    • derive_option_ref_with_nullable_false
    • derive_option_ref_with_nullable_false_and_default
    • derive_inline_option_ref_with_nullable_false_and_default
    • Plus adjustments to several existing snapshots to match the new allOf vs. degenerate oneOf behavior.

👀 What to Look For

  • component.rs: around lines 1287–1310 and 1358–1375, verify the branching logic for nullable vs. default/title.
  • schema.rs: addition of the default and title fields and their builder methods on RefBuilder.
  • Tests: that the JSON snapshots for the three new cases reflect the intended allOf or $ref+siblings, not a one-member oneOf.

✅ Outcome

  • Non-nullable Option<T> + default/title/description now produces a clean allOf (or $ref with siblings), not a useless oneOf.
  • Inline (#[schema(inline)]) fields still work correctly, merging defaults via allOf around the inlined schema.

Let me know if anything looks off or needs further tweaking!

@JMLX42
Copy link
Contributor Author

JMLX42 commented May 4, 2025

FYI this fix has interesting side effects with the fresh 7.13.0 release of OpenAPI Generator that includes this PR: OpenAPITools/openapi-generator#21043

One might argue that this fix might not be needed since oneOf with a single variant are now likely to get inlined anyway. But I would answer that as far as the protocol definition is concerned:

@JMLX42
Copy link
Contributor Author

JMLX42 commented Jun 19, 2025

@juhaku anything I can do to help moving this forward?

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.

1 participant