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

Handling path parameters declaration place #77

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dmitryd20
Copy link
Contributor

@dmitryd20 dmitryd20 commented Nov 8, 2021

Closes #64

Done:

  • taking path parameters from Path object instead of Operation
  • giving warning if path parameters are declared in Operation

Details:

First, let's compare two specs.

1:

paths:
      /messages/{id}:
        parameters:
          - name: id
            required: true
            in: path
            schema:
              type: string
        get:
          responses:
            default:
                description: "Все ок"
                content:
                    application/json:
                        schema:
                            type: integer

2:

paths:
      /messages/{id}:
        get:
          parameters:
            - name: id
              required: true
              in: path
              schema:
                type: string
          responses:
            default:
                description: "Все ок"
                content:
                    application/json:
                        schema:
                            type: integer

In spec 1 path parameter id is declared inside path object and this it how it should be.

In spec 2 path the parameter is declared inside operation. Previously we didn't take attention on this, but this declaration is a potential problem. If there are more then one operations for one path, the parameter should be duplicated for each operation. So now we give warning if path parameters are declared in Operation, like in spec 2.

How to check:

Specs from example above are used in ParameterTests.

Spec 2 is used in SwaggerCorrectorTests to check if SwaggerCorrector moves path parameters declared in Operation to Path

@dmitryd20 dmitryd20 requested a review from LastSprint November 8, 2021 10:18
@dmitryd20 dmitryd20 self-assigned this Nov 8, 2021
@LastSprint
Copy link
Contributor

I'm not sure that this is correct behavior.
For instance:

paths:
  /user:
    get:
      summary: Return specific user buy id
      parameters:
        - name:
           required: true
           in: path
           schema:
             type: string
    ...........
    post:
       summary: Create new user
    ...........
    delete:
       summary: Delete specific user
      parameters:
        - name:
           required: true
           in: path
           schema:
             type: string
     ...........

So I declared CRUD controller in the way when only a part of operations contains path parameter which is declared in operation.

Tbh I don't know is it useful, but I guess that we have to mange situation with global parameters as it is - I means we have to add global parameter to each operation, don't we?

@dmitryd20
Copy link
Contributor Author

So I declared CRUD controller in the way when only a part of operations contains path parameter which is declared in operation.

Situation where only a part of operations in service use path parameter is complicated for generation.
In your example path string is /user. It doesn't include parameter template, so it is impossible to guess, where to paste parameter value. And if path string is /user/{id}, the template for parameter id will be unused with POST operation, which leads to an error too.
As I see, the only way to solve this is to declare /user and /user/{id} as different paths.

@LastSprint
Copy link
Contributor

So I declared CRUD controller in the way when only a part of operations contains path parameter which is declared in operation.

Situation where only a part of operations in service use path parameter is complicated for generation. In your example path string is /user. It doesn't include parameter template, so it is impossible to guess, where to paste parameter value. And if path string is /user/{id}, the template for parameter id will be unused with POST operation, which leads to an error too. As I see, the only way to solve this is to declare /user and /user/{id} as different paths.

And if I will rewrite it in that way

paths:
  /user/{id}:
    get:
      summary: Return specific user buy id
      parameters:
        - name: id
           required: true
           in: path
           schema:
             type: string
    ...........
    post:
       summary: Create new user
    ...........
    delete:
       summary: Delete specific user
      parameters:
        - name: id
           required: true
           in: path
           schema:
             type: string
     ...........

Now it's possible to guess and POST doesn't have any parameter

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.

Wrong handling path parameters
2 participants