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

Query param route predicate - extension of QueryRoutePredicateFactory #3472

Open
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

polifr
Copy link

@polifr polifr commented Jul 23, 2024

I tried to generalize the QueryRoutePredicateFactory so that it could use a bean of type Predicate instead of being limited to a regexp. Test classes are also implemented.

polifr added 4 commits July 21, 2024 02:49
A predicate that checks if a query parameter value matches criteria of a
given predicate.
# Conflicts:
#	spring-cloud-gateway-server/src/main/java/org/springframework/cloud/gateway/route/builder/PredicateSpec.java
@spencergibb
Copy link
Member

I'd prefer this in one class as having multiple query predicates is confusing.

@polifr
Copy link
Author

polifr commented Sep 26, 2024

@spencergibb you are suggesting to merge the two classes into one single predicate factory, that can manage both cases, right? I can do it, paying attention not to introduce regressions on the QueryRoutePredicateFactory.

@spencergibb
Copy link
Member

@polifr yes, since this will only be available thru the java dsl, it should be ok. the regex can be retrofit to a Predicate.

@polifr
Copy link
Author

polifr commented Oct 3, 2024

@spencergibb I modified the QueryRoutePredicateFactory class to unify the regexp and predicate approach. Let me know if this is what you meant, so that I can proceed with adjustments on junit tests and cleaning of the previous proposal.

Copy link
Member

@spencergibb spencergibb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you can now remove QueryParamRoutePredicateFactory

@spencergibb
Copy link
Member

Yes, you can now remove QueryParamRoutePredicateFactory

@polifr
Copy link
Author

polifr commented Oct 15, 2024

Ok, I removed the QueryParamRoutePredicateFactory and QueryParamRoutePredicateFactoryTests and extend the current QueryRoutePredicateFactoryTests to cover the new options. I also fixed the checkstyle errors that made the build fail.

@polifr polifr requested a review from spencergibb November 5, 2024 08:10
@polifr
Copy link
Author

polifr commented Dec 23, 2024

Hi @spencergibb , is there something that stills blocks this pull request? I can't see any change request that has to be done.

@polifr
Copy link
Author

polifr commented Dec 23, 2024

There was a format error in the header of the test class, I fixed it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Todo
Development

Successfully merging this pull request may close these issues.

3 participants