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

Matrix parameter is not generated correctly #1967

Open
cristalp opened this issue Aug 28, 2024 · 17 comments
Open

Matrix parameter is not generated correctly #1967

cristalp opened this issue Aug 28, 2024 · 17 comments
Labels
bug Something isn't working

Comments

@cristalp
Copy link
Contributor

We have a couple of endpoints with matrix parameters.

One of them looks like this:

  public Response getChanged(
...
      @Parameter(required = false, description = "Filter für die zu berücksichtigenden Geschäftsstati: " +
          "Bei leerem Wert erfolgt keine Einschränkung")
      @MatrixParam("status") final List<String> status,
...
  }

(I have kept the original German description because of the JSON that follows later.)

With our original toolchain with Swagger annotations, I get:

    "/api/changed" : {
      "get" : {
...
        {
          "name" : "status",
          "in" : "path",
          "description" : "Filter für die zu berücksichtigenden Geschäftsstati: Bei leerem Wert erfolgt keine Einschränkung",
          "required" : true,
          "style" : "matrix",
          "schema" : {
            "type" : "array",
            "items" : {
              "type" : "string"
            }
          }
        },
...

Note that it's generated as required, which is a bug in that Swagger Maven plugin.

In SwaggerUI, it looks like this and can be used:

old

With SmallRye, I get:

    "/api/changed{changed}" : {
      "get" : {
...
        {
          "name" : "changed",
          "in" : "path",
          "required" : true,
          "schema" : {
            "type" : "object",
            "properties" : {
              "status" : {
                "type" : "array",
                "items" : {
                  "type" : "string"
                }
              }
            }
          },
          "style" : "matrix",
          "explode" : true
        },
...

In SwaggerUI, it looks like this and can't be used:
new

I see several problems:

  • The parameter name is not taken from the code, but generated probably from the method name.
  • The parameter is shown as required, even though required = false.
  • The description of the parameter is ignored.
  • It doesn't work, which is why I suggest that matrix parameters should be generated in the same way that the Swagger toolchain does it.
@MikeEdgar MikeEdgar added the bug Something isn't working label Aug 28, 2024
@MikeEdgar
Copy link
Member

Definitely a bug

@cristalp
Copy link
Contributor Author

I just noticed that example within @Parameter doesn't work either.

@cristalp
Copy link
Contributor Author

Also, the matrix parameter name is sometimes added to the show path, like /api/foobar{my-matrix-parameter}. I can't define whether this is a good or bad thing, but it looks kind of weird.

@MikeEdgar
Copy link
Member

I think that needs to be there because different matrix params could be located in different path segments.

/api/segment1{seg1-matrix}/segment2{seg2-matrix}

@MikeEdgar
Copy link
Member

@cristalp the topic of matrix parameters comes up so infrequently, I had to relearn it from what little examples and documentation exists 😆 I believe both the old form and the new form are technically correct. That is, if you were to provide {"status":["status-value"]} as the value for the changed parameter, it would be serialized properly by Swagger UI. Did you try any values in the JSON object for changed?

The way smallrye currently processes matrix parameters is by assigning the name to be the same as the URL segment that the matrix is attached to, in this case changed. It then populates each matrix parameter into that matrix as a property. This is why the schema type is object. If you had multiple @MatrixParams at the same path, it would maybe be clearer.

@cristalp
Copy link
Contributor Author

cristalp commented Aug 30, 2024

@MikeEdgar There's nothing better than revisiting age-old code ;-)

Well, there's two things to this. First of all it's the user friendliness. We have people using Swagger UI who couldn't tell JSON from a tomato ;-)

So, they can understand this (old solution):
old

But they can't understand this:
new

Also, as you mentioned, the name in SwaggerUI is taken from the URL segment. Now this is very confusing for the users. Even if they have to enter JSON, the parameter should be generated as status, not as changed. And the description should be added.

And second, it works with the old solution, but it doesn't with SmallRye. I put in some logging:

System.out.println("*** Status " + status);
System.out.println("*** " + status.getClass());
System.out.println("*** " + status.get(0).getClass());

And I get:

*** Status [["foo","bar"]]
*** class org.jboss.resteasy.core.StringParameterInjector$UnmodifiableArrayList
*** class java.lang.String

The first output looks to me as if it's a List<List<String>>, not a List<String>, but the rest looks ok. I'm not sure why it doesn't work, though.

@MikeEdgar
Copy link
Member

Can you see what the URL looks like for the object-style matrix param? I'm curious how the server receives it. I have definitely seen something like what you show where a string list is converted to a list of a single string, which itself is a toString of the actual list. I forget the details, but I believe it was happening in RestEasy.

Anyway, I agree this can be improved. I need to take some time to try some different scenarios to see how Swagger UI handles it, as well as the server.

@cristalp
Copy link
Contributor Author

I'm not sure whether I understand what you mean.

This is what is generated by SmallRye:

    "/api/changed{changed}" : {
      "get" : {
        "parameters" : [ {
          "name" : "changed",
          "in" : "path",
          "required" : true,
          "schema" : {
            "type" : "object",
            "properties" : {
              "status" : {
                "type" : "array",
                "items" : {
                  "type" : "string"
                }
              }
            }
          },
          "style" : "matrix",
          "explode" : true
        },
...
    }

@MikeEdgar
Copy link
Member

When you submit the request with {"status":["foo","bar"]} in Swagger UI, what does the generated curl command show was used as the URL for the request?

@cristalp
Copy link
Contributor Author

cristalp commented Aug 30, 2024

Ah, ok! There are a couple of other parameters, but here's what I see:

curl -X 'GET' \
  'http://localhost:8080/service/api/changed;status=%5B%22foo%22%2C%22bar%22%5D?dateTimeFrom=2021-01-30T10%3A22%3A44&dateTimeTo=2021-05-14T15%3A15%3A00' \
  -H 'accept: */*'

Unescaped:

curl -X 'GET' \
  'http://localhost:8080/service/api/changed;status=["foo","bar"]?dateTimeFrom=2021-01-30T10:22:44&dateTimeTo=2021-05-14T15:15:00' \
  -H 'accept: */*'

@MikeEdgar
Copy link
Member

Thanks. Let me try some variations to see what works. There is also a good chance that since the documentation is so minimal (that I can see) on matrix parameters, neither Swagger UI nor the server implementations align with it. Worst case, we can get this resolved by making sure you are able to specific an OAS @Parameter annotation that accomplishes what you need given your environment.

@cristalp
Copy link
Contributor Author

Hmmm... if I do the same with the old toolchain, I get

curl -X 'GET' \
  'https://localhost:8080/service/api/changed?dateTimeFrom=2021-01-30T10:22:44&dateTimeTo=2021-01-31T15:15:00' \
  -H 'accept: */*'

So, it looks good in SwaggerUI, but effectively does nothing!

@cristalp
Copy link
Contributor Author

Yes, anything that looks nice in SwaggerUI and works is fine by me.

@MikeEdgar
Copy link
Member

Oh that's interesting, so Swagger UI isn't actually sending anything to the server with the old style?

@cristalp
Copy link
Contributor Author

Yes, nothing. Good looks, but that's all ;-)

@MikeEdgar
Copy link
Member

@cristalp can you confirm that status serializes like ;status=["foo","bar"] ? When I try it, Swagger UI generates ;status=foo,bar. That is also incorrect, Swagger UI is not respecting the explode: true, which should result in ;status=foo;status=bar.

Which server are you using? I can say that Quarkus does not seem to support explode: false, which would mean the parameter is sent the way Swagger UI does, ;status=foo,bar.

@cristalp
Copy link
Contributor Author

Well, that's what I see in SwaggerUI, yes.
You're right, in neither case are these matrix parameters.

We're using JBoss EAP 7.4.18.GA (WildFly Core 15.0.37.Final-redhat-00001). And this is a JEE application, not Quarkus.

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
Development

No branches or pull requests

2 participants