Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The generated openapi scheme for union has changed. Is it possible to restore the previous schema? #263

Closed
szagi3891 opened this issue Apr 20, 2022 · 35 comments
Labels
question Further information is requested Stale

Comments

@szagi3891
Copy link

Previously the union scheme was generated like this:

{
    "type": "object",
    "properties": {
        "type": {
            "type": "string",
            "enum": [
                "CreateSessionResponseOk",
                "CreateSessionResponseErrors",
                "CreateSessionResponseErrorAccess"
            ]
        }
    },
    "oneOf": [{
        "$ref": "#/components/schemas/CreateSessionResponseOk"
    }, {
        "$ref": "#/components/schemas/CreateSessionResponseErrors"
    }, {
        "$ref": "#/components/schemas/CreateSessionResponseErrorAccess"
    }],
    "discriminator": {
        "propertyName": "type"
    }
}

After recently updating the poem in the project to the latest version, I noticed that it generates like this:


{
    "type": "object",
    "anyOf": [
        {
            "required": [
                "type"
            ],
            "allOf": [{
                "$ref": "#/components/schemas/CreateSessionResponseOk"
            }, {
                "type": "object",
                "title": "CreateSessionResponseOk",
                "properties": {
                    "type": {
                        "type": "string",
                        "example": "CreateSessionResponseOk"
                    }
                }
            }]
        }, {
            "required": [
                "type"
            ],
            "allOf": [{
                "$ref": "#/components/schemas/CreateSessionResponseErrors"
            }, {
                "type": "object",
                "title": "CreateSessionResponseErrors",
                "properties": {
                    "type": {
                        "type": "string",
                        "example": "CreateSessionResponseErrors"
                    }
                }
            }]
        }, {
            "required": [
            "type"
            ],
            "allOf": [{
                "$ref": "#/components/schemas/CreateSessionResponseErrorAccess"
            }, {
                "type": "object",
                "title": "CreateSessionResponseErrorAccess",
                "properties": {
                    "type": {
                        "type": "string",
                        "example": "CreateSessionResponseErrorAccess"
                    }
                }
            }]
        }
    ],
    "discriminator": {
        "propertyName": "type"
    }
}

Is it possible to restore the previous specification format ? The previous version was much better

@szagi3891 szagi3891 added the question Further information is requested label Apr 20, 2022
@sunli829
Copy link
Collaborator

The default is anyOf, you can use the oneOf attribute to specify that it generates the oneOf type.

#[oai(one_of, inline)]

@szagi3891
Copy link
Author

This has not changed anything.


#[derive(Debug, Object, Serialize, Deserialize)]
#[oai(inline)] 
pub struct CreateSessionResponseOk {
    #[serde(rename="accountId")]
    #[oai(rename="accountId")]
    pub account_id: u64,
    #[serde(rename="access_token")]
    #[oai(rename="access_token")]
    pub access_token: String,
}


#[derive(Debug, Object, Serialize, Deserialize)]
#[oai(inline)] 
pub struct GenericCreateSessionResponseOk {
    #[serde(rename="accountId")]
    #[oai(rename="accountId")]
    pub account_id: Option<u64>,
    #[serde(rename="access_token")]
    #[oai(rename="access_token")]
    pub access_token: String,
}

#[derive(Debug, Object, Serialize, Deserialize)]
#[oai(inline)] 
pub struct CreateSessionResponseErrors {
    pub error: String,
    // #[oai(rename="error_description")]
    pub error_description: String,
    // #[oai(rename="accountId")]
    pub account_id: Option<u64>,
}

#[derive(Debug, Object, Serialize, Deserialize)]
#[oai(inline)] 
pub struct CreateSessionResponseErrorAccess {
    pub code: String,
    pub message: String,
}

#[derive(Debug, Union, Serialize, Deserialize)]
#[oai(discriminator_name="type")]
#[oai(one_of, inline)] 
pub enum GenericCreateSessionResponse {
    Ok(GenericCreateSessionResponseOk),
    Errors(CreateSessionResponseErrors),
    ErrorAccess(CreateSessionResponseErrorAccess)
}

This specification is still being generated:

      "CreateSessionResponse": {
        "type": "object",
        "anyOf": [
          {
            "required": [
              "type"
            ],
            "allOf": [
              {
                "type": "object",
                "required": [
                  "accountId",
                  "access_token"
                ],
                "properties": {
                  "accountId": {
                    "type": "integer",
                    "format": "uint64"
                  },
                  "access_token": {
                    "type": "string"
                  }
                }
              },
              {
                "type": "object",
                "title": "CreateSessionResponseOk",
                "properties": {
                  "type": {
                    "type": "string",
                    "example": "CreateSessionResponseOk"
                  }
                }
              }
            ]
          },
          {
            "required": [
              "type"
            ],
            "allOf": [
              {
                "type": "object",
                "required": [
                  "error",
                  "error_description"
                ],
                "properties": {
                  "error": {
                    "type": "string"
                  },
                  "error_description": {
                    "type": "string"
                  },
                  "account_id": {
                    "type": "integer",
                    "format": "uint64"
                  }
                }
              },
              {
                "type": "object",
                "title": "CreateSessionResponseErrors",
                "properties": {
                  "type": {
                    "type": "string",
                    "example": "CreateSessionResponseErrors"
                  }
                }
              }
            ]
          },
          {
            "required": [
              "type"
            ],
            "allOf": [
              {
                "type": "object",
                "required": [
                  "code",
                  "message"
                ],
                "properties": {
                  "code": {
                    "type": "string"
                  },
                  "message": {
                    "type": "string"
                  }
                }
              },
              {
                "type": "object",
                "title": "CreateSessionResponseErrorAccess",
                "properties": {
                  "type": {
                    "type": "string",
                    "example": "CreateSessionResponseErrorAccess"
                  }
                }
              }
            ]
          }
        ],
        "discriminator": {
          "propertyName": "type"
        }
      },

@szagi3891
Copy link
Author

szagi3891 commented Apr 21, 2022

@sunli829
I found this piece of documentation
https://swagger.io/docs/specification/data-models/inheritance-and-polymorphism/

Maybe it is better to generate the enum specification this in way ?
There is much less code and the discriminator is explicitly specified:

{
    "type": "object",
    "anyOf": [
        {
            "$ref": "#/components/schemas/CreateSessionResponseOk",
        }, {
            "$ref": "#/components/schemas/CreateSessionResponseErrors"
        }, {
            "$ref": "#/components/schemas/CreateSessionResponseErrorAccess"
        }
    ],
    "discriminator": {
        "propertyName": "type",
        "mapping":
            "CreateSessionResponseOk": "#/components/schemas/CreateSessionResponseOk",
            "CreateSessionResponseErrors": "#/components/schemas/CreateSessionResponseErrors",
            "CreateSessionResponseErrorAccess": "#/components/schemas/CreateSessionResponseErrorAccess"
    }
}

@sunli829
Copy link
Collaborator

Since each object in the union doesn't actually have a type field, I added it for them with allOf, otherwise it would be weird not to see it in the swagger ui.

@szagi3891
Copy link
Author

This is a swaggerUI error.

I understand that you have good intentions. But the result is a strange contract.
It is better to stick to the specifications.

@szagi3891
Copy link
Author

can you expand on how discriminator is defined ?

If you add explicit mapping to structures, that's already cool for me.

    "discriminator": {
        "propertyName": "type",
        "mapping":
            "CreateSessionResponseOk": "#/components/schemas/CreateSessionResponseOk",
            "CreateSessionResponseErrors": "#/components/schemas/CreateSessionResponseErrors",
            "CreateSessionResponseErrorAccess": "#/components/schemas/CreateSessionResponseErrorAccess"
    }

@sunli829
Copy link
Collaborator

OK, I'll update later. 😁

@sunli829
Copy link
Collaborator

Swagger ui doesn't show type field, can you help me?

https://github.com/poem-web/poem/tree/rework-openapi-union

cd examples
cargo run --bin example-openapi-union

@szagi3891
Copy link
Author

I would like to advise you but I am having trouble getting this new version to work:

In the test repository I switched to this new branch:
https://github.com/szagi3891/poem-test
https://github.com/szagi3891/poem-test/commit/4df7b618acba36461eb2d5e3f96b65ccbd4cacae

    Finished dev [unoptimized + debuginfo] target(s) in 16.41s
     Running `target/debug/example-openapi-oneof`
thread 'main' panicked at 'explicit panic', /Users/grzegorzszeliga/.cargo/git/checkouts/poem-5d35782906f249bc/09ed17c/poem-openapi/src/registry/mod.rs:323:41
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

@szagi3891
Copy link
Author

RUST_BACKTRACE=1 cargo run 

....

    Finished dev [unoptimized + debuginfo] target(s) in 0.17s
     Running `target/debug/example-openapi-oneof`
thread 'main' panicked at 'explicit panic', /Users/grzegorzszeliga/.cargo/git/checkouts/poem-5d35782906f249bc/09ed17c/poem-openapi/src/registry/mod.rs:323:41
stack backtrace:
   0: rust_begin_unwind
             at /rustc/306ba8357fb36212b7d30efb9eb9e41659ac1445/library/std/src/panicking.rs:584:5
   1: core::panicking::panic_fmt
             at /rustc/306ba8357fb36212b7d30efb9eb9e41659ac1445/library/core/src/panicking.rs:143:14
   2: core::panicking::panic
             at /rustc/306ba8357fb36212b7d30efb9eb9e41659ac1445/library/core/src/panicking.rs:48:5
   3: poem_openapi::registry::MetaSchemaRef::as_reference
             at /Users/grzegorzszeliga/.cargo/git/checkouts/poem-5d35782906f249bc/09ed17c/poem-openapi/src/registry/mod.rs:323:41
   4: <example_openapi_oneof::ProxyResponse<R> as poem_openapi::types::Type>::schema_ref
             at ./src/main.rs:286:17
   5: <poem_openapi::payload::json::Json<T> as poem_openapi::payload::Payload>::schema_ref
             at /Users/grzegorzszeliga/.cargo/git/checkouts/poem-5d35782906f249bc/09ed17c/poem-openapi/src/payload/json.rs:36:9
   6: <poem_openapi::payload::json::Json<T> as poem_openapi::base::ApiResponse>::meta
             at /Users/grzegorzszeliga/.cargo/git/checkouts/poem-5d35782906f249bc/09ed17c/poem-openapi/src/payload/json.rs:80:29
   7: <example_openapi_oneof::Api2 as poem_openapi::base::OpenApi>::meta
             at ./src/main.rs:123:1
   8: poem_openapi::openapi::OpenApiService<T,W>::document
             at /Users/grzegorzszeliga/.cargo/git/checkouts/poem-5d35782906f249bc/09ed17c/poem-openapi/src/openapi.rs:370:24
   9: poem_openapi::openapi::OpenApiService<T,W>::spec
             at /Users/grzegorzszeliga/.cargo/git/checkouts/poem-5d35782906f249bc/09ed17c/poem-openapi/src/openapi.rs:442:19
  10: poem_openapi::openapi::OpenApiService<T,W>::spec_endpoint
             at /Users/grzegorzszeliga/.cargo/git/checkouts/poem-5d35782906f249bc/09ed17c/poem-openapi/src/openapi.rs:356:20
  11: example_openapi_oneof::main::{{closure}}
             at ./src/main.rs:312:17
  12: <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll
             at /rustc/306ba8357fb36212b7d30efb9eb9e41659ac1445/library/core/src/future/mod.rs:91:19
  13: tokio::park::thread::CachedParkThread::block_on::{{closure}}
             at /Users/grzegorzszeliga/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.17.0/src/park/thread.rs:263:54
  14: tokio::coop::with_budget::{{closure}}
             at /Users/grzegorzszeliga/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.17.0/src/coop.rs:102:9
  15: std::thread::local::LocalKey<T>::try_with
             at /rustc/306ba8357fb36212b7d30efb9eb9e41659ac1445/library/std/src/thread/local.rs:442:16
  16: std::thread::local::LocalKey<T>::with
             at /rustc/306ba8357fb36212b7d30efb9eb9e41659ac1445/library/std/src/thread/local.rs:418:9
  17: tokio::coop::with_budget
             at /Users/grzegorzszeliga/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.17.0/src/coop.rs:95:5
  18: tokio::coop::budget
             at /Users/grzegorzszeliga/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.17.0/src/coop.rs:72:5
  19: tokio::park::thread::CachedParkThread::block_on
             at /Users/grzegorzszeliga/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.17.0/src/park/thread.rs:263:31
  20: tokio::runtime::enter::Enter::block_on
             at /Users/grzegorzszeliga/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.17.0/src/runtime/enter.rs:151:13
  21: tokio::runtime::thread_pool::ThreadPool::block_on
             at /Users/grzegorzszeliga/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.17.0/src/runtime/thread_pool/mod.rs:73:9
  22: tokio::runtime::Runtime::block_on
             at /Users/grzegorzszeliga/.cargo/registry/src/github.com-1ecc6299db9ec823/tokio-1.17.0/src/runtime/mod.rs:477:43
  23: example_openapi_oneof::main
             at ./src/main.rs:315:5
  24: core::ops::function::FnOnce::call_once
             at /rustc/306ba8357fb36212b7d30efb9eb9e41659ac1445/library/core/src/ops/function.rs:230:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

@sunli829
Copy link
Collaborator

Sorry, I fixed this now. 😂

@szagi3891
Copy link
Author

Yes, it is much better now :)

Only now there is another problem. The "discriminator" is missing one of the types:

    "/api2/fun3": {
      "get": {
        "responses": {
          "200": {
            "description": "",
            "content": {
              "application/json": {
                "schema": {
                  "type": "object",
                  "anyOf": [
                    {
                      "type": "object",
                      "required": [
                        "response"
                      ],
                      "properties": {
                        "response": {
                          "type": "string"
                        }
                      }
                    },
                    {
                      "$ref": "#/components/schemas/ProxyResponseMessage"
                    },
                    {
                      "$ref": "#/components/schemas/ProxyResponseUnknown"
                    }
                  ],
                  "discriminator": {
                    "propertyName": "type",
                    "mapping": {
                      "ProxyResponseMessage": "#/components/schemas/ProxyResponseMessage",
                      "ProxyResponseUnknown": "#/components/schemas/ProxyResponseUnknown"
                    }
                  }
                }
              }
            }
          }
        }
      }
    },

@szagi3891
Copy link
Author

I suggest that It is better to generate a model for each of the generics.
What do you think about it ?

    "/api2/fun3": {
      "get": {
        "responses": {
          "200": {
            "description": "",
            "content": {
              "application/json": {
                "schema": {
                  "type": "object",
                  "anyOf": [
                    {
                      "$ref": "#/components/schemas/ProxyResponseOk<string>"
                    },
                    {
                      "$ref": "#/components/schemas/ProxyResponseMessage"
                    },
                    {
                      "$ref": "#/components/schemas/ProxyResponseUnknown"
                    }
                  ],
                  "discriminator": {
                    "propertyName": "type",
                    "mapping": {
                      "ProxyResponseOk<string>": "#/components/schemas/ProxyResponseOk<string>",
                      "ProxyResponseMessage": "#/components/schemas/ProxyResponseMessage",
                      "ProxyResponseUnknown": "#/components/schemas/ProxyResponseUnknown"
                    }
                  }
                }
              }
            }
          }
        }
      }
    },

...



"schemas": {

    ...
        
    "ProxyResponseOk<string>": {
        "type": "object",
        "required": [
            "response"
        ],
        "properties": {
            "response": {
                "type": "string"
            }
        }
    },
    "ProxyResponseMessage": {
        "type": "object",
        "required": [
            "code",
            "message"
        ],
        "properties": {
            "code": {
                "type": "string"
            },
            "message": {
                "type": "string"
            }
        }
    },

    ...

}

...

@sunli829
Copy link
Collaborator

Thanks for the suggestion, I will try it tomorrow. 🙂

@szagi3891
Copy link
Author

@sunli829 Do you want the "type" field to necessarily be shown by swaggerUI ?
I got information from a colleague that this openapi visualisation tool https://redocly.com/ correctly shows descriminators.

@szagi3891
Copy link
Author

@sunli829 Hello. Will you be publishing these changes ?

Even what is now on the branch unfinished works well. I use these changes in production :)

@sunli829
Copy link
Collaborator

In v2.0.0 it is no longer necessary to use concrete attribute to assign a name to each concrete type, so convenient! 😀

@szagi3891
Copy link
Author

Cool. I missed the moment that you released version 2.0.0-alpha.1 :) I will try this version in my project

@szagi3891
Copy link
Author

@sunli829 - Hello. Would it be possible for you to add this mapping fix ?

@sunli829
Copy link
Collaborator

Sorry for the late reply, in fact I still don't know how to fix it. @szagi3891

@szagi3891
Copy link
Author

The changes you made in the branch rework-openapi-union fix my problem.
https://github.com/poem-web/poem/commits/rework-openapi-union

maybe you will use this solution method ?

sunli829 added a commit that referenced this issue Jul 12, 2022
Upgrade swagger_ui/redoc/rapidoc
@sunli829
Copy link
Collaborator

Fixed in master, could you please help me to test it? 🙂

@szagi3891
Copy link
Author

Sure, I'll be happy to help :)

      "CreateSessionResponse": {
        "type": "object",
        "anyOf": [
          {
            "$ref": "#/components/schemas/CreateSessionResponse[CreateSessionResponseOk]"
          },
          {
            "$ref": "#/components/schemas/CreateSessionResponse[CreateSessionResponseErrors]"
          },
          {
            "$ref": "#/components/schemas/CreateSessionResponse[CreateSessionResponseErrorAccess]"
          }
        ],
        "discriminator": {
          "propertyName": "type",
          "mapping": {
            "Ok": "#/components/schemas/CreateSessionResponse[CreateSessionResponseOk]",
            "Errors": "#/components/schemas/CreateSessionResponse[CreateSessionResponseErrors]",
            "ErrorAccess": "#/components/schemas/CreateSessionResponse[CreateSessionResponseErrorAccess]"
          }
        }
      },
      "CreateSessionResponse[CreateSessionResponseOk]": {
        "allOf": [
          {
            "type": "object",
            "required": [
              "type"
            ],
            "properties": {
              "type": {
                "type": "string",
                "example": "Ok"
              }
            }
          },
          {
            "$ref": "#/components/schemas/CreateSessionResponseOk"
          }
        ]
      },
      "CreateSessionResponseOk": {
        "type": "object",
        "required": [
          "accountId",
          "access_token"
        ],
        "properties": {
          "accountId": {
            "type": "integer",
            "format": "uint64"
          },
          "access_token": {
            "type": "string"
          }
        }
      },

"discriminator" and "mapping" looks very good.

Is it possible to remove this intermediate structure "CreateSessionResponse[CreateSessionResponseOk]" ?

@sunli829
Copy link
Collaborator

It cannot be deleted because the CreateSessionResponse[CreateSessionResponseOk] structure is required, which has an additional type attribute than CreateSessionResponseOk. 🙂

@szagi3891
Copy link
Author

Information about this additional type, is provided by this structure:

        "discriminator": {
          "propertyName": "type",
          "mapping": {
            "Ok": "#/components/schemas/CreateSessionResponse[CreateSessionResponseOk]",
            "Errors": "#/components/schemas/CreateSessionResponse[CreateSessionResponseErrors]",
            "ErrorAccess": "#/components/schemas/CreateSessionResponse[CreateSessionResponseErrorAccess]"
          }
        }

It seems that swagger ui has a bug and cannot correctly visualise the type specified in the discriminator.

redoc, on the other hand, is good at visualising the discriminator
https://redocly.com/docs/resources/discriminator/

@sunli829
Copy link
Collaborator

This should not be Swagger's bug, as the example provided in the spec, every object should contain the petType property.

components:
  schemas:
    Pet:
      type: object
      required:
      - petType
      properties:   # <<<----------------------- add the petType property
        petType:
          type: string
      discriminator:
        propertyName: petType
        mapping:
          dog: Dog
    Cat:
      allOf:
      - $ref: '#/components/schemas/Pet'
      - type: object
        # all other properties specific to a `Cat`
        properties:
          name:
            type: string
    Dog:
      allOf:
      - $ref: '#/components/schemas/Pet'
      - type: object
        # all other properties specific to a `Dog`
        properties:
          bark:
            type: string
    Lizard:
      allOf:
      - $ref: '#/components/schemas/Pet'
      - type: object
        # all other properties specific to a `Lizard`
        properties:
          lovesRocks:
            type: boolean

@szagi3891
Copy link
Author

You are right.

https://swagger.io/docs/specification/data-models/inheritance-and-polymorphism/

In our example, the discriminator points to the objectType property that contains the data type name. The discriminator is used with anyOf or oneOf keywords only. It is important that all the models mentioned below anyOf or oneOf contain the property that the discriminator specifies. This means, for example, that in our code above, both simpleObject and complexObject must have the objectType property. This property is required in these schemas:

It looks like your change is working very well :)

@szagi3891
Copy link
Author

Do you know this solution?
https://docs.rs/schemars/latest/schemars/

Perhaps it is worth applying to poem ?

@sunli829
Copy link
Collaborator

I know, I just want all the work to be done by myself, which is more controllable. 😂

@szagi3891
Copy link
Author

I understand :)

Do you support such enums ?
https://serde.rs/enum-representations.html#externally-tagged

@sunli829
Copy link
Collaborator

It is not supported yet, I will add it later. 🙂

@sunli829
Copy link
Collaborator

The OpenAPI specification doesn't support this, do you really need it?

@szagi3891
Copy link
Author

Not necessary. But from what I've heard, there's a better alternative to discriminator.

@github-actions
Copy link

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

@github-actions github-actions bot added the Stale label Aug 19, 2022
@github-actions
Copy link

This issue was closed because it has been stalled for 5 days with no activity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested Stale
Projects
None yet
Development

No branches or pull requests

2 participants