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

[JAXRS] use contextPath variable for @ApplicationPath in RestApplication #850

Merged
merged 9 commits into from
Aug 22, 2018
Merged

[JAXRS] use contextPath variable for @ApplicationPath in RestApplication #850

merged 9 commits into from
Aug 22, 2018

Conversation

michbeck100
Copy link
Contributor

PR checklist

  • Read the contribution guidelines.
  • Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh and ./bin/security/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\.
  • Filed the PR against the correct branch: master, 3.3.x, 4.0.x. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

The mustache templates for the JAXRS application define / in the @ApplicationPath annotation. This PR replaces this with {{{contextPath}}}, which contains the path of the server url. If no additional path is given and the server url contains no trailing / the value is an empty string. This will be replaced again with / by JAXRS.

@michbeck100
Copy link
Contributor Author

@wing328 Any more comments?

@jmini
Copy link
Member

jmini commented Aug 21, 2018

@michbeck100: reviewing PRs take some time. This PR is also on my list...

To ease test: can you remind me how to create different values for {{{contextPath}}} ? I just need to generate project for a small spec with a server http://api.something.com/ and then with a server http://api.something.com/v2? In the first case I should get @ApplicationPath("") and @ApplicationPath("/v2") in the second case?

@michbeck100
Copy link
Contributor Author

Exactly, but in the first case you will get @ApplicationPath("/"), but you can also omit the /. The JAXRS specification allows that

@wing328
Copy link
Member

wing328 commented Aug 21, 2018

[main] INFO  o.o.codegen.DefaultCodegen - Skipped overwriting RestApplication.java as the file already exists in /Users/williamcheng/Code/openapi-generator/samples/server/petstore/jaxrs-resteasy/default/src/gen/java/org/openapitools/api/RestApplication.java

@michbeck100 can you please manual remove samples/server/petstore/jaxrs-resteasy/default/src/gen/java/org/openapitools/api/RestApplication.java and regenerate the sample again by running ./bin/jaxrs-resteasy-petstore-server.sh?

@michbeck100
Copy link
Contributor Author

@wing328 This behavior exists not just for jars-resteasy. The RestApplication.java and pom.xml are only written if they don't already exist. Maybe this should be changed, as not every developer is aware of this?

@jmini
Copy link
Member

jmini commented Aug 21, 2018

@michbeck100:
I have the vision that the samples/ that it should be possible to delete all the samples/ folder and to create it with all the bin/*.sh scripts we have (see issues #80 and other).
But we are not there yet.

For standard usage of the generator, splitting the files into a folder that is regenerated each time src-gen/ and a folder that you keep src/ is great. => generation gap pattern.

For usages with our samples/ folder, which is a sort of integration test of our generator, this is not optimal. Some bin scripts have started to add a small delete command that make the generator regenerate all items (see some of the spring scripts), but this is also not the case everywhere.

@jmini
Copy link
Member

jmini commented Aug 22, 2018

I found an other case: <openapi-generator>/samples/server/petstore/jaxrs-spec/src/gen/java/org/openapitools/api/RestApplication.java that I have updated with 227dc43

@jmini jmini merged commit 526e980 into OpenAPITools:master Aug 22, 2018
@jmini jmini added this to the 3.2.2 milestone Aug 22, 2018
@jmini
Copy link
Member

jmini commented Aug 22, 2018

The change looks good so we decided to merge it now in order to be in the 3.2.2 release.


One question:
How do you test a project created with jaxrs-spec how do you test it? I mean for example starting a jetty that serves this server (same question for jaxrs-resteasy)


One finding:

The trailing / is not removed:

servers:
  - url: 'http://localhost:9999/'

Creates

@ApplicationPath("/")

And

servers:
  - url: 'http://localhost:9999/api/v3/'

Creates

@ApplicationPath("/api/v3/")

I am not sure this can be added in DefaultGenerator, as it will impact all generators.
And maybe it is not a problem.

@michbeck100
Copy link
Contributor Author

For JAXRS it's not a problem. I personally like the trailing / even more than an empty string.

@jmini jmini changed the title [JAXRS] use contextPath variable for RestApplication templates [JAXRS] use contextPath variable for @ApplicationPath in RestApplication Aug 22, 2018
@jmini
Copy link
Member

jmini commented Aug 22, 2018

For JAXRS it's not a problem. I personally like the trailing / even more than an empty string.

Glad to hear this.

Please if you can me an hint on how to test the a project generated with jaxrs-spec or jaxrs-resteasy. I would appreciate it.

@michbeck100 michbeck100 deleted the context_path branch August 23, 2018 06:30
@michbeck100
Copy link
Contributor Author

A given openapi yaml like this:

servers:
- url: http://localhost:8080/v2
paths:
  /pet:

would generate

@ApplicationPath("/v2")

Once you start the server, the url to /pet would be http://localhost:8080/v2/pet. Before the change this was http://localhost:8080/pet without v2, because the base path was fixed to /.

@jmini
Copy link
Member

jmini commented Aug 23, 2018

@michbeck100 : yes I have checked the generated code.

The part I am missing is: "Once you start the server" (this is probably a dummy Java/JackartaEE question, but what is the quickest way to start a server like a jetty with a JaxRS implementation. I have no JavaEE App server installed).

@michbeck100
Copy link
Contributor Author

You could start a sample server with

$ mvn package org.apache.tomee.maven:tomee-embedded-maven-plugin:7.0.5:run

in the root dir of the sample where the pom.xml is. This will start an embedded TomEE server and deploy the war file from target. After that you will see the available REST resources in the log output and you access them with postman for example.

@wing328
Copy link
Member

wing328 commented Aug 24, 2018

$ mvn package org.apache.tomee.maven:tomee-embedded-maven-plugin:7.0.5:run

What about adding that to an auto-generated README?

@jmini
Copy link
Member

jmini commented Aug 24, 2018

@wing328 already on my list.

@jmini jmini mentioned this pull request Aug 29, 2018
4 tasks
@jmini
Copy link
Member

jmini commented Aug 29, 2018

What about adding that to an auto-generated README?

PR #920

A-Joshi pushed a commit to ihsmarkitoss/openapi-generator that referenced this pull request Feb 27, 2019
…ion (OpenAPITools#850)

* Use contextPath variable for RestApplication templates
* Update generated RestApplication.java files, as they are skipped by default otherwise
* Update Petstore sample for jaxrs so that CIs can verify the change
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants