juhaku / utoipa

Simple, Fast, Code first and Compile time generated OpenAPI documentation for Rust
Apache License 2.0
2.46k stars 191 forks source link

Overly complicated discriminator #621

Closed jayvdb closed 1 month ago

jayvdb commented 1 year ago

We have a Rust struct, where all of the variants have a member report_type.

    #[derive(..., utoipa::ToSchema)]
    #[serde(tag = "report_type")]
    pub enum ReportData {
        Report1(Report1),
        Report2(Report2),
        Report3(Report3),
        ...
    }

utoipa emits

    ReportData:
      oneOf:
        - allOf:
            - $ref: "#/components/schemas/Report1"
            - type: object
              required:
                - report_type
              properties:
                report_type:
                  type: string
                  enum:
                    - Report1
        - allOf:
            - $ref: "#/components/schemas/Report2"
            - type: object
              required:
                - report_type
              properties:
                report_type:
                  type: string
                  enum:
                    - Report2
        - allOf:
            - $ref: "#/components/schemas/Report3"
            - type: object
              required:
                - report_type
              properties:
                report_type:
                  type: string
                  enum:
                    - Report3
      ...
      discriminator:
        propertyName: report_type

As the report_type fields in each schema, it should be

    ReportData:
      oneOf:
        - $ref: "#/components/schemas/Report1"
        - $ref: "#/components/schemas/Report2"
        - $ref: "#/components/schemas/Report3"
      discriminator:
        propertyName: report_type
juhaku commented 1 month ago

@jayvdb Does the report_type come from the Report1 Report2 and Report3. I am trying to figure out how the discriminator should actually work with the schemas and I cannot quite figure it out. As the serde(tag = "report_type") will tag the enum variants with report_type and put each variant name as it's value. However the actual report_type does not exist in the Report1, Report2 and Report3 as a field. Moreover since the field is not in the type itself then the generated schema will be wrong.

#[derive(..., utoipa::ToSchema)]
#[serde(tag = "report_type")]
pub enum ReportData {
   Report1(Report1),
   Report2(Report2),
   Report3(Report3),
   ...
}

E.g The above code should accept JSON as {report_type: "Report1", ...} but the generated spec JSON only show fields from within the Report1 without the report_type field that was added by the serde(tag = ...).

So instead to get the correct schema. The serde(untagged) should be used instead and then each report should have an embedded field for the report_type. Then the discriminator should be defined with some other means e.g. schema(discriminator = "report_type")

#[derive(..., utoipa::ToSchema)]
#[serde(untagged)]
#[schema(discriminator = "report_type")] // <-- see here
pub enum ReportData {
    Report1(Report1),
    Report2(Report2),
    Report3(Report3),
    ...
}
jayvdb commented 1 month ago

Sorry @juhaku , now I look at this again, the way I described this issue is incorrect.

Does the report_type come from the Report1 Report2 and Report3?

no, they do not contain the field report_type.

In the raw data, a ReportData consists of a report_type and the fields inside one of Report1 Report2 and Report3

i.e.

report_data:
  report_type: "Report1"
  report1_field1: "foo"
  report1_field1: "bar"

And the way that utoipa is describing that does look like the optimal openapi.

I am going to try your suggestion of using #[serde(untagged)] and will report back soon.

juhaku commented 1 month ago

Great, however since there is no custom attribute for discriminator as of now, you need to plug it in manually

jayvdb commented 1 month ago

Our Rust is generated is generated from OpenAPI using https://github.com/oxidecomputer/progenitor to do the majority of the work, and then lots of hacks around it, so it is a bit more complex to do as you suggested, and this issue is actually about one of those hacks, so it is a bit of work to change it to #[serde(untagged)] , but it feels like switching to untagged will mean I can remove a few hacks.

juhaku commented 1 month ago

Right, with that, less hacks the better :smile: I am planning on removing the whole #[serde(tag = ...)] as discriminator since it anyway results incorrect schema. That said with the #[serde(untagged)] the disciminator field need be manually set to each variant type and the #[schema(discriminator = ...)] only would point to that property name which works as the discriminator for all the reference variants that should be common to all variants.

jayvdb commented 1 month ago

Ok, I can generate untagged, and it everything works well except I loose the discriminator in the utoipa output.

#[derive(..., utoipa::ToSchema)]
#[serde(untagged)]
//#[schema(discriminator = "report_type")] // <-- see here
pub enum ReportData {
    Report1(Report1),
    Report2(Report2),
    Report3(Report3),
    ...
}

utoipa output

    ReportReportData:
      oneOf:
        - $ref: "#/components/schemas/Report1"
        - $ref: "#/components/schemas/Report2"
        - $ref: "#/components/schemas/Report3"

That is with the latest release, and I cant build my project with utoipa master atm due to some other problem I havent understood yet - probably my problem, not a problem with utoipa. However it looks like utoipa master doesnt yet have #[schema(discriminator = .. )] support yet.

jayvdb commented 1 month ago

Oh, I think I understand now what you were saying earlier: in the current release, I need to manually implement ToSchema on ReportData in order to add the discriminator.

juhaku commented 1 month ago

Yeah, sorry for being bit unclear. I am going to work on this as I am refactoring the enum processing anyways.