smallrye / smallrye-open-api

SmallRye implementation of Eclipse MicroProfile OpenAPI
Apache License 2.0
115 stars 89 forks source link

Matrix parameter is not generated correctly #1967

Open cristalp opened 2 weeks ago

cristalp commented 2 weeks ago

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:

MikeEdgar commented 2 weeks ago

Definitely a bug

cristalp commented 2 weeks ago

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

cristalp commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

@cristalp the topic of matrix parameters comes up so infrequently, I had to relearn it from what little examples and documentation exists :laughing: 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 commented 2 weeks ago

@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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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

MikeEdgar commented 2 weeks ago

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

cristalp commented 2 weeks ago

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

MikeEdgar commented 2 weeks ago

@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 commented 2 weeks ago

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.