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

Allow to use of multiple Swagger configurations per single ServletConfig using base path as descriminator #1656

Merged

Conversation

reta
Copy link
Contributor

@reta reta commented Feb 10, 2016

Hey guys,

Please take a look on this enhancement which allows to use multiple Swagger configurations per single servlet (more precisely, ServletConfig). Essentially, this is more generic approach than the one taken by #1482 and is using the base path as a unique context discriminator. The original discussion has been started here https://groups.google.com/forum/#!topic/swagger-swaggersocket/bf7uVBWYRTw and the solution suggested by @frantuma has been implemented.

There are two configuration parameters available:

  • Servlet init parameter 'swagger.use.path.based.config' = true | false
  • usePathBasedConfig property of BeanConfig

When those properties are set, the SwaggerContextService is going to use base path to uniquely identify the API Swagger's configuration components: context, config and scanner. This is very important in multiple use cases, for example when many independent APIs are deployed using single servlet (the best exampe could be OSGi container).

This approach works quite well for deploying multiple Swagger configs in the same JVM and even within same servlet. As the further suggestion, we could simplify SwaggerContextService a lot by removing SCANNER_ID_KEY, CONTEXT_ID_KEY and CONFIG_ID_KEY and rely solely on path-based configuration (less boilerplate to the users with the same goals being achieved).

What do you think guys?
Thanks a lot.

PS: The original issue was posted to Apache CXF JIRA is https://issues.apache.org/jira/browse/CXF-6740

Best Regards,
Andriy Redko & Aki Yoshida

@fehguy
Copy link
Contributor

fehguy commented Feb 11, 2016

Thanks Andriy and Aki. @frantuma is going through the PR now.

@frantuma
Copy link
Member

Hi Andriy & Aki, thanks. This seems to work ok, and shouldn't impact compatibility, so I'd say we can merge happily; maybe one small thing, I believe ApiListingResources gets something like "/api/" (leading and trailing slashes) for uriInfo.getBaseUri().getPath(). Possibly we could strip slash of do a wider match..
With regards to the suggestion to get rid of some "KEY mess" I would say we'd wait for next major release, also @fehguy pls let me know what you think.

Thanks and regards
Francesco

@reta
Copy link
Contributor Author

reta commented Feb 11, 2016

Hi Francesco,

Thanks a lot for your review and feedback. I think it is pretty good idea to have base path handled uniformly. I will do the change and update the PR shortly. Thank you!

Best Regards,
Andriy Redko

@reta reta force-pushed the master.multiple.configs.per.servlet branch from aed1d72 to 9fbd9bd Compare February 12, 2016 00:32
@reta
Copy link
Contributor Author

reta commented Feb 12, 2016

Hi Francesco,

The PR has been updated to normalize base path (by adding leading and trailing slashes), test cases, SwaggerContextService and ApiListingResources have been updated to use the proper property. Thanks again for spotting that.

Best Regards,
Andriy Redko

@webron
Copy link
Contributor

webron commented Mar 1, 2016

@frantuma - is this PR okay now? Obviously needs to be rebased before merging.

@reta reta force-pushed the master.multiple.configs.per.servlet branch 3 times, most recently from 08a207a to 8e70e02 Compare March 2, 2016 00:47
@reta
Copy link
Contributor Author

reta commented Mar 2, 2016

@webron @frantuma The PR has been rebased, thanks guys.

@reta reta force-pushed the master.multiple.configs.per.servlet branch 2 times, most recently from 5a70683 to f38dd3b Compare March 9, 2016 00:17
@reta
Copy link
Contributor Author

reta commented Mar 28, 2016

@frantuma @webron What do you think guys, could we merge this PR? Thanks in advance.

@reta reta force-pushed the master.multiple.configs.per.servlet branch from f38dd3b to 2d5bad8 Compare May 16, 2016 23:11
@frantuma frantuma merged commit 2d5bad8 into swagger-api:master May 18, 2016
frantuma added a commit that referenced this pull request May 18, 2016
…rvlet

Includes #1656 - Allow to use of multiple Swagger configurations per single ServletConfig using base path as descriminator
@webron
Copy link
Contributor

webron commented May 19, 2016

@reta, @elakito - pushed 1.5.10-SNAPSHOT so you can test it.

@reta
Copy link
Contributor Author

reta commented May 20, 2016

Awesome, thanks a lot, @webron !

@nailtonvieira
Copy link

Thanks a lot, @webron

@dheerajarora
Copy link

dheerajarora commented May 22, 2016

Thanks @webron . Could you please let me know when is the release data of 1.5.10?

@webron
Copy link
Contributor

webron commented May 22, 2016

We just released 1.5.9 a week ago, so there's no ETA for 1.5.10.

@swag-cxf
Copy link

Hi webron,

is there any update on 1.5.10 release schedule as we really want to use this feature with cxf multiple jax-rs resources.

@webron
Copy link
Contributor

webron commented Jul 26, 2016

No ETA, though we may try to get a release out in a couple of weeks. You can, of course, always use the SNAPSHOT.

@swag-cxf
Copy link

Do we have snapshot version which has this fix? please let me know artifact id?

@webron
Copy link
Contributor

webron commented Jul 26, 2016

From #1656 (comment):

pushed 1.5.10-SNAPSHOT so you can test it.

@swag-cxf
Copy link

Could you please tell me the snapshot repository location as it seems during build, pom.xml is not coming for modules due to which cxf build is failing.. Let me know snapshot-repo location.

@swag-cxf
Copy link

swag-cxf commented Aug 1, 2016

Hi Webron,

How do u expect client to pass on beanconfig with CXF Swagger2Feature? Is below looks correct as it's not working for me?

 <!-- CXF Swagger2Feature -->  
    <bean id="swagger2Feature" class="org.apache.cxf.jaxrs.swagger.Swagger2Feature">
        <property name="basePath" value="/cxf/swaggerSample2"/>
        <property name="usePathBasedConfig" value="true" />        
    </bean>

@swag-cxf
Copy link

swag-cxf commented Aug 1, 2016

Hi Webron,

How do u expect client to pass on beanconfig with CXF Swagger2Feature? Is below looks correct as it's not working for me?

@swag-cxf
Copy link

swag-cxf commented Aug 1, 2016

Don't know while updating comment, entries getting missed for BeanConfig.

@swag-cxf
Copy link

swag-cxf commented Aug 1, 2016

Not sure as i am not able to update xml elements in comments. Sorry for multiple comments.
Need to understand what extra value are expected in BeanConfig.

should usePathBasedConfig be passed as true? what else?

@webron
Copy link
Contributor

webron commented Aug 1, 2016

Swagger2Feature is part of the CXF library and is not part of swagger-core. You'd need to ask them for support on it.

@swag-cxf
Copy link

swag-cxf commented Aug 1, 2016

I have dropped a note on cxf-users list. What extra BeanConfig parameters you expect from 1.5.10 perspective to let multiple base path scannable? For ex, do you expect client to pass usePathBasedConfig true/false, and what other from OSGI perspective?

@reta
Copy link
Contributor Author

reta commented Aug 1, 2016

Hey @swag-cxf ,

Thanks a lot for your comments. Swagger2Feature is going to be updated once the Swagger Core (with this PR merged) is released. The patch is actually ready but CXF cannot depend on snapshot but release versions only (as a rule). The usage for non-OSGi / OSGi deployment looks pretty much how you posted it in here (XML snippet). Anyway, I would be happy to share the patch with you if you need it.

Sorry for not answering earlier, @webron, it is definitely CXF land :-)
Thank you!

Best Regards,
Andriy Redko

@swag-cxf
Copy link

swag-cxf commented Aug 1, 2016

Thanks Andriy.

It would be great if you can share the patch as I tried modifying Swagger2Feature to consider usePathBasedConfig and passed same in blueprint but seems not working.

beanConfig.setUsePathBasedConfig(Boolean.parseBoolean(getUsePathBasedConfig()));

<bean id="swagger2Feature" class="org.apache.cxf.jaxrs.swagger.Swagger2Feature">
    <property name="basePath" value="/cxf/swaggerSample"/>
    <property name="usePathBasedConfig" value="true" />
</bean>

@reta
Copy link
Contributor Author

reta commented Aug 1, 2016

Yeah, it needs is a little bit more work than just that. @swag-cxf , I will attach the patch against master to https://issues.apache.org/jira/browse/CXF-6740, today or tomorrow at latest. Thanks.

@swag-cxf
Copy link

swag-cxf commented Aug 1, 2016

Thanks really appreciate that @reta.

@webron
Copy link
Contributor

webron commented Aug 1, 2016

We've been planning to push 1.5.10 for a while now, but you know how that goes. I'll see if we can push an official release within the next couple of weeks (no promises though).

@swag-cxf
Copy link

swag-cxf commented Aug 1, 2016

Thanks @webron i can wait for couple of weeks:)

@reta
Copy link
Contributor Author

reta commented Aug 1, 2016

Thanks a lot, @webron !

@reta
Copy link
Contributor Author

reta commented Aug 1, 2016

@swag-cxf Patch against latest master attached to https://issues.apache.org/jira/browse/CXF-6740 (CXF-6740-Collision-by-Swagger.patch), please give it a try. We've tested it and it works perfectly fine, as expected (samples available jax_rs/description_swagger2_osgi, jax_rs/description_swagger2_per_path_config).

Please post your comments / issues on CXF ticket, we'll try to address them.

Thank you.

@nailtonvieira
Copy link

nailtonvieira commented Aug 2, 2016

Hello guys. What is the address of the snapshot repository? I tried:

<repository> 
             <Id> Swagger </id>
             <Url> https://oss.sonatype.org/content/repositories/snapshots/ </url>
</repository>

But it did not work.

@swag-cxf
Copy link

swag-cxf commented Aug 2, 2016

swagger-snapshot-repo Apache Maven Plugin Snapshots https://oss.sonatype.org/content/repositories/snapshots/ false true

@nailtonvieira
Copy link

Thanks @swag-cxf !

@swag-cxf
Copy link

Hi @webron,

Any update on deployment of 1.5.10?

Thanks.

@webron
Copy link
Contributor

webron commented Aug 12, 2016

Aiming for the end of next week. Been merging additional PRs and hoping to squeeze a few more in.

@swag-cxf
Copy link

thanks @webron

@swag-cxf
Copy link

Hi @webron,

by any chance 1.5.10 pushed?

@webron
Copy link
Contributor

webron commented Aug 19, 2016

You'll see it under https://github.com/swagger-api/swagger-core/releases once it is (plus, the main README will reflect it).

webron added a commit to swagger-api/swagger-samples that referenced this pull request Aug 19, 2016
bump core version, adds multi sample with basepath discriminator refs swagger-api/swagger-core#1656
@webron
Copy link
Contributor

webron commented Aug 19, 2016

It's pushed now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants