Xpath maps function update #1485
Conversation
… into master_connectid_merge # Conflicts: # app/AndroidManifest.xml # app/build.gradle # app/res/layout-land/home_screen.xml # app/res/layout/fragment_phone_available_bottom_sheet.xml # app/res/layout/fragment_recovery_code.xml # app/res/layout/fragment_secondary_phone_number.xml # app/res/layout/fragment_signup.xml # app/res/layout/home_screen.xml # app/res/layout/screen_connect_message.xml # app/res/layout/screen_connect_password_verify.xml # app/res/layout/screen_connect_phone_verify.xml # app/res/layout/screen_connect_user_deactivate_otp_verify.xml # app/res/layout/screen_connect_verify.xml # app/res/layout/screen_login.xml # app/res/layout/select_install_mode_fragment.xml # app/res/navigation/nav_graph_connectid.xml # app/res/values/colors.xml # app/res/values/strings.xml # app/res/values/themes.xml # app/src/org/commcare/activities/CommCareSetupActivity.java # app/src/org/commcare/activities/DispatchActivity.java # app/src/org/commcare/activities/HomeButtons.java # app/src/org/commcare/activities/HomeScreenBaseActivity.java # app/src/org/commcare/activities/LoginActivity.java # app/src/org/commcare/activities/LoginActivityUiController.java # app/src/org/commcare/activities/SettingsHelper.java # app/src/org/commcare/activities/StandardHomeActivity.java # app/src/org/commcare/activities/StandardHomeActivityUIController.java # app/src/org/commcare/activities/connect/ConnectIdActivity.java # app/src/org/commcare/android/database/connect/models/ConnectJobRecord.java # app/src/org/commcare/android/database/connect/models/ConnectLinkedAppRecord.java # app/src/org/commcare/android/database/connect/models/ConnectUserRecord.java # app/src/org/commcare/connect/ConnectConstants.java # app/src/org/commcare/connect/SMSBroadcastReceiver.java # app/src/org/commcare/connect/database/ConnectAppDatabaseUtil.java # app/src/org/commcare/connect/database/ConnectJobUtils.java # app/src/org/commcare/connect/database/ConnectUserDatabaseUtil.java # app/src/org/commcare/connect/network/ApiConnect.java # app/src/org/commcare/connect/network/ApiConnectId.java # app/src/org/commcare/connect/network/ConnectNetworkHelper.java # app/src/org/commcare/connect/network/ConnectNetworkServiceFactory.kt # app/src/org/commcare/connect/network/ConnectSsoHelper.java # app/src/org/commcare/connect/workers/ConnectHeartbeatWorker.kt # app/src/org/commcare/fragments/SelectInstallModeFragment.java # app/src/org/commcare/fragments/connectId/ConnectIDSecondaryPhoneNumber.java # app/src/org/commcare/fragments/connectId/ConnectIDSignupFragment.java # app/src/org/commcare/fragments/connectId/ConnectIdBiometricConfigFragment.java # app/src/org/commcare/fragments/connectId/ConnectIdMessageFragment.java # app/src/org/commcare/fragments/connectId/ConnectIdPasswordVerificationFragment.java # app/src/org/commcare/fragments/connectId/ConnectIdPhoneAvailableBottomSheet.java # app/src/org/commcare/fragments/connectId/ConnectIdPinFragment.java # app/src/org/commcare/fragments/connectId/ConnectIdUserDeactivateOTPVerificationFragment.java # app/src/org/commcare/google/services/analytics/AnalyticsParamValue.java # app/src/org/commcare/google/services/analytics/CCAnalyticsEvent.java # app/src/org/commcare/google/services/analytics/CCAnalyticsParam.java # app/src/org/commcare/google/services/analytics/FirebaseAnalyticsUtil.java # app/src/org/commcare/network/CommcareRequestGenerator.java # app/src/org/commcare/network/HttpUtils.java # app/src/org/commcare/preferences/AppManagerDeveloperPreferences.java # app/src/org/commcare/utils/BiometricsHelper.java # app/src/org/commcare/utils/CrashUtil.java # app/src/org/commcare/utils/EncryptionKeyAndTransform.java # app/src/org/commcare/utils/KeyboardHelper.java # app/src/org/commcare/utils/PhoneNumberHelper.java # build.gradle
WalkthroughThis update replaces the JTS geometry library with the geodesy library for polygon operations, refactors related utility and XPath functions to use geodesic-aware computations, updates test cases for precision and coverage, and introduces Gradle wrapper scripts and configuration for build management. Minor documentation and utility enhancements are also included. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant PolygonUtils
participant GeodesyLib
Caller->>PolygonUtils: createPolygon(latLngList)
PolygonUtils->>GeodesyLib: Validate and convert to GlobalCoordinates
PolygonUtils-->>Caller: List<GlobalCoordinates>
Caller->>PolygonUtils: isPointInsideOrOnPolygon(point, polygon)
PolygonUtils->>GeodesyLib: Ray casting/geodesic checks
PolygonUtils-->>Caller: Boolean
Caller->>PolygonUtils: findClosestPoint(point, polygon)
PolygonUtils->>GeodesyLib: Compute closest geodesic point
PolygonUtils-->>Caller: Closest point as string
Possibly related PRs
Suggested labels
Suggested reviewers
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: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
gradle/wrapper/gradle-wrapper.jaris excluded by!**/*.jar
📒 Files selected for processing (11)
build.gradle(1 hunks)gradle/wrapper/gradle-wrapper.properties(1 hunks)gradlew(1 hunks)gradlew.bat(1 hunks)src/main/java/org/javarosa/core/model/utils/PolygonUtils.java(1 hunks)src/main/java/org/javarosa/core/services/storage/IStorageUtilityIndexed.java(1 hunks)src/main/java/org/javarosa/core/services/storage/util/DummyIndexedStorageUtility.java(2 hunks)src/main/java/org/javarosa/xpath/expr/XPathClosestPointOnPolygonFunc.java(3 hunks)src/main/java/org/javarosa/xpath/expr/XPathIsPointInsidePolygonFunc.java(3 hunks)src/test/java/org/javarosa/core/storage/IndexedStorageUtilityTests.java(2 hunks)src/test/java/org/javarosa/xpath/test/XPathEvalTest.java(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: task-list-completed
- GitHub Check: build
- GitHub Check: task-list-completed
🔇 Additional comments (17)
gradle/wrapper/gradle-wrapper.properties (1)
1-6: LGTM! Standard Gradle wrapper configuration.The Gradle wrapper properties file follows standard conventions and uses a stable Gradle version (7.5.1). This will ensure consistent build environments across different development setups.
src/test/java/org/javarosa/core/storage/IndexedStorageUtilityTests.java (1)
57-57: LGTM! Good code cleanup.The removal of unused imports and addition of blank lines improves code hygiene without affecting functionality.
Also applies to: 126-129
build.gradle (1)
86-86: Verify the geodesy library version and security status.The dependency change from JTS to Gavaghan Geodesy aligns with the PR objective to incorporate earth curvature calculations. However, please verify that version 1.1.3 is current and secure.
What is the latest version of org.gavaghan:geodesy library and are there any known security vulnerabilities?src/main/java/org/javarosa/xpath/expr/XPathIsPointInsidePolygonFunc.java (2)
3-3: LGTM! Correct import updates for geodesy library.The import changes correctly reflect the migration from JTS geometry to the Gavaghan Geodesy library.
Also applies to: 13-13
67-71: I'll directly verify the method signatures inPolygonUtils.javausing ripgrep:#!/bin/bash set -e # Locate the PolygonUtils source file file=$(rg --files-with-matches "class PolygonUtils" -g "*.java" | head -n1) if [[ -z "$file" ]]; then echo "⚠️ Could not find PolygonUtils.java" exit 1 fi echo "🔎 Checking in: $file" # Verify createPolygon echo -e "\n🔍 createPolygon signature:" rg -n "public static List<GlobalCoordinates> createPolygon" "$file" -C2 || echo "❌ createPolygon not found" # Verify isValidCoordinates echo -e "\n🔍 isValidCoordinates signature:" rg -n "public static void isValidCoordinates" "$file" -C2 || echo "❌ isValidCoordinates not found" # Verify isPointInsideOrOnPolygon echo -e "\n🔍 isPointInsideOrOnPolygon signature:" rg -n "public static boolean isPointInsideOrOnPolygon" "$file" -C2 || echo "❌ isPointInsideOrOnPolygon not found"src/test/java/org/javarosa/xpath/test/XPathEvalTest.java (2)
614-638: Test precision improvements align with geodesic calculations.The updated expected coordinates show increased precision, which is consistent with the shift from planar geometry (JTS) to ellipsoidal geodesic calculations (Gavaghan Geodesy). The more precise floating-point representations reflect the enhanced accuracy of earth curvature-aware computations.
704-712: Excellent addition of large polygon test cases.The new test cases for ~150km polygons are valuable additions that validate the geodesic calculations over larger distances where the difference between planar and ellipsoidal calculations becomes more significant.
src/main/java/org/javarosa/xpath/expr/XPathClosestPointOnPolygonFunc.java (2)
2-2: Proper import updates for Gavaghan Geodesy library.The import changes correctly reflect the shift from JTS to Gavaghan Geodesy library for geodesic calculations.
Also applies to: 13-13
73-77: Good integration with updated PolygonUtils API.The method properly adapts to the new
List<GlobalCoordinates>polygon representation and correctly uses the updatedPolygonUtilsmethods. The addition of coordinate validation viaisValidCoordinatesimproves robustness by catching invalid input early.gradlew.bat (1)
1-92: Standard and well-formed Gradle wrapper script.This is a properly implemented Windows batch script for the Gradle wrapper that handles environment setup, Java detection, error handling, and argument passing correctly. The script follows Gradle wrapper best practices.
gradlew (1)
1-241: Excellent POSIX-compliant Gradle wrapper script.This is a thoroughly documented and well-implemented shell script that properly handles symlink resolution, OS-specific adjustments, Java detection, and argument processing. The extensive documentation and robust error handling demonstrate good practices.
src/main/java/org/javarosa/core/model/utils/PolygonUtils.java (6)
2-5: Appropriate library imports for geodesic calculations.The imports correctly bring in the Gavaghan Geodesy library components needed for ellipsoid-aware geographic calculations, replacing the previous JTS dependencies.
23-51: Robust polygon creation with proper validation.The method correctly handles polygon creation from coordinate pairs, validates coordinates, ensures polygon closure, and checks for minimum vertex requirements. The logic is sound and well-implemented.
60-64: Clean coordinate validation method.Making the validation method public and simplifying it to focus on geographic bounds is a good design choice that supports reuse across the codebase.
73-97: Well-implemented geodesic closest point calculation.The method properly iterates through polygon edges and uses geodesic projections to find the closest point. The use of ellipsoidal distance calculations provides more accurate results than planar geometry.
109-135: Correct geodesic projection implementation.The projection onto a geodesic segment is mathematically sound, using azimuth and distance calculations to find the closest point on each edge. The handling of edge cases (projection before start or after end of segment) is appropriate.
144-173: Verify the numeric stability of the ray casting algorithm.The ray casting implementation looks correct, but there's a potential concern with the small epsilon value added to avoid division by zero.
The tolerance value
1e-10in line 167 may be too small and could lead to precision issues with certain coordinate values. Consider using a more appropriate tolerance based on the coordinate precision requirements.#!/bin/bash # Check for similar epsilon usage patterns in geospatial libraries rg -A 3 -B 3 "1e-10|1e-9|1e-8" --type java
|
|
||
| /** | ||
| * Load multiple record objects from storage at one time from a list of record ids. | ||
| * <p> | ||
| * If the provided recordMap already contains entries for any ids, it is _not_ | ||
| * required for them to be retrieved from storage again. | ||
| * | ||
| * if orderby is passed it will give the sorted list based on this key, need to add DESC or ASC for | ||
| * sorting done by ascending or descending | ||
| * | ||
| * @throws RequestAbandonedException If the current request is abandoned, this method will | ||
| * throw a RequestAbandonedException. Callers should not | ||
| * generally catch that exception unless they rethrow it | ||
| * or another exception, but they should anticipate that | ||
| * they may need to clean up if the bulk read doesn't complete | ||
| */ | ||
|
|
||
|
|
There was a problem hiding this comment.
Remove duplicate Javadoc and fix parameter inconsistency.
This creates duplicate documentation for the bulkRead method, as there's already comprehensive Javadoc starting at line 242. Additionally, the new documentation mentions an "orderby" parameter that doesn't exist in the method signature (line 272-273).
Either remove this duplicate documentation or clarify if it's intended for a different method overload. If an orderby parameter is planned, the method signature should be updated accordingly.
-
- /**
- * Load multiple record objects from storage at one time from a list of record ids.
- * <p>
- * If the provided recordMap already contains entries for any ids, it is _not_
- * required for them to be retrieved from storage again.
- *
- * if orderby is passed it will give the sorted list based on this key, need to add DESC or ASC for
- * sorting done by ascending or descending
- *
- * @throws RequestAbandonedException If the current request is abandoned, this method will
- * throw a RequestAbandonedException. Callers should not
- * generally catch that exception unless they rethrow it
- * or another exception, but they should anticipate that
- * they may need to clean up if the bulk read doesn't complete
- */
-📝 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.
| /** | |
| * Load multiple record objects from storage at one time from a list of record ids. | |
| * <p> | |
| * If the provided recordMap already contains entries for any ids, it is _not_ | |
| * required for them to be retrieved from storage again. | |
| * | |
| * if orderby is passed it will give the sorted list based on this key, need to add DESC or ASC for | |
| * sorting done by ascending or descending | |
| * | |
| * @throws RequestAbandonedException If the current request is abandoned, this method will | |
| * throw a RequestAbandonedException. Callers should not | |
| * generally catch that exception unless they rethrow it | |
| * or another exception, but they should anticipate that | |
| * they may need to clean up if the bulk read doesn't complete | |
| */ |
🤖 Prompt for AI Agents
In src/main/java/org/javarosa/core/services/storage/IStorageUtilityIndexed.java
around lines 254 to 271, there is duplicate Javadoc for the bulkRead method and
a mention of an "orderby" parameter that does not exist in the method signature
at lines 272-273. Remove this duplicate Javadoc block or clarify if it is meant
for a different method overload. If the orderby parameter is intended, update
the method signature to include it; otherwise, remove the reference to avoid
inconsistency.
| private Object getFieldValue(T record, String fieldName) throws Exception { | ||
| // Get the field by name and set accessible | ||
| Field field = record.getClass().getDeclaredField(fieldName); | ||
| field.setAccessible(true); | ||
| return field.get(record); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
🧩 Verification correct
Address the unused reflection method and improve exception handling.
The new getFieldValue method has several concerns:
- Unused code: This method is not referenced anywhere in the provided context
- Security risk: Using reflection to bypass access controls could introduce security vulnerabilities
- Poor exception handling: Throwing generic
Exceptionmasks specific reflection exceptions
Consider the following improvements:
-private Object getFieldValue(T record, String fieldName) throws Exception {
+private Object getFieldValue(T record, String fieldName) throws NoSuchFieldException, IllegalAccessException {
// Get the field by name and set accessible
Field field = record.getClass().getDeclaredField(fieldName);
field.setAccessible(true);
return field.get(record);
}Please verify if this method is actually needed:
🏁 Script executed:
#!/bin/bash
# Search for usage of getFieldValue method
rg -A 3 "getFieldValue" --type javaLength of output: 615
Address the unused reflection method and improve exception handling.
The new getFieldValue method has several concerns:
- Unused code: This method is not referenced anywhere in the provided context
- Security risk: Using reflection to bypass access controls could introduce security vulnerabilities
- Poor exception handling: Throwing generic
Exceptionmasks specific reflection exceptions
Consider the following improvements:
-private Object getFieldValue(T record, String fieldName) throws Exception {
+private Object getFieldValue(T record, String fieldName) throws NoSuchFieldException, IllegalAccessException {
// Get the field by name and set accessible
Field field = record.getClass().getDeclaredField(fieldName);
field.setAccessible(true);
return field.get(record);
}Please verify if this method is actually needed:
#!/bin/bash
# Search for usage of getFieldValue method
rg -A 3 "getFieldValue" --type java🤖 Prompt for AI Agents
In
src/main/java/org/javarosa/core/services/storage/util/DummyIndexedStorageUtility.java
around lines 178 to 183, the getFieldValue method is unused, uses reflection
that may pose security risks, and throws a generic Exception which obscures
specific errors. First, verify if this method is needed by searching for its
usage in the codebase. If it is not used, remove the method entirely. If it is
needed, replace the generic Exception with more specific exceptions such as
NoSuchFieldException and IllegalAccessException, and consider safer alternatives
to reflection or restrict its use to trusted contexts to mitigate security
concerns.
|
extra files in this file |
Product Description
Link
this is the XPath implementation of function
-is-point-inside-polygon
-closest-point-of-polygon
purpose is to keep earth curvature in mind to calculate the closest point
used Gavaghan Geodesy lib to do so
Technical Summary
Safety Assurance
Safety story
Automated test coverage
QA Plan
Special deploy instructions
Rollback instructions
Review
Duplicate PR
Automatically duplicate this PR as defined in contributing.md.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor
Tests