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

Add RequestPredicate Visitor to WebFlux.fn [SPR-17322] #21856

Closed
spring-issuemaster opened this Issue Oct 2, 2018 · 8 comments

Comments

Projects
None yet
2 participants
@spring-issuemaster
Copy link
Collaborator

spring-issuemaster commented Oct 2, 2018

Arjen Poutsma opened SPR-17322 and commented

Next to the RouterFunctions.Visitor, we should have a RequestPredicates.Visitor, which allows one to iterate over the matching predicates.


Referenced from: commits b1b28d4, 88cb126

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

spring-issuemaster commented Oct 12, 2018

Arjen Poutsma commented

I've created a prototype for this, and I would like to make sure that it's adequate before I commit it to master.

Sébastien Deleuze, could you take a look at this branch, and see if it does everything you need? The changes are here: poutsma@c6ed61d
As you can see, I reimplemented the ToStringVisitor to use the new predicate visitor, instead of relying on the predicate's toString.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

spring-issuemaster commented Oct 15, 2018

Sébastien Deleuze commented

Hey, sorry I was not available to answer on time before 5.1.1 since I was traveling, but I like the approach you took and I think it fits the need.
Since we have some time before 5.1.2, I am going to ask on Spring Fu side if we can draft something that would use your branch to give you more concrete feedback before 5.1.2.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

spring-issuemaster commented Oct 20, 2018

tgirard12 commented

This new predicate is very useful when we want to know all the RequestPredicate for all the routes. We can use it for automatic documentation for exemple.

I used both Router and Request visitor to get all the informations I want.

When I play with it in Kotlin with the RouterFunctionDsl, I could not used the visitor only for one route because  GET("/foo") { ok() } doesn't return the PathPatternPredicate but Unit.

 

 

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

spring-issuemaster commented Oct 23, 2018

Sébastien Deleuze commented

tgirard12 Do you think there is something I should fix in RouterFunctionDsl to make it work?

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

spring-issuemaster commented Oct 24, 2018

tgirard12 commented

It's work all ready, but the syntax is not very clean : (GET("/bar").apply { accept(visitor) }) { handle(it) }.
GET("/foo", ::handle) could return the RequestPredicate but in this case, we could add predicates after invoking the ::handle function. I think it's not a good thing.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

spring-issuemaster commented Oct 25, 2018

Sébastien Deleuze commented

Arjen Poutsma Do you think there could be a way to configure in more centralized and less intrusive fashion the request predicates, in order to be able to generate the documentation without modifying user routes?

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

spring-issuemaster commented Nov 20, 2018

Sébastien Deleuze commented

tgirard12 Unless I am mistaken, It seems already possible to add a visitor in a non intrusive way (which is the original goal of this issue) with the Kotlin DSL, since it generates a RouterFunction without modifying user routes, like it is with Java DSL with a visitor that implement both RouterFunctions.Visitor and RequestPredicates.Visitor, see this test and this visitor.

So I am ok for inclusion in 5.1.3.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

spring-issuemaster commented Nov 20, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.