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

Adds fix for composed schema model vars #8816

Closed
wants to merge 4 commits into from

Conversation

spacether
Copy link
Contributor

@spacether spacether commented Feb 24, 2021

Adds fix for composed schema model vars
Per guidance from json schema and openapi people, when a composed schema has properties or additionalProperties
object payloads are validated to the properties or additionalProerties definition at the composed schema level, independent of properties in composed oneOf/anyOf/allOf definition

So that means that CodegenModel.vars must store only the properties present in the composed schema, if they are present.
This has implications to how we validate composed schemas across generators, and that all generators should eventually support supportsAdditionalPropertiesWithComposedSchema = True.
For now only python and java support it.

This is a fix for: #8813

Blocker

Landing this PR is blocked by: #8802
because we need additionalProperties to be set at the model level before we can use it to allow any value in in all key value pairs.

@sebastien-rosset should the go generator set supportsAdditionalPropertiesWithComposedSchema to True?

PR checklist

  • Read the contribution guidelines.
  • Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
  • Run the following to build the project and update samples:
    ./mvnw clean package 
    ./bin/generate-samples.sh
    ./bin/utils/export_docs_generators.sh
    
    Commit all changed files.
    This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
    These must match the expectations made by your contribution.
    You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*.
    For Windows users, please run the script in Git BASH.
  • File the PR against the correct branch: master, 5.1.x, 6.0.x
  • If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

@spacether spacether linked an issue Feb 24, 2021 that may be closed by this pull request
6 tasks
@spacether spacether marked this pull request as draft February 24, 2021 02:09
@wing328
Copy link
Member

wing328 commented Mar 2, 2021

Per guidance from json schema and openapi people, when a composed schema has properties or additionalProperties

Can you please share the links to the discussions?

@spacether
Copy link
Contributor Author

Per guidance from json schema and openapi people, when a composed schema has properties or additionalProperties

Can you please share the links to the discussions?

Sure, here it is: OAI/OpenAPI-Specification#2478

public void setDeclawed(Boolean declawed) {
this.declawed = declawed;
}

Copy link
Member

Choose a reason for hiding this comment

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

@spacether the declawed property has been removed as part of the change, which doesn't seem to be correct according to the definition of Cat:

    Cat:
      allOf:
        - $ref: '#/components/schemas/Animal'
        - $ref: '#/components/schemas/Address'
        - type: object
          properties:
            declawed:
              type: boolean

ref: https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator/src/test/resources/3_0/java/petstore-with-fake-endpoints-models-for-testing-with-http-signature.yaml#L1411-L1418

Copy link
Contributor Author

@spacether spacether Mar 10, 2021

Choose a reason for hiding this comment

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

The CodegenModel Cat not containing declawed when vars only contains properties in the current schema is correct. declawed definition only exists in that allOf third schema. It does not have an explicit definition inside Cat because cat has by default:

Cat:
  properties: {} # no declawed property here
  additionalProperties: true # this allows ingestion of any type of value

Cat containing allOf properties in generated code is an implementation decision.
Per our chat, an allOf schema could be composed also, which means that the variables that it includes in a composed schema like Cat can change from one payload to another or could event be different types like null or object.
This is why I think putting this behind a CLI option may be the best path forward.
Fixing Java looks non-trivial to me but maybe I am missing something.

Do you have a straightforward way to fix Java in this PR?
How do you think I should proceed here?

Copy link
Member

Choose a reason for hiding this comment

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

The CodegenModel Cat not containing declawed when vars only contains properties in the current schema is correct

Can you share the link(s) to discussion supporting this claim so that everyone is on the same page?

Copy link
Contributor Author

@spacether spacether Mar 11, 2021

Choose a reason for hiding this comment

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

The point of this PR is that vars should only contain the properties defined in properties of the current scheme. It should not include composed schema properties. This is consistent with how the openapi folks told me to interpret schemas.

Yup it is in the link I pasted above OAI/OpenAPI-Specification#2478

And here is the slack discussion https://json-schema.slack.com/archives/C5CF75URH/p1613937923034800

CodegenModel is one of our classes which implemente Schema and includes some extra info.

Fixes typo

Adds fix for codegenProperty and a test of it

Removes comment

Clarifies comment

Clarifies what setVars is setting in IJsonSchemaValidationProperties

Adds testQueryParameterVarsCorrect testBodyParamAndResponseVarsCorrect

Removes unused imports

Samples updated

Moves fix for vars into python generator only

Samples and docs regen
@spacether
Copy link
Contributor Author

spacether commented Jun 3, 2021

So this change works to remove the vars that were hoisted into composed schemas from those composed schemas. They still exist in the source schemas and can be assigned as additional properties in the composed schemas.
One would still need to add the composed schema propertyName property back in to schemas that use it.

I am concerned about merging this because it will remove the variables from the properties and signatures of the composed schemas. While not a breaking change, I suspect that many users will not want that.
It may make sense for me to segregate this feature into my future python-experimental generator.

@spacether spacether closed this Nov 18, 2021
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.

[BUG] ComposedSchema vars incorrectly includes extra properties
2 participants