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

Duplicate path params for subresources that reference param multiple times #1256

Closed
MikeEdgar opened this issue Sep 13, 2022 · 3 comments · Fixed by #1259 or #1390
Closed

Duplicate path params for subresources that reference param multiple times #1256

MikeEdgar opened this issue Sep 13, 2022 · 3 comments · Fixed by #1259 or #1390
Labels
bug Something isn't working
Milestone

Comments

@MikeEdgar
Copy link
Member

See quarkusio/quarkus#27902

@Spindl
Copy link

Spindl commented Feb 9, 2023

I fear that this fix did not do the trick.

I can still confirm this behavior not only for path parameters but also for query parameters in two other use cases, one being Quarkus and the other using the maven plugin to generate the openapi.jsons.
And at least in the case of the duplicated query parameters this also has an impact on clients generated from the openapi.json because the parameters will be added to the signatures of all generated operation methods for the given path, even though they only belong to one operation.

Here are examples for both cases.
In both of them, the duplicated query parameters come from @BeanParam beans and are related to paging, even though they look a bit different.

Here is the shortened example for Quarkus 2.15.0.Final (which uses version 2.3.1 if I got that correctly). As you can see, the paging parameters are present in the GET operation "filterCoreTests" as well as in the Path element for "/core-tests". This causes the generated method signature for the POST operation "createCoreTest" to have them as well, which is wrong.

{
  "/core-tests": {
    "get": {
      "operationId": "filterCoreTests",
      "parameters": [
        {
          "name": "pageNr",
          "in": "query",
          "description": "The number of the page to retrieve.",
          "schema": {
            "format": "int32",
            "default": "1",
            "maximum": 100000,
            "minimum": 1,
            "type": "integer"
          }
        },
        {
          "name": "pageSize",
          "in": "query",
          "description": "The size of a single page.",
          "schema": {
            "format": "int32",
            "maximum": 1000,
            "minimum": 0,
            "type": "integer"
          }
        },
        {
          "name": "sortBy",
          "in": "query",
          "description": "A string specifying by which properties to sort.",
          "schema": {
            "type": "string"
          }
        }
      ],
      "responses": {...}
    },
    "post": {
      "operationId": "createCoreTest",
      "requestBody": {...},
      "responses": {...}
    },
    "parameters": [
      {
        "name": "pageNr",
        "in": "query",
        "description": "The number of the page to retrieve.",
        "schema": {
          "format": "int32",
          "default": "1",
          "maximum": 100000,
          "minimum": 1,
          "type": "integer"
        }
      },
      {
        "name": "pageSize",
        "in": "query",
        "description": "The size of a single page.",
        "schema": {
          "format": "int32",
          "maximum": 1000,
          "minimum": 0,
          "type": "integer"
        }
      },
      {
        "name": "sortBy",
        "in": "query",
        "description": "A string specifying by which properties to sort.",
        "schema": {
          "type": "string"
        }
      }
    ]
  }
}

And here is the example from the maven plugin in the current version 3.1.2.
As you can see the same problem occurs here, the parameters are duplicated as well.

{
  "/core-test/core-tests": {
    "get": {
      "operationId": "core-tests_search",
      "parameters": [
        {
          "name": "limit",
          "in": "query",
          "description": "An integer which is the limit (max number) of returned records (used for pagination).",
          "schema": {
            "format": "int32",
            "maximum": 10000,
            "minimum": 0,
            "type": "integer"
          }
        },
        {
          "name": "offset",
          "in": "query",
          "description": "An integer (< list-size-1) that is the offset of the first record to transmit (used for pagination).",
          "schema": {
            "format": "int32",
            "minimum": 0,
            "type": "integer"
          }
        },
        {
          "name": "page",
          "in": "query",
          "description": "An integer that designates the page number of the first record to transmit (used for pagination).",
          "schema": {
            "format": "int32",
            "minimum": 0,
            "type": "integer"
          }
        },
        {
          "name": "sort",
          "in": "query",
          "description": "The requested order of the result. Valid sorting properties: id, name, parent, strategyName.",
          "schema": {
            "type": "string"
          }
        }
      ],
      "responses": {...},
    },
    "post": {
      "operationId": "core-tests_create",
      "parameters": [
        {
          "name": "_includeMetadata",
          "in": "query",
          "description": "Determines whether meta data should be included in the response (not included by default).",
          "required": false,
          "schema": {
            "type": "boolean"
          }
        }
      ],
      "requestBody": {...},
      "responses": {...},
    },
    "parameters": [
      {
        "name": "limit",
        "in": "query",
        "description": "An integer which is the limit (max number) of returned records (used for pagination).",
        "schema": {
          "format": "int32",
          "maximum": 10000,
          "minimum": 0,
          "type": "integer"
        }
      },
      {
        "name": "offset",
        "in": "query",
        "description": "An integer (< list-size-1) that is the offset of the first record to transmit (used for pagination).",
        "schema": {
          "format": "int32",
          "minimum": 0,
          "type": "integer"
        }
      },
      {
        "name": "page",
        "in": "query",
        "description": "An integer that designates the page number of the first record to transmit (used for pagination).",
        "schema": {
          "format": "int32",
          "minimum": 0,
          "type": "integer"
        }
      },
      {
        "name": "sort",
        "in": "query",
        "description": "The requested order of the result.",
        "schema": {
          "type": "string"
        }
      }
    ]
  }
}

In this version I could also dig into the reason a little bit, and it seems that excludeOperationParameters added in #1259 is never called because the locatorPathParameter list is always null.
The parameter is set to null in the JaxRsAnnotationScanner and only forwarded from there on.

@MikeEdgar
Copy link
Member Author

@Spindl - do you have a small reproducer? The fix and test case must miss some aspect of this problem.

cc: @bcluap, @tanadeau

@MikeEdgar MikeEdgar reopened this Feb 9, 2023
@Spindl
Copy link

Spindl commented Feb 9, 2023

Yeah, I finally managed to reproduce the problem. It seems to be related to generic methods (or their bridge methods), which are not found in the ancestry and thus their parameters are evaluated for the path items.

You can find the reproducer here: https://github.com/Spindl/smallrye-openapi-1256-reproducer

I think some things I did are not required, like the split in a API and a Impl module. The crucial factor seems to be the generic filter which requires the compiler to create the bridge method in the RestInterface.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
2 participants