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

Fix model sanitisation #19

Merged
merged 4 commits into from
Feb 20, 2019

Conversation

macisamuele
Copy link
Collaborator

The goal of this PR is to address a problem, discovered internally, related to the too aggressive approach during sanitisation.

Model named as "model with spaces" or "model.with.dots" could lead to inconsistent names.
The updated approach does avoid the split of the model name but instead does replace the "bad" characters with something (an underscore) that will force a valid PascalCase representation and will be removed by sanitizeKotlinSpecificNames call.


Internally we had something similar to

... # this is only a snipped of the specs
definitions:
  a:
    type: object
    x-model: a
  wrapper:
    a:
      additionalProperties:
        $ref: '#/definitions/a'
      type: object
      x-model: wrapper.a

Without the fix the generated code ignores the fact that #/definitions/wrapper/properties/a should be a Map<String, A> and it generates as A and this is bad as it changes the semantic of the endpoint and potentially makes parsing the response crashing as incompatible.

With the fix the generated code for #/definitions/wrapper/properties/a will be WrapperA (a type alias to Map<String, A>) which is what we would expect.

Copy link
Collaborator

@cortinico cortinico left a comment

Choose a reason for hiding this comment

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

LGTM :)

@cortinico cortinico merged commit 78731ce into Yelp:master Feb 20, 2019
@macisamuele macisamuele deleted the maci-fix-model-sanitisation branch February 20, 2019 17:13
@cortinico cortinico added this to the 1.1.0 milestone Jun 13, 2019
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.

None yet

2 participants