Commcare 2.57 Backmerge #1482
Commcare 2.57 Backmerge #1482
Conversation
WalkthroughCoordinate validation logic was added to polygon utility methods to ensure all latitude and longitude values are within valid geographic ranges. The methods now throw Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant PolygonUtils
Caller->>PolygonUtils: createPolygon(coordinates)
PolygonUtils->>PolygonUtils: isValidCoordinates(lat, lon) for each point
alt Invalid coordinate
PolygonUtils-->>Caller: throw IllegalArgumentException
else All valid
PolygonUtils-->>Caller: return Polygon
end
Caller->>PolygonUtils: isPointInsideOrOnPolygon(point, polygon)
PolygonUtils->>PolygonUtils: isValidCoordinates(point.lat, point.lon)
alt Invalid coordinate
PolygonUtils-->>Caller: throw IllegalArgumentException
else Valid
PolygonUtils-->>Caller: boolean result
end
Caller->>PolygonUtils: getClosestPointOnPolygon(point, polygon)
PolygonUtils->>PolygonUtils: isValidCoordinates(point.lat, point.lon)
alt Invalid coordinate
PolygonUtils-->>Caller: throw IllegalArgumentException
else Valid
PolygonUtils-->>Caller: closest point string
end
Possibly related PRs
Suggested labels
Poem
✨ 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 with CodeRabbit:
SupportNeed help? Create a ticket on our support 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 (
|
![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 (4)
src/main/java/org/javarosa/core/model/utils/PolygonUtils.java (3)
37-38: Validation should happen before mutating stateYou add the
Coordinateto the array and only then validate it.
If validation fails, the partially-filledcoordinatesarray is left in an inconsistent state until the exception unwinds.
It’s safer (and fractionally cheaper) to validate the raw lat/lon doubles before constructing / storing theCoordinate.-coordinates[i] = new Coordinate(longitude, latitude); // JTS uses x=longitude, y=latitude -isValidCoordinates(coordinates[i].getY(), coordinates[i].getX()); +isValidCoordinates(latitude, longitude); +coordinates[i] = new Coordinate(longitude, latitude); // JTS uses x=longitude, y=latitude
54-58: Method name implies a boolean result but returnsvoid
isValidCoordinates(...)suggests a predicate returningtrue/false.
Since the method throws on failure and has no return value, a clearer name such asvalidateCoordinates(..)(orassertValidCoordinates) prevents confusion.While you’re here, consider including the offending values in the exception message to aid debugging.
-private static void isValidCoordinates(double latitude, double longitude) { - if ((latitude < -90.0 || latitude > 90.0) || (longitude < -180.0 || longitude > 180.0)) { - throw new IllegalArgumentException("Invalid polygon coordinates"); - } -} +private static void validateCoordinates(double latitude, double longitude) { + if ((latitude < -90.0 || latitude > 90.0) || (longitude < -180.0 || longitude > 180.0)) { + throw new IllegalArgumentException( + String.format("Invalid coordinates: lat=%f lon=%f (valid lat ∈ [-90,90], lon ∈ [-180,180])", + latitude, longitude)); + } +}Note: remember to update the three call sites.
67-70: RepeatedGeometryFactoryinstantiationBoth
isPointInsideOrOnPolygonandgetClosestPointOnPolygoncreate a newGeometryFactoryon every call.
GeometryFactoryis inexpensive, but it’s also stateless; a singleprivate static final GeometryFactory GF = new GeometryFactory();reused everywhere avoids needless allocations.Also applies to: 82-85
src/test/java/org/javarosa/xpath/test/XPathEvalTest.java (1)
684-704: Tests are valuable but highly repetitive – consider parameterisationThe six new negative-case assertions differ only in their input strings.
JUnit 4 doesn’t have native parameterised tests, but@RunWith(Parameterized.class)or JUnit 5’s@ParameterizedTestwould let you express the same coverage with a single data-driven method, reducing boiler-plate and making future additions easier.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/org/javarosa/core/model/utils/PolygonUtils.java(3 hunks)src/test/java/org/javarosa/xpath/test/XPathEvalTest.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: task-list-completed
- GitHub Check: build
Backmerge 2.57
Summary by CodeRabbit
New Features
Bug Fixes
Tests