-
Notifications
You must be signed in to change notification settings - Fork 89
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
Comments
Definitely a bug |
I just noticed that |
Also, the matrix parameter name is sometimes added to the show path, like |
I think that needs to be there because different matrix params could be located in different path segments.
|
@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 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 |
@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): But they can't understand this: 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 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:
The first output looks to me as if it's a |
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 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. |
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
},
...
} |
When you submit the request with |
Ah, ok! There are a couple of other parameters, but here's what I see:
Unescaped:
|
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 |
Hmmm... if I do the same with the old toolchain, I get
So, it looks good in SwaggerUI, but effectively does nothing! |
Yes, anything that looks nice in SwaggerUI and works is fine by me. |
Oh that's interesting, so Swagger UI isn't actually sending anything to the server with the old style? |
Yes, nothing. Good looks, but that's all ;-) |
@cristalp can you confirm that status serializes like Which server are you using? I can say that Quarkus does not seem to support |
Well, that's what I see in SwaggerUI, yes. 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. |
We have a couple of endpoints with matrix parameters.
One of them looks like this:
(I have kept the original German description because of the JSON that follows later.)
With our original toolchain with Swagger annotations, I get:
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:
With SmallRye, I get:
In SwaggerUI, it looks like this and can't be used:
I see several problems:
required = false
.The text was updated successfully, but these errors were encountered: