-
Notifications
You must be signed in to change notification settings - Fork 6k
add jersey 2 to jaxrs generator #2263
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
Conversation
@David-Marin-Calleja thanks for the PR but the CI tests failed. Here is part of the result:
You can run the test locally by |
I'll review this tomorrow and let you know if I've any questions. Thanks again for the contribution. |
I got the following when running
Would you please take a look? Can you also create |
@wing328 I'm not sure how this got in: But it's illegal by the Swagger Spec and may be confusing to people. Query parameters are not allowed in the path segment... |
@fehguy we added those (fake endpoints) internationally for unit testing. Agree that query parameters should not be allowed in the path segment. FYI. |
Can we just remove that and replace it with a full path instead? Because it's illegal, people will see it and think it's OK. Actually codegen (via parser) should not allow it, so if I add that check in the parser, you'll start failing the build. |
Understood the side effect of putting that in Petstore. If using the full path, the endpoint won't work since we need to modify the Petstore server as well. I might have missed it but do we have the source code of Petstore server available publicly? (btw, other Java server stub generator does not throw errors for path with URL query string) |
The petstore source lives here: https://github.com/swagger-api/swagger-samples/tree/docker/java/java-jaxrs It doesn't matter if other generators work, it's simply illegal with the swagger definition to define the URL with a query string. They really shouldn't work. |
OK. We'll look into swagger-sample to add more proper test cases before removing the workaround. |
@David-Marin-Calleja I'd a look at the error message again and it seems like
Is that accidentally removed as part of this PR?
|
@wing328 I will look into it. |
This was handled in #2488. |
adding jersey 2 to Java server (jaxrs). Command line example:
generate -l jaxrs --library jersey2