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

Feature/webflux #2233

Merged
merged 15 commits into from
Aug 18, 2018
Merged

Feature/webflux #2233

merged 15 commits into from
Aug 18, 2018

Conversation

deblockt
Copy link
Contributor

@deblockt deblockt commented Feb 6, 2018

What's this PR do/fix?

This PR provide a webflux support.

Are there unit tests? If not how should this be manually tested?

Curently existing unit tests are failed. I don't understand why. I need help on this part.
I have not already add unit test for webflux support.

Any background context you want to provide?

I have remove spring dependency to springfox-core. This can create some breaking change for thrid part libs.

What are the relevant issues?

see #1773

@ghost ghost assigned dilipkrish Feb 28, 2018
@ghost ghost added the in progress label Feb 28, 2018
@dilipkrish dilipkrish added PR and removed in progress labels Feb 28, 2018
@dilipkrish
Copy link
Member

@deblockt Sorry its taken so long to get this PR, but Im getting ready to pull this in. Would it be possible to resolve the conflicts?

@neil1hart
Copy link

I can take a look as I want this to work. It depends on spring 5; can that PR be merged in first. There are several tests that fail so I may incrementally go through these changes.

@prafsoni
Copy link

Any updates on this?

@ligasgr
Copy link
Contributor

ligasgr commented Aug 4, 2018

Hi Team,
I've got a branch with changes from original branch from this PR plus additional fixes, merge with latest master and crude set of contract tests.
How should we proceed with merging this in?
Should I raise a PR in the repo of deblock (https://github.com/deblockt/springfox), get it merged in there and then have this PR updated or should I raise a separate PR in this repo?

@ligasgr
Copy link
Contributor

ligasgr commented Aug 4, 2018

FYI branch is https://github.com/ligasgr/springfox/tree/feature/webflux

@dilipkrish
Copy link
Member

@ligasgr that is awesome. You could create another PR that superscedes this one. FYI, One issue I had with this PR, that I would like to fix before pulling this in, is the duplication this introduces on the DocumentationBootstrapper. I think we should extract a chunk to spring-web and create a think shim for spring-web-mvc and spring-web-webflux, very similar to the thin shim we have for spring-data-rest

@ligasgr
Copy link
Contributor

ligasgr commented Aug 7, 2018

Thanks for your comments. I'll try to see how this could be achieved. Initial thoughts are:

  • Add some sort of factory for RelativePathProvider and have it injected intoDocumentationPluginsBootstrapper that will inject it into DefaultConfiguration to avoid passing around ServletContext (it would only be autowired into that new factory). The code of DefaultConfiguration and DocumentationPluginsBootstrapper could then be moved back into common place (web) along with most of the tests that were moved to webmvc project (these that don't use mvc specific classes).
  • Merge SpringfoxWebMvcConfiguration and SpringfoxWebFluxConfiguration as they are exactly the same and move into web project.

How does that sound to you?

@dilipkrish
Copy link
Member

dilipkrish commented Aug 8, 2018

That does sound good. Essentially we want only one DocumentationPluginsBootstrapper and not one for the webflux request handlers and one for webmvc. Thank you for doing this, when I last looked at it, it seemed a little bit work work, especially with regards to moving the tests etc.

@ligasgr ligasgr mentioned this pull request Aug 9, 2018
@dilipkrish dilipkrish merged commit 0344724 into springfox:master Aug 18, 2018
@dilipkrish dilipkrish added this to the 3.0 milestone Aug 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants