Skip to content

Conversation

@markus-wa
Copy link
Contributor

PR checklist

  • Read the contribution guildelines.
  • Ran the shell/batch 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)
  • Filed the PR against the correct branch: master for non-breaking changes and 2.3.0 branch for breaking (non-backward compatible) changes.

Description of the PR

Fix for #4569 - basePath was ignored for jaxrs-cxf

@markus-wa markus-wa changed the title jaxrs-cxf: Re-added usage of contextPath in api.mustache (basePath) [JAXRS-CXF] Issue 4569 - Re-added usage of contextPath in api.mustache (basePath) Jan 17, 2017
@wing328
Copy link
Contributor

wing328 commented Jan 18, 2017

@markus-wa Thanks for the PR but your commit (as shown in the Commits tab) is not linked to your Github account, which means this PR won't count as your contribution in https://github.com/swagger-api/swagger-codegen/graphs/contributors.

Let me know if you need help fixing it.

Ref: https://github.com/swagger-api/swagger-codegen/wiki/FAQ#how-can-i-update-commits-that-are-not-linked-to-my-github-account

@wing328
Copy link
Contributor

wing328 commented Jan 18, 2017

@jfiala please take a look at this PR when you've time.

@markus-wa
Copy link
Contributor Author

I have added the email from the commit to my github account.

@wing328 wing328 added this to the v2.2.2 milestone Jan 18, 2017
@jfiala
Copy link
Contributor

jfiala commented Jan 18, 2017

@wing328 I removed this when I reworked the cxf template, because using the basePath in the @path is not the usual configuration for CXF.
CXF uses spring and the basePath is configured in web.xml/spring configuration files:

@markus-wa
Please take a look at the generated web.xml file (with config generateSpringApplication=true)

<servlet-mapping>
        <servlet-name>CXFServiceServlet</servlet-name>
        <url-pattern>/rest/*</url-pattern>
    </servlet-mapping>

There the servlet mapping is done.

The next step is the JAXRS-server which is configured in Spring (see ApplicationContext.xml):

<jaxrs:server id="restServer" address="/services">

With these two configurations the services are now located at

http://<servername>/<appcontext-name>/rest/services/<myservices>

The JAXRS @path(..) in the interface is always connected to the root ("/"), as there are already two other places where you can configure the servlet-mapping and the jaxrs:server-endpoint.

This is how I worked with the cxf language so far, other usages usually caused troubles with other features, so that was the most stable and also understandable solution to start with.

If you simply need the basePath in @path, I'd suggest to customize the template (but I wonder then how you are doing servlet-mapping/jaxrs-endpoint configuration in spring) or if you think it makes really sense for the cxf-language, please also submit how the spring configuration should change in your opinion and verify all the features work (including swagger-ui, Swagger2Feature etc.), so pls test then with all the CLI flags turned on.

@wing328
So right now, pls don't merge, as the CXF server stub will no longer work then as designed/expected.

@markus-wa
Copy link
Contributor Author

Until now we have been using a single CxfNonSpringServlet for multiple services with different basePaths. The @path seems to be the only option in this case as far as I'm aware.

I will get back to you with more details - maybe a command line option might be the way to go?

@jfiala
Copy link
Contributor

jfiala commented Jan 19, 2017

@markus-wa I'm green with the CLI option, in fact the solution you proposed seems the cleaner way in JaxRS terms (contract -> jaxrs interface). You can also provide an alternative to the spring configuration I did by supporting a flag for the CxfNonSpringServlet, this would make it complete. Pls let me know if you need assistance in powering up the cxf language support...

…n flags

 * useAnnotatedBasePath=true uses the @path annotation in the generated interface to set the basePath, default is false

 * generateNonSpringApplication=true generates a web.xml with CXFNonSpringJaxrsServlets. If useAnnotatedBasePath=true only one servlet will be created, otherwise one for each API
@jfiala
Copy link
Contributor

jfiala commented Jan 26, 2017

@markus-wa great, this looks good!

@jfiala
Copy link
Contributor

jfiala commented Jan 26, 2017

for the generated samples, you generated them with the CLI flag useAnnotatedBasePath turned on. In fact we would need a separate sample for all off (=default), one with all spring flags turned on and one with the generateNonSpringServlet turned on. @wing328 pls can you advise here how we should handle this in your opinion?

@markus-wa
Copy link
Contributor Author

Woops, what happened here? https://travis-ci.org/swagger-api/swagger-codegen/jobs/195578396

@wing328
Copy link
Contributor

wing328 commented Jan 26, 2017

@markus-wa I just pushed a fix to the latest master. Pls pull the latest to give it a try.

@wing328
Copy link
Contributor

wing328 commented Jan 26, 2017

@wing328 pls can you advise here how we should handle this in your opinion?

A good starting is by looking at the following 2 shell scripts:
spring-delegate-j8.sh
spring-delegate.sh

The differences are the output folder (samples/server/petstore/springboot-delegate-j8) and the additional option (java8=true).

For your case, you can do something similar with the additional option generateNonSpringServlet.

If you want, I can do it later after merging this PR into master.

Walther added 3 commits January 26, 2017 17:53
…servlets

Multiple servlets were generated if useAnnotatedBasePath was false.
This is not necessary as there is only ONE basePath per contract
@markus-wa
Copy link
Contributor Author

@jfiala @wing328 I've added the sample generation, let me know if i missed something.

@wing328 wing328 merged commit 4900427 into swagger-api:master Feb 6, 2017
@wing328
Copy link
Contributor

wing328 commented Feb 6, 2017

@markus-wa I've resolved the merge conflicts and merge the changes into master. Thanks for your contribution.

Stwissel added a commit to Stwissel/swagger-codegen that referenced this pull request Feb 9, 2017
* 'master' of github.com:swagger-api/swagger-codegen: (40 commits)
  remove default temp folder during initalization (swagger-api#4749)
  [Java-retrofit] Fix for swagger-api#4750 String comparison with equals (swagger-api#4751)
  update java server stub samples with new uuid mapping
  update java petstore with new uuid mapping
  [Java-Feign] Fixed String comparison using equals instead of == operator (swagger-api#4740)
  add SPINEN
  update jaxrs spec petstore sample (mac)
  [Jaxrs-spec] fix usage of Jersey templates in shellscript (swagger-api#4722)
  [Bash] Bash generator improvements (swagger-api#4730)
  [Java][Issue swagger-api#1806] generate using java.util.UUID for UUIDs
  Revert "rx2 support" (swagger-api#4737)
  rx2 support (swagger-api#4708)
  add https and change order for HPE
  add Hewlett Packard Enterprise (hpe.com)
  Add "Simpfony" to list of companies using Swagger (swagger-api#4726)
  add https://www.slamby.com/
  [csharp] Fix enum default value (swagger-api#4681)
  fix issue swagger-api#4672 - XmlExampleGenerator does not properly handle properties of several numeric types (swagger-api#4673)
  [JAXRS-CXF] Issue 4569 - Re-added usage of contextPath in api.mustache (basePath) (swagger-api#4580)
  [Jaxrs-cxf-cdi] merge beanvalidation templates to single one swagger-api#4719 (swagger-api#4723)
  ...
Stwissel added a commit to Stwissel/swagger-codegen that referenced this pull request Feb 9, 2017
* master: (40 commits)
  remove default temp folder during initalization (swagger-api#4749)
  [Java-retrofit] Fix for swagger-api#4750 String comparison with equals (swagger-api#4751)
  update java server stub samples with new uuid mapping
  update java petstore with new uuid mapping
  [Java-Feign] Fixed String comparison using equals instead of == operator (swagger-api#4740)
  add SPINEN
  update jaxrs spec petstore sample (mac)
  [Jaxrs-spec] fix usage of Jersey templates in shellscript (swagger-api#4722)
  [Bash] Bash generator improvements (swagger-api#4730)
  [Java][Issue swagger-api#1806] generate using java.util.UUID for UUIDs
  Revert "rx2 support" (swagger-api#4737)
  rx2 support (swagger-api#4708)
  add https and change order for HPE
  add Hewlett Packard Enterprise (hpe.com)
  Add "Simpfony" to list of companies using Swagger (swagger-api#4726)
  add https://www.slamby.com/
  [csharp] Fix enum default value (swagger-api#4681)
  fix issue swagger-api#4672 - XmlExampleGenerator does not properly handle properties of several numeric types (swagger-api#4673)
  [JAXRS-CXF] Issue 4569 - Re-added usage of contextPath in api.mustache (basePath) (swagger-api#4580)
  [Jaxrs-cxf-cdi] merge beanvalidation templates to single one swagger-api#4719 (swagger-api#4723)
  ...
@wing328 wing328 changed the title [JAXRS-CXF] Issue 4569 - Re-added usage of contextPath in api.mustache (basePath) [JAXRS-CXF] Re-added usage of contextPath in api.mustache (basePath) Feb 21, 2017
@markus-wa markus-wa deleted the issue-4569 branch March 29, 2017 10:23
davidgri pushed a commit to davidgri/swagger-codegen that referenced this pull request May 11, 2017
…e (basePath) (swagger-api#4580)

* jaxrs-cxf: Re-added usage of contextPath in api.mustache (basePath)

* jaxrs-cxf: Added useAnnotatedBasePath and generateNonSpringApplication flags

 * useAnnotatedBasePath=true uses the @path annotation in the generated interface to set the basePath, default is false

 * generateNonSpringApplication=true generates a web.xml with CXFNonSpringJaxrsServlets. If useAnnotatedBasePath=true only one servlet will be created, otherwise one for each API

* jaxrs-cxf generateNonSpringApplication: removed creation of multiple servlets

Multiple servlets were generated if useAnnotatedBasePath was false.
This is not necessary as there is only ONE basePath per contract

* jaxrs-cxf: added sample generation for useAnnotatedBasePath and generateNonSpringApplication
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.

3 participants