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
Enhance route duplication detection by including param and header predicates #5469
Conversation
…dicates. Motivation: In route duplication detection, it's essential to consider param and header predicates alongside other route conditions. Modification: - Introduce `Route.hasDuplicateRouteCondition()` method to encapsulate route condition comparison logic previously handled in `Routers`. - Centralizing this logic improves maintainability and ensures consistency when adding more fields to `Route` class. Result: - Routes with differing predicates, including param and header predicates, are correctly identified as non-duplicates.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5469 +/- ##
============================================
- Coverage 74.13% 74.12% -0.02%
- Complexity 20991 20997 +6
============================================
Files 1818 1818
Lines 77233 77252 +19
Branches 9856 9858 +2
============================================
+ Hits 57256 57261 +5
- Misses 15309 15320 +11
- Partials 4668 4671 +3 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a minor suggestion, but looks good overall 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 👍
// No overlap in supported methods. | ||
return false; | ||
} | ||
if (!consumes.isEmpty() && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Q) If consumes
is empty, it can consume any media type. In the following situation, should we assume that the two consumes
are duplicate?
- this.consumes = [JSON]
- others.consumes = []
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point. I think that it was a bug, and we should treat them as the same.
I'll address this in a separate PR because it might introduce unexpected behavior changes.
// No overlap in consume types. | ||
return false; | ||
} | ||
if (!produces.isEmpty() && produces.stream().noneMatch( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a naming nit. 🙇
* assert route.hasDuplicateRouteCondition(other); | ||
* }</pre> | ||
*/ | ||
boolean hasDuplicateRouteCondition(Route other); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe hasConflicts()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍👍
Motivation:
In route duplication detection, it's essential to consider param and header predicates alongside other route conditions.
Modification:
Route.hasDuplicateRouteCondition()
method to encapsulate route condition comparison logic previously handled inRouters
.Route
class.Result: