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

Fixed referenced parameters validation #1065

Merged
merged 3 commits into from
Apr 9, 2019

Conversation

jakubmalek
Copy link
Contributor

Fix for issue #1063.

@@ -1408,13 +1412,25 @@ else if(!v.isValueNode()) {
}
Set<String> filter = new HashSet<>();

for(Parameter param:parameters) {
parameters.stream().map(this::getParameterDefinition).forEach(param -> {
if(!filter.add(param.getName()+"#"+param.getIn())) {
Copy link
Contributor

@cvgaviao cvgaviao Apr 14, 2019

Choose a reason for hiding this comment

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

@gracekarina , this is not working when the parameter is coming from another file. param.getName() will always return null in this case, so it won't pass the next filter.

I do not understand yet how $ref works, but seems that the getParameterDefinition must return a different value.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi please, send us a spec where this will fail so we can test and addressed the issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

File one:

openapi: 3.0.0
info:
  version: 1.0.0
  title: an API
  description: An API for reproduce a *There are duplicate parameter values*.

paths:
  /anPath:
    get:
      parameters:
        - $ref: './references.oas3.yaml#/components/parameters/ParamQ_One'
        - $ref: './references.oas3.yaml#/components/parameters/ParamQ_Two'
      responses:
        '200':
          description: OK
          content:
            application/json:
              schema:
                type: array
                items:
                  type: string

file references.oas3.yaml:

components:
  parameters:
    ParamQ_One:
      in: query
      name: customer-id
      required: true
      schema:
        format: int32
        type: integer
    ParamQ_Two:
      in: query
      name: unit-id
      schema:
        format: int32
        type: integer

Copy link
Contributor

Choose a reason for hiding this comment

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

well, to resolve parameters it should be done after deserializing in the resolver part. I'm looking at it, but this is a different issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

@gracekarina,
well, could be, but I don't think it is a different issue...
we'll still get 'duplicated values' because a parameter is only unique when we have its name + its type. if we don't have its type then the problem of duplicate will arise and will preventing any generator to work.

Copy link
Contributor

Choose a reason for hiding this comment

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

What could be done is validate in deserializing time, if the ref in the parameter is internal, or remote so we would avoid the message warning.

Copy link
Contributor

@cvgaviao cvgaviao Apr 18, 2019

Choose a reason for hiding this comment

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

Agreed, and I don't see any option other than to resolve that $ref at this deserializing time... :)
Perhaps, borrow some code from io.swagger.v3.parser.processors.ParameterProcessor.processParameter(Parameter) ?

Copy link
Contributor

Choose a reason for hiding this comment

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

merged PR #1080

Copy link
Contributor

@cvgaviao cvgaviao Apr 19, 2019

Choose a reason for hiding this comment

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

@gracekarina, well done, girl !
thanks

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.

3 participants