- Notifications
You must be signed in to change notification settings - Fork6
Add geometry filter for Event API#195
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
coderabbitaibot commentedJun 17, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
WalkthroughA new enum, Changes
Sequence Diagram(s)sequenceDiagram participant Client participant Controller participant Service participant ApiClient participant EventAPI Client->>Controller: GET /events?geometryFilterType=ALL Controller->>Service: getEvents(feed, bbox, geometryFilterType) Service->>ApiClient: getEvents(feed, after, bbox, pageSize, sortOrder, geometryFilterType) ApiClient->>EventAPI: GET /api/events?geometryFilterType=ALL EventAPI-->>ApiClient: Event data ApiClient-->>Service: Event data Service-->>Controller: Event data Controller-->>Client: Event dataPoem
✨ Finishing Touches
🧪 Generate Unit Tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat withCodeRabbit:
SupportNeed help? Create a ticket on oursupport page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File ( |
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.
Actionable comments posted: 7
🔭 Outside diff range comments (1)
src/main/java/io/kontur/disasterninja/client/EventApiClient.java (1)
104-118:geometryFilterTypealready defaulted – duplicateObjects.requireNonNullElse
Objects.requireNonNullElseis repeated in bothgetEventsandgetEvent. Extract the defaulting logic to a private helper or reuse the suggestion above to avoid duplication.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (6)
src/main/java/io/kontur/disasterninja/client/EventApiClient.java(4 hunks)src/main/java/io/kontur/disasterninja/controller/EventsController.java(3 hunks)src/main/java/io/kontur/disasterninja/dto/GeometryFilterType.java(1 hunks)src/main/java/io/kontur/disasterninja/service/EventApiService.java(3 hunks)src/test/java/io/kontur/disasterninja/client/EventApiClientTest.java(7 hunks)src/test/java/io/kontur/disasterninja/service/EventApiServiceTest.java(9 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (3)
src/test/java/io/kontur/disasterninja/service/EventApiServiceTest.java (1)
125-129:Tests correctly updated for new parameterMocks now include the
GeometryFilterType.ALLargument ensuring the new API surface is fully covered.src/test/java/io/kontur/disasterninja/client/EventApiClientTest.java (1)
50-55:URI assertions cover new query parameterRegex expectations have been amended to assert
geometryFilterType=ALL; this safeguards against regressions.src/main/java/io/kontur/disasterninja/service/EventApiService.java (1)
40-42:New overload keeps existing callers intact – well done
Delegating to the 3-arg variant withGeometryFilterType.ALLpreserves backward compatibility and makes the change non-breaking.
| publicenumGeometryFilterType { | ||
| ALL, | ||
| NONE | ||
| } |
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.
🧹 Nitpick (assertive)
Add minimal Javadoc for public enum
Public enums form part of the external contract. Two-line Javadoc describing the intent of the enum and each value will greatly aid API consumers and generate clearer OpenAPI docs.
🤖 Prompt for AI Agents
In src/main/java/io/kontur/disasterninja/dto/GeometryFilterType.java aroundlines 3 to 6, add minimal Javadoc comments for the public enum and its values.Include a brief two-line description above the enum explaining its purpose, andadd a short comment for each enum constant describing what ALL and NONErepresent. This will improve API clarity and enhance generated OpenAPIdocumentation.| publicOptional<EventApiSearchEventResponse>getEvents(StringeventApiFeed,OffsetDateTimeafter, | ||
| List<BigDecimal>bbox,intpageSize, | ||
| SortOrdersortOrder,GeometryFilterTypegeometryFilterType) { | ||
| UriComponentsBuilderuriBuilder =UriComponentsBuilder.fromUriString(EVENT_API_EVENT_LIST_URI); | ||
| uriBuilder.queryParam("episodeFilterType","NONE"); | ||
| uriBuilder.queryParam("sortOrder",Objects.requireNonNullElse(sortOrder,"ASC")); | ||
| uriBuilder.queryParam("geometryFilterType", | ||
| Objects.requireNonNullElse(geometryFilterType,GeometryFilterType.ALL)); | ||
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.
🧹 Nitpick (assertive)
Consider null-safe defaulting outside the URI builder
Defaulting parameters up-front (before calling the URI builder) keeps the template construction cleaner and avoids repeatingObjects.requireNonNullElse. E.g.:
SortOrdereffectiveSort =sortOrder ==null ?SortOrder.ASC :sortOrder;GeometryFilterTypeeffectiveGeo =geometryFilterType ==null ?GeometryFilterType.ALL :geometryFilterType;uriBuilder.queryParam("sortOrder",effectiveSort) .queryParam("geometryFilterType",effectiveGeo);
Reduces cognitive load and improves readability.
🤖 Prompt for AI Agents
In src/main/java/io/kontur/disasterninja/client/EventApiClient.java around lines51 to 59, the code uses Objects.requireNonNullElse directly inside the URIbuilder queryParam calls for sortOrder and geometryFilterType. To improvereadability and reduce repetition, assign default values to local variables forsortOrder and geometryFilterType before building the URI, then use thesevariables in the queryParam calls. This makes the URI construction cleaner andeasier to understand.| uriBuilder.queryParam("episodeFilterType","NONE"); | ||
| uriBuilder.queryParam("sortOrder",Objects.requireNonNullElse(sortOrder,"ASC")); | ||
| uriBuilder.queryParam("geometryFilterType", |
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.
Compile-time type mismatch insortOrder default
Objects.requireNonNullElse(sortOrder, "ASC") mixes theSortOrder enum with aString, which will not compile – the second argument must be of the same type as the first.
-uriBuilder.queryParam("sortOrder", Objects.requireNonNullElse(sortOrder, "ASC"));+uriBuilder.queryParam("sortOrder",+ Objects.requireNonNullElse(sortOrder, SortOrder.ASC));
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| uriBuilder.queryParam("episodeFilterType","NONE"); | |
| uriBuilder.queryParam("sortOrder",Objects.requireNonNullElse(sortOrder,"ASC")); | |
| uriBuilder.queryParam("geometryFilterType", | |
| uriBuilder.queryParam("episodeFilterType","NONE"); | |
| uriBuilder.queryParam("sortOrder", | |
| Objects.requireNonNullElse(sortOrder,SortOrder.ASC)); | |
| uriBuilder.queryParam("geometryFilterType", |
🤖 Prompt for AI Agents
In src/main/java/io/kontur/disasterninja/client/EventApiClient.java around lines55 to 57, the call to Objects.requireNonNullElse uses a String "ASC" as thedefault for sortOrder, which is an enum type. Replace the String "ASC" with thecorresponding SortOrder enum constant (e.g., SortOrder.ASC) to ensure botharguments are of the same enum type and fix the compile-time type mismatch.| List<BigDecimal>bbox, | ||
| @Parameter(description ="Geometry filter type",example ="ALL") | ||
| @RequestParam(defaultValue ="ALL") | ||
| GeometryFilterTypegeometryFilterType | ||
| ) { | ||
| List<EventListDto>events =service.getEvents(feed,bbox); | ||
| List<EventListDto>events =service.getEvents(feed,bbox,geometryFilterType); | ||
| if (offset >=events.size()) { |
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.
🧹 Nitpick (assertive)
Delegate paginationafter filtering in service layer
service.getEvents(feed, bbox, geometryFilterType) returns the full list which is then paginated in the controller.
Pullinglimit &offset into the service call would prevent unbounded list creation and potential OOM scenarios on feeds with large result sets.
Not a blocker for this PR but worth refactoring soon.
🤖 Prompt for AI Agents
In src/main/java/io/kontur/disasterninja/controller/EventsController.java aroundlines 60 to 66, the current code fetches the full event list fromservice.getEvents before applying pagination in the controller, which risks highmemory usage. Refactor by adding limit and offset parameters to theservice.getEvents method and move pagination logic into the service layer tofetch only the required subset of events, preventing unbounded list creation andpotential out-of-memory errors.| publicEventDtogetEvent(UUIDeventId,Stringfeed,GeometryFilterTypegeometryFilterType) { | ||
| EventApiEventDtoevent =client.getEvent(eventId,feed,false,geometryFilterType); | ||
| if (event ==null) { |
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.
🛠️ Refactor suggestion
Duplicate null-safety concern for single-event retrieval
Apply the samenull check (orObjects.requireNonNull) here to prevent accidental NPEs and ensure consistent defaulting.
🤖 Prompt for AI Agents
In src/main/java/io/kontur/disasterninja/service/EventApiService.java aroundlines 80 to 82, add a null check or use Objects.requireNonNull on the result ofclient.getEvent to prevent potential NullPointerExceptions when retrieving asingle event. This will ensure consistent null-safety handling similar to otherparts of the code.| publicList<EventEpisodeListDto>getEventEpisodes(UUIDeventId,Stringfeed) { | ||
| EventApiEventDtoevent =client.getEvent(eventId,feed,true); | ||
| EventApiEventDtoevent =client.getEvent(eventId,feed,true,GeometryFilterType.ALL); | ||
| if (event ==null) { |
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.
🧹 Nitpick (assertive)
Expose geometry filtering for episode retrieval for API symmetrygetEventEpisodes hard-codesGeometryFilterType.ALL. If consumers may want to elide geometry, consider an overload mirroring the pattern used elsewhere:
publicList<EventEpisodeListDto>getEventEpisodes(UUIDeventId,Stringfeed,GeometryFilterTypegeometryFilterType) {returngetEventEpisodes(eventId,feed,geometryFilterType ==null ?GeometryFilterType.ALL :geometryFilterType);}
Keeps the current signature as a delegating helper, maintains backward compatibility, and provides full control to callers.
🤖 Prompt for AI Agents
In src/main/java/io/kontur/disasterninja/service/EventApiService.java aroundlines 88 to 90, the method getEventEpisodes currently hardcodesGeometryFilterType.ALL, limiting caller control. To fix this, add an overloadedgetEventEpisodes method that accepts a GeometryFilterType parameter, defaultingto ALL if null, and have the existing method delegate to this new overload. Thismaintains backward compatibility while exposing geometry filtering options tocallers.| publicList<EventListDto>getEvents(Stringfeed,List<BigDecimal>bbox,GeometryFilterTypegeometryFilterType) { | ||
| OffsetDateTimeafter =OffsetDateTime.now().minusDays(4).truncatedTo(ChronoUnit.HOURS); | ||
| Optional<EventApiClient.EventApiSearchEventResponse>eventsResponse =client.getEvents(feed,after,bbox, | ||
| pageSize,EventApiClient.SortOrder.ASC); | ||
| pageSize,EventApiClient.SortOrder.ASC,geometryFilterType); | ||
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.
Add a null-safety guard forgeometryFilterType
External code can directly invoke this overload and passnull. Whennull reachesEventApiClient, query-string construction will either blow up or emitgeometryFilterType=null. Guard early:
+ if (geometryFilterType == null) {+ geometryFilterType = GeometryFilterType.ALL;+ }
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| publicList<EventListDto>getEvents(Stringfeed,List<BigDecimal>bbox,GeometryFilterTypegeometryFilterType) { | |
| OffsetDateTimeafter =OffsetDateTime.now().minusDays(4).truncatedTo(ChronoUnit.HOURS); | |
| Optional<EventApiClient.EventApiSearchEventResponse>eventsResponse =client.getEvents(feed,after,bbox, | |
| pageSize,EventApiClient.SortOrder.ASC); | |
| pageSize,EventApiClient.SortOrder.ASC,geometryFilterType); | |
| publicList<EventListDto>getEvents(Stringfeed,List<BigDecimal>bbox,GeometryFilterTypegeometryFilterType) { | |
| if (geometryFilterType ==null) { | |
| geometryFilterType =GeometryFilterType.ALL; | |
| } | |
| OffsetDateTimeafter =OffsetDateTime.now().minusDays(4).truncatedTo(ChronoUnit.HOURS); | |
| Optional<EventApiClient.EventApiSearchEventResponse>eventsResponse =client.getEvents(feed,after,bbox, | |
| pageSize,EventApiClient.SortOrder.ASC,geometryFilterType); | |
| // ... | |
| } |
🤖 Prompt for AI Agents
In src/main/java/io/kontur/disasterninja/service/EventApiService.java aroundlines 43 to 47, add a null check for the geometryFilterType parameter at thestart of the getEvents method. If geometryFilterType is null, assign it adefault non-null value before passing it to the client.getEvents call to preventnull pointer exceptions or invalid query-string construction.
Uh oh!
There was an error while loading.Please reload this page.
Summary
GeometryFilterTypeenumTesting
./gradlew test(fails: Unsupported class file major version 65)https://chatgpt.com/codex/tasks/task_e_6851b525209c8324bc9b59ca253202a4
Summary by CodeRabbit
New Features
Bug Fixes