Added duplication detection configuration and GeoUtils distance helper#52
Added duplication detection configuration and GeoUtils distance helper#52
Conversation
WalkthroughAdds configuration binding for duplicate-detection properties, a haversine geolocation utility, a repository query for recent reports, a DuplicateDetectionService to find nearby recent reports, integrates duplicate checks into ReportService, and adds tests plus default properties. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ReportService
participant DuplicateDetectionService
participant ReportRepository
participant Database
participant GeoUtils
participant Config as DuplicateProperties
Client->>ReportService: createReport(reportDto)
ReportService->>DuplicateDetectionService: findDuplicate(reportDto)
DuplicateDetectionService->>Config: read radius/timeWindow
DuplicateDetectionService->>ReportRepository: findAllByEventTypeAndStatusAndSubmittedAtAfter(...)
ReportRepository->>Database: query recent active reports
Database-->>ReportRepository: list of ReportEntity
DuplicateDetectionService->>GeoUtils: distanceInMeters(candidate, incoming)
GeoUtils-->>DuplicateDetectionService: distance (meters)
alt distance <= radius
DuplicateDetectionService-->>ReportService: Optional.of(matching ReportEntity)
ReportService-->>Client: throws IllegalArgumentException (duplicate)
else no match
DuplicateDetectionService-->>ReportService: Optional.empty()
ReportService->>ReportRepository: proceed to save new report
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
backend/src/main/java/org/fungover/zipp/util/GeoUtils.java (2)
9-23: Add input validation for coordinate bounds.The method accepts latitude and longitude values without validation. Invalid inputs (e.g., latitude outside -90 to 90, longitude outside -180 to 180, NaN, or infinity) could produce incorrect results or propagate errors.
Apply this diff to add input validation:
public static double distanceInMeters(double lat1, double lon1, double lat2, double lon2) { + if (Math.abs(lat1) > 90 || Math.abs(lat2) > 90) { + throw new IllegalArgumentException("Latitude must be between -90 and 90 degrees"); + } + if (Math.abs(lon1) > 180 || Math.abs(lon2) > 180) { + throw new IllegalArgumentException("Longitude must be between -180 and 180 degrees"); + } + if (!Double.isFinite(lat1) || !Double.isFinite(lon1) || !Double.isFinite(lat2) || !Double.isFinite(lon2)) { + throw new IllegalArgumentException("Coordinates must be finite numbers"); + } + double latDistance = Math.toRadians(lat2 - lat1); double lonDistance = Math.toRadians(lon2 - lon1);
9-23: Consider using an established geospatial library.While the haversine implementation is correct, geospatial calculations are complex and error-prone. Established libraries like GeoTools, Spatial4j, or JTS provide well-tested implementations with additional features (different distance algorithms, coordinate system transformations, etc.).
backend/src/main/java/org/fungover/zipp/config/DuplicateProperties.java (1)
5-8: Add validation constraints to prevent invalid configuration.The configuration properties lack validation constraints, which could allow negative or zero values for
radiusInMetersandtimeWindowInMinutes. Adding validation annotations ensures the application fails fast with a clear error message if misconfigured.Apply this diff to add validation:
package org.fungover.zipp.config; +import jakarta.validation.constraints.Positive; import org.springframework.boot.context.properties.ConfigurationProperties; +import org.springframework.validation.annotation.Validated; +@Validated @ConfigurationProperties(prefix = "duplicate") public class DuplicateProperties { + @Positive private final int radiusInMeters; + @Positive private final int timeWindowInMinutes;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/src/main/java/org/fungover/zipp/BackendApplication.java(1 hunks)backend/src/main/java/org/fungover/zipp/config/DuplicateProperties.java(1 hunks)backend/src/main/java/org/fungover/zipp/util/GeoUtils.java(1 hunks)backend/src/main/resources/application.properties(1 hunks)
🔇 Additional comments (5)
backend/src/main/resources/application.properties (1)
22-25: LGTM!The configuration properties are well-documented and follow Spring Boot naming conventions. The default values (50 meters radius, 10-minute window) are reasonable starting points for duplicate detection.
backend/src/main/java/org/fungover/zipp/util/GeoUtils.java (1)
3-7: LGTM!The utility class follows best practices with a private constructor and uses the correct WGS84 mean Earth radius value.
backend/src/main/java/org/fungover/zipp/BackendApplication.java (1)
3-3: LGTM!The configuration properties are properly enabled using
@EnableConfigurationProperties. This is the correct approach for registering theDuplicatePropertiesconfiguration class.Also applies to: 6-6, 9-9
backend/src/main/java/org/fungover/zipp/config/DuplicateProperties.java (2)
10-13: LGTM!The constructor correctly implements the constructor binding pattern for Spring Boot configuration properties.
15-21: LGTM!The getter methods correctly expose the configuration values.
jennymakki
left a comment
jennymakki
left a comment
There was a problem hiding this comment.
Code is clean, well-structured, and a good start for further development with the detection configuration.
SpinalGlitter
left a comment
SpinalGlitter
left a comment
There was a problem hiding this comment.
Very clean code and it seems to do that it suppose to do.
Tobias-hubs
left a comment
Tobias-hubs
left a comment
There was a problem hiding this comment.
The duplication detection setup looks good.
DuplicatePropertiescleanly exposes configurable radius and time window.GeoUtilsprovides a clear and reusable distance calculation.- Properties are well integrated into
application.properties.
No issues spotted — this PR is ready to merge.
0x0xoscar
dismissed stale reviews from Tobias-hubs, SpinalGlitter, and jennymakki
via
760c5a3
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
backend/src/main/resources/application.properties (1)
27-30: LGTM! Configuration properties follow Spring conventions.The duplicate detection configuration is well-structured with clear naming and reasonable default values. The 50-meter radius and 10-minute window seem appropriate for detecting duplicate reports in proximity.
Optional suggestion: Consider whether these defaults should be documented in a README or configuration guide to help operators understand when they might need to adjust these values for different use cases (urban vs. rural areas, high-traffic vs. low-traffic scenarios, etc.).
backend/src/main/java/org/fungover/zipp/BackendApplication.java (1)
4-4: Configuration properties binding is correctly implemented.The addition of
@EnableConfigurationProperties(DuplicateProperties.class)explicitly enables the configuration properties binding for the duplicate detection feature. The imports are correct and the annotation is properly placed.Note: In Spring Boot 3.x and later (you're using 4.0.0),
@ConfigurationPropertiesclasses are typically auto-detected when they're in a component-scanned package and annotated with@Componentor@ConfigurationProperties. While@EnableConfigurationPropertiesis not harmful and makes the configuration explicit, it may be redundant depending on howDuplicatePropertiesis structured. The explicit approach is valid and can improve clarity about which configuration classes are active.Also applies to: 7-7, 13-13
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/src/main/java/org/fungover/zipp/BackendApplication.java(1 hunks)backend/src/main/resources/application.properties(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: PR Build
- GitHub Check: Jenkins
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/src/main/java/org/fungover/zipp/config/DuplicateProperties.java(1 hunks)backend/src/main/java/org/fungover/zipp/util/GeoUtils.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/src/main/java/org/fungover/zipp/config/DuplicateProperties.java
🔇 Additional comments (1)
backend/src/main/java/org/fungover/zipp/util/GeoUtils.java (1)
3-8: Utility class structure looks goodStatic-only helper with a private constructor is appropriate here; fits the intended usage as a pure utility without state.
| public static double distanceInMeters(double lat1, double lon1, double lat2, double lon2) { | ||
| double latDistance = Math.toRadians(lat2 - lat1); | ||
| double lonDistance = Math.toRadians(lon2 - lon1); | ||
|
|
||
| double a = Math.sin(latDistance / 2) * Math.sin(latDistance / 2) + Math.cos(Math.toRadians(lat1)) | ||
| * Math.cos(Math.toRadians(lat2)) * Math.sin(lonDistance / 2) * Math.sin(lonDistance / 2); | ||
|
|
||
| double c = 2 * Math.atan2(Math.sqrt(a), Math.sqrt(1 - a)); | ||
|
|
||
| return EARTH_RADIUS * c; |
There was a problem hiding this comment.
Clamp haversine intermediate to avoid rare NaNs and consider documenting units
The haversine implementation is correct, but a can be marginally > 1 due to floating‑point rounding for near‑antipodal points, making Math.sqrt(1 - a) NaN. Clamping a before computing c makes this more robust, and a short Javadoc noting that inputs are in degrees would help avoid misuse.
A minimal change:
- double a = Math.sin(latDistance / 2) * Math.sin(latDistance / 2) + Math.cos(Math.toRadians(lat1))
- * Math.cos(Math.toRadians(lat2)) * Math.sin(lonDistance / 2) * Math.sin(lonDistance / 2);
-
- double c = 2 * Math.atan2(Math.sqrt(a), Math.sqrt(1 - a));
+ double a = Math.sin(latDistance / 2) * Math.sin(latDistance / 2)
+ + Math.cos(Math.toRadians(lat1)) * Math.cos(Math.toRadians(lat2))
+ * Math.sin(lonDistance / 2) * Math.sin(lonDistance / 2);
+
+ // Guard against slight floating-point overshoot of 1.0
+ double aClamped = Math.min(1.0, Math.max(0.0, a));
+ double c = 2 * Math.atan2(Math.sqrt(aClamped), Math.sqrt(1 - aClamped));Optionally, add a brief Javadoc like:
/**
* Returns the great-circle distance between two points specified in decimal degrees.
*/🤖 Prompt for AI Agents
In backend/src/main/java/org/fungover/zipp/util/GeoUtils.java around lines 10 to
19, the haversine intermediate variable `a` can slightly exceed 1 due to
floating-point rounding and make Math.sqrt(1 - a) NaN; clamp `a` into the [0.0,
1.0] range before computing `c` (e.g., a = Math.min(1.0, Math.max(0.0, a))).
Also add a brief Javadoc to the method stating that input lat/lon are in decimal
degrees and the returned value is meters.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
backend/src/main/java/org/fungover/zipp/util/GeoUtils.java (1)
10-19: Address the previously-raised concerns about floating-point edge case and missing documentation.The concerns from the previous review remain unaddressed:
Floating-point edge case: The variable
acan marginally exceed 1.0 due to floating-point rounding for near-antipodal points, causingMath.sqrt(1 - a)to return NaN. Clampingato the range [0.0, 1.0] would prevent this.Missing documentation: The method lacks Javadoc specifying that inputs are in decimal degrees and the return value is in meters.
Apply this diff to address both concerns:
+ /** + * Calculates the great-circle distance between two geographic points using the Haversine formula. + * + * @param lat1 latitude of the first point in decimal degrees + * @param lon1 longitude of the first point in decimal degrees + * @param lat2 latitude of the second point in decimal degrees + * @param lon2 longitude of the second point in decimal degrees + * @return the distance between the two points in meters + */ public static double distanceInMeters(double lat1, double lon1, double lat2, double lon2) { double latDistance = Math.toRadians(lat2 - lat1); double lonDistance = Math.toRadians(lon2 - lon1); double a = Math.sin(latDistance / 2) * Math.sin(latDistance / 2) + Math.cos(Math.toRadians(lat1)) * Math.cos(Math.toRadians(lat2)) * Math.sin(lonDistance / 2) * Math.sin(lonDistance / 2); - double c = 2 * Math.atan2(Math.sqrt(a), Math.sqrt(1 - a)); + // Clamp a to [0.0, 1.0] to guard against floating-point rounding errors + double aClamped = Math.min(1.0, Math.max(0.0, a)); + double c = 2 * Math.atan2(Math.sqrt(aClamped), Math.sqrt(1 - aClamped)); return EARTH_RADIUS * c; }
🧹 Nitpick comments (1)
backend/src/main/java/org/fungover/zipp/util/GeoUtils.java (1)
4-5: Consider clarifying the comment about WGS84.While 6,371,000 meters is a reasonable mean Earth radius for haversine calculations, WGS84 actually defines Earth as an ellipsoid rather than a sphere. The comment could be more precise to avoid confusion.
Consider this wording:
- // Mean Earth radius in meters according to WGS84 standard + // Mean Earth radius in meters for spherical approximation
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/src/main/java/org/fungover/zipp/util/GeoUtils.java(1 hunks)
🔇 Additional comments (1)
backend/src/main/java/org/fungover/zipp/util/GeoUtils.java (1)
3-8: LGTM! Standard utility class pattern.The use of
finalclass and private constructor correctly prevents instantiation and subclassing, following Java best practices for utility classes.
…ubmitted. Also added the DuplicateDetectionService
Implemented duplicate report detection and integration
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
backend/src/main/java/org/fungover/zipp/repository/ReportRepository.java (1)
19-20: New finder method fits Spring Data conventions; consider future pagination if volume growsThe derived query name and parameters align with the entity fields and the intended “recent active reports of a given type” use case. If this endpoint ever needs to scan a large number of rows (e.g., in high-traffic deployments), consider adding a
Pageable/Slicevariant later to cap result size, though for a short time window this is likely fine for now.backend/src/test/java/org/fungover/zipp/service/DuplicateDetectionServiceTest.java (1)
32-93: Good coverage of spatial logic; consider adding time-window and empty-result casesThese tests nicely cover the “within radius” vs “outside radius” paths and verify repository interaction. To fully exercise the service’s behavior, consider:
- A case where the repository returns an empty list (no candidates in the time window).
- A case that asserts the computed cutoff is actually used (e.g., by having a fake repository or argument captor that differentiates inside/outside the time window).
You could also factor out the common setup (mock repository, properties, service, baseReport) into a helper or@BeforeEachto reduce duplication, but that’s optional.backend/src/main/java/org/fungover/zipp/service/ReportService.java (1)
27-42: Duplicate detection integration looks correct; consider a domain-specific exceptionInjecting
DuplicateDetectionServiceand checking for duplicates before persisting is a good place in the flow and uses the DTO’s coordinates consistently with the rest of the service. If clients need to distinguish “duplicate report” from other validation errors, consider introducing a dedicated exception (e.g.,DuplicateReportException) or an error code so the controller layer can map it to a specific HTTP status/response body, instead of a genericIllegalArgumentException.backend/src/main/java/org/fungover/zipp/service/DuplicateDetectionService.java (1)
21-42: Distance + time-window logic looks solid; only minor testability/config tweaks to considerThe overall flow (derive cutoff from
submittedAt/now, query recent ACTIVE reports of same type, then filter by haversine distance using JTS coordinates) is coherent and matches the described behavior; the lat/lon vsPoint.getY()/getX()usage is consistent with how points are created elsewhere. For future robustness you might:
- Inject a
Clockinstead of callingInstant.now()directly, so the “no submittedAt” path can be tested deterministically.- Optionally validate or document constraints on
radiusInMetersandtimeWindowInMinutes(non-negative, non-zero) to avoid surprising behavior from misconfiguration.
These are nice-to-haves; the current implementation is functionally sound.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/src/main/java/org/fungover/zipp/repository/ReportRepository.java(2 hunks)backend/src/main/java/org/fungover/zipp/service/DuplicateDetectionService.java(1 hunks)backend/src/main/java/org/fungover/zipp/service/ReportService.java(1 hunks)backend/src/main/resources/application.properties(1 hunks)backend/src/test/java/org/fungover/zipp/service/DuplicateDetectionServiceTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/src/main/resources/application.properties
🧰 Additional context used
🧬 Code graph analysis (1)
backend/src/main/java/org/fungover/zipp/service/DuplicateDetectionService.java (2)
backend/src/main/java/org/fungover/zipp/util/GeoUtils.java (1)
GeoUtils(3-22)backend/src/main/java/org/fungover/zipp/service/ReportService.java (1)
Service(20-93)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build, Test & Analyze
|
Jenkins Build #5 Summary (for PR #52)
Details:
Error Logs (truncated): |
alfredbrannare
left a comment
alfredbrannare
left a comment
There was a problem hiding this comment.
Seems logical,
Nice job!
7 participants
This PR includes the initial setup for the duplicate report detection feature.
Added
These components will be used in later PRs to implement the repository query, duplicate detection service and integration with the report creation.
Summary by CodeRabbit
New Features
Improvements
Configuration
Tests
✏️ Tip: You can customize this high-level summary in your review settings.