-
Notifications
You must be signed in to change notification settings - Fork 35
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
Issue311 improve hook searching #613
base: develop
Are you sure you want to change the base?
Conversation
Add the tests to cover new implementations lto cover HookHandlerSearch
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.
As discussed on the phone, you should also add some integration tests in the gateleen-test
module. Where is the documentation of this new API feature?
gateleen-hook/src/main/java/org/swisspush/gateleen/hook/HookHandler.java
Outdated
Show resolved
Hide resolved
gateleen-hook/src/main/java/org/swisspush/gateleen/hook/HookHandler.java
Outdated
Show resolved
Hide resolved
gateleen-hook/src/main/java/org/swisspush/gateleen/hook/HookHandler.java
Outdated
Show resolved
Hide resolved
gateleen-hook/src/test/java/org/swisspush/gateleen/hook/HookHandlerTest.java
Outdated
Show resolved
Hide resolved
gateleen-hook/src/main/java/org/swisspush/gateleen/hook/HookHandler.java
Outdated
Show resolved
Hide resolved
gateleen-hook/src/test/java/org/swisspush/gateleen/hook/HookHandlerTest.java
Outdated
Show resolved
Hide resolved
Add new tests for HookHandler Create testes with storage
- Verifies successful listener search when multiple listeners are present in storage. - Ensures correct retrieval of a single listener from storage. - Tests failure case when searching for a non-existent listener among multiple listeners. - Ensures proper handling of search when no listeners are registered in storage.
…istener is found for a given query parameter.
…eners registered. - Test for handling searches with no matching listeners, returning an empty list. - Test for handling searches when no listeners are registered, ensuring an empty list is returned. - Fix hookHandler Search
…tory. It checks if the destination of each item contains the query string (queryParam), adding matching items to the result.
- improve listeners tests
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #613 +/- ##
=============================================
+ Coverage 48.44% 48.87% +0.42%
- Complexity 1862 1892 +30
=============================================
Files 238 238
Lines 11953 12049 +96
Branches 1261 1278 +17
=============================================
+ Hits 5791 5889 +98
+ Misses 5659 5653 -6
- Partials 503 507 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
4fdab6c
to
4004646
Compare
|
||
``` | ||
GET http://myserver:7012/gateleen/server/listenertest/_hooks/listeners/listener/1?q=testQuery |
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.
This is not how listener hooks are searched. The URL does not contain the listener-ID.
```json | ||
{ | ||
"listeners": [ | ||
{ |
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.
The content of searching listeners and routes is not an array of objects (with destination, methods properties) but an array containing a list of names (strings). How did you even get this response?
private static final String CONTENT_TYPE_JSON = "application/json"; | ||
private static final String LISTENERS_KEY = "listeners"; | ||
private static final String ROUTES_KEY = "routes"; | ||
private static final String DESTINATION_KEY = "destination"; |
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.
Your newly created DESTINATION_KEY
static variable is not used
@@ -569,6 +577,22 @@ public boolean handle(final RoutingContext ctx) { | |||
} | |||
} | |||
|
|||
// 1. Check if the request method is GET | |||
if (request.method() == HttpMethod.GET) { | |||
String uri = request.uri(); |
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.
Extract var requestUri = request.uri();
only once in the handle
method. It is already done for PUT
and DELETE
requests. We should optimize this.
// 1. Check if the request method is GET | ||
if (request.method() == HttpMethod.GET) { | ||
String uri = request.uri(); | ||
String queryParam = request.getParam("q"); |
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.
Using any other query params than q
in GET
requests should result in a 400 Bad Request
and not just return all listeners/hooks. Also create tests to verify this.
|
||
// Register a route with a different query param | ||
String differentQueryParam = "differentQuery"; | ||
TestUtils.registerRoute(requestUrlBase + routePath + "?q=" + differentQueryParam, targetUrlBase, new String[]{"GET", "POST"}, null, true, true); |
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.
Query param in PUT request for route registration. This does not work
TestUtils.registerRoute(requestUrlBase + routePath + "?q=" + differentQueryParam, targetUrlBase, new String[]{"GET", "POST"}, null, true, true); | ||
|
||
// Send GET request with non-matching query param | ||
given().queryParam("q", nonMatchingQueryParam) |
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.
Invalid uri
.body("routes", Matchers.empty()); | ||
|
||
// Validate response | ||
checkGETStatusCodeWithAwait(requestUrl, 200); |
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.
Verify response
// No routes registered | ||
|
||
// Send GET request with a query param | ||
given().queryParam("q", queryParam) |
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.
invalid uri
.body("routes", Matchers.empty()); | ||
|
||
// Validate response | ||
checkGETStatusCodeWithAwait(requestUrl, 200); |
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.
Verify response
Documentation for the Hook Search API (Listeners and Routes)
Available Endpoints
Query Parameter
Both endpoints support the q query parameter, which is used to filter the search results.
Example Request:
GET /registrations/listeners?q=myDestination
GET /registrations/routes?q=myDestination
Response Structure
Example Response - Searching for Listeners:
Example Response - Searching for Routes:
Key Features
Separate Endpoints for Listeners and Routes: The API provides distinct endpoints for Listeners and Routes searches, ensuring optimized search results based on specific hook types.
String-Based Search: The API looks for matches in the destination field of both Listeners and Routes. It searches for any substring match rather than an exact match, making the search more flexible.
Optimized Search Logic: The API uses efficient search mechanisms internally to quickly find results across registered listeners or routes, reducing response time even with large datasets.
Example Workflow
A client wants to find all Listeners with a destination that contains the string "myDestination". They make the following request:
GET /registrations/listeners?q=myDestination
The API returns a JSON object containing the identifiers of matching Listeners: