fix:added a method to identify the data types properly and show in th… #35394
Conversation
WalkthroughThe updates to the Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
Documentation and Community
|
![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
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- app/server/appsmith-plugins/firestorePlugin/src/main/java/com/external/plugins/FirestorePlugin.java (10 hunks)
Additional comments not posted (5)
app/server/appsmith-plugins/firestorePlugin/src/main/java/com/external/plugins/FirestorePlugin.java (5)
93-98: LGTM! Verify the usage of new constants and regex pattern.The new constants and regex pattern are well-defined.
Ensure that these new constants and the regex pattern are correctly used throughout the codebase.
Verification successful
Usage of new constants and regex pattern verified.
The new constants (
LATITUDE,LONGITUDE,SECONDS,NANO_SECONDS) and the regex pattern (REFERENCE_PATTERN) are appropriately used in theFirestorePlugin.javafile.
- File:
app/server/appsmith-plugins/firestorePlugin/src/main/java/com/external/plugins/FirestorePlugin.java
- Lines:
REFERENCE_PATTERN: Line 93, 98LATITUDE: Line 94, 99, 100LONGITUDE: Line 95, 101, 102SECONDS: Line 96, 97, 99NANO_SECONDS: Line 97, 98, 100Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of new constants and regex pattern in the codebase. # Test: Search for the usage of each constant and the regex pattern. Expect: Appropriate usage in relevant methods. rg --type java 'LATITUDE|LONGITUDE|SECONDS|NANO_SECONDS|REFERENCE_PATTERN'Length of output: 11984
809-811: LGTM! Verify the correctness ofcheckAndConvertDataTypemethod.The
checkAndConvertDataTypemethod is correctly applied tomapBody.Ensure that the
checkAndConvertDataTypemethod is functioning as expected.Verification successful
Verified: The
checkAndConvertDataTypemethod is correctly implemented and applied.The method handles various data types and converts them appropriately, ensuring the correct data types are used within the Firestore plugin.
- Converts specific map structures to
TimestampandGeoPointobjects.- Validates and processes strings as Firestore document references.
- Returns the original value for other types.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness of `checkAndConvertDataType` method. # Test: Search for the implementation and usage of `checkAndConvertDataType` method. Expect: Correct implementation and usage. rg --type java 'checkAndConvertDataType'Length of output: 427
Script:
#!/bin/bash # Extract the implementation details of the `checkAndConvertDataType` method from FirestorePlugin.java ast-grep --lang java --pattern $'private Object checkAndConvertDataType(Object value, CollectionReference collection) { $$$ }'Length of output: 3925
836-858: LGTM! Verify the correctness ofcheckAndConvertDataTypemethod.The
checkAndConvertDataTypemethod correctly identifies and converts timestamp and geolocation data types.Ensure that the method handles all edge cases and functions as expected.
Verification successful
Verification Successful:
checkAndConvertDataTypeMethodThe
checkAndConvertDataTypemethod is correctly implemented and used within theFirestorePlugin.javafile. It ensures that specific data types are converted to Firestore-compatible types before the data is used in Firestore operations.
- The method is invoked in a
replaceAllcall on themapBodyvariable, which is aMap<String, Object>.- The method handles timestamp and geolocation data types, converting them to Firestore's
TimestampandGeoPointtypes, respectively.- The usage context confirms that the method is applied correctly to transform data before Firestore operations.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correctness of `checkAndConvertDataType` method. # Test: Search for the implementation and usage of `checkAndConvertDataType` method. Expect: Correct implementation and usage. rg --type java 'checkAndConvertDataType'Length of output: 427
Script:
#!/bin/bash # Extract more context around the usage of `checkAndConvertDataType` method in `FirestorePlugin.java`. # Search for the `mapBody` variable and its surrounding code. rg --type java -A 10 'mapBody' app/server/appsmith-plugins/firestorePlugin/src/main/java/com/external/plugins/FirestorePlugin.javaLength of output: 7936
504-504: LGTM! Verify the correctness ofhandleDocumentLevelMethodmethod.The
handleDocumentLevelMethodmethod includes changes to handle the new data types.Ensure that the method handles all edge cases and functions as expected.
911-920: LGTM! Verify the correctness ofresultToMapmethod.The
resultToMapmethod correctly handlesTimestampandGeoPointtypes.Ensure that the method handles all edge cases and functions as expected.
| @@ -829,6 +833,30 @@ private Mono<ActionExecutionResult> methodAddToCollection( | |||
| }); | |||
| } | |||
|
|
|||
| private Object checkAndConvertDataType(Object value, CollectionReference collection) { | |||
There was a problem hiding this comment.
@Naveen-Goud Please add unit test case.
There was a problem hiding this comment.
Hii @sagar-qa007 ,I have added the test case for this scenario. Please can you go through it.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- app/server/appsmith-plugins/firestorePlugin/src/test/java/com/external/plugins/FirestorePluginExecutorTest.java (1 hunks)
Additional comments not posted (5)
app/server/appsmith-plugins/firestorePlugin/src/test/java/com/external/plugins/FirestorePluginExecutorTest.java (5)
1-24: Imports are appropriate.The imports include necessary libraries for Firestore, testing, and reactive programming. No unused imports are detected.
26-29: Class declaration is appropriate.The class is annotated with
@Testcontainersand contains aFirestorePluginExecutorinstance.
31-35: Firestore emulator setup is correct.The Firestore emulator is correctly set up using
FirestoreEmulatorContainer.
39-50:setUpmethod is correct.The
setUpmethod correctly initializes the Firestore connection and sets the datasource configuration.
52-83: Test methodtestMethodAddToCollectionis comprehensive.The test method correctly tests the addition of a document to a Firestore collection and validates the data types.
NilanshBansal
added
Integrations Pod General
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
|
Hii @somangshu @NilanshBansal @Nikhil-Nandagopal , this PR has been approved , can you merge the changes !! thank you. |
|
@Naveen-Goud this needs further testing from our end. We will be prioritising this in the next week. |
rishabhrathod01
requested review from
rishabhrathod01
and removed request for
a team
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
...ver/appsmith-plugins/firestorePlugin/src/main/java/com/external/plugins/FirestorePlugin.java
Show resolved
Hide resolved
|
Hi @rishabhrathod01, I have resolved the comments.Can you review the updated code. |
|
@Naveen-Goud the server unit test is failing for above PR, could you please check. |
|
Hii @rishabhrathod01 , I have updated the test files and removed few statement in FirestorePluginTest, since they are covered in test file firestorePluginExecutorTest which contains the test cases that I have written. thank you. |
NilanshBansal
requested review from
a team
and removed request for
rishabhrathod01
|
@Naveen-Goud could you please resolve the merge conflict by taking pull from latest release? |
Naveen-Goud
force-pushed
the
fix/7176-Firestore_DB_needs_additional_API_support_to_ingest_certain_data_types
branch
from
8a1ca32 to
d5297ca
Compare
|
Hii @rishabhrathod01 , I have resolved the merge conficts , could you review this PR. thank you. |
|
@Naveen-Goud there are server unit tests that are failing on the PR, could you please resolve them? cc: @NilanshBansal I have unassigned myself here from the reviewer, we could re-assign the reviewer to this PR. |
|
Hi @NilanshBansal ,I have checked the test case failing files, those files are not related to the changes I have made. thank you. |
NilanshBansal
added
Query & JS Pod
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
Description of PR
Changes in PR
fixes- #7176
output
below is the screen shot, when we try to get the documents then they are also represented based on their format
Summary by CodeRabbit
New Features
Bug Fixes
Tests