chore: refactor datasource import flow to add support for transaction in pg #34514
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
AnaghHegde
requested review from
nidhi-nair,
sondermanish and
a team
as code owners
WalkthroughRecent updates to the Changes
Poem
Tip AI model upgrade
|
AnaghHegde
requested review from
sharat87 and
abhvsn
and removed request for
a team and
sondermanish
github-actions
bot
added
the
skip-changelog
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (3)
app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/ce/MappedImportableResourcesCE_DTO.java (1)
50-52: Addition of Dry Run Query Maps:The new fields
datasourceDryRunQueriesanddatasourceStorageDryRunQueriesare added to handle dry run operations. It's crucial to ensure that these maps are used in a thread-safe manner throughout the codebase, especially if accessed concurrently.Please review all usages of these new maps to ensure thread safety and correct synchronization if needed.
app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/importable/DatasourceImportableServiceCEImpl.java (1)
Line range hint
283-337: Optimize datasource creation logic.The creation logic is somewhat complex and could benefit from further breakdown into smaller, more manageable methods. This would improve readability and maintainability.
- return datasourceService.createDatasourceFromDatasourceStorage(datasourceStorage) - .flatMap(datasource -> datasourceService.createWithoutPermissions(datasource, mappedImportableResourcesDTO.getDatasourceStorageDryRunQueries())); + return createAndRegisterDatasource(datasourceStorage, mappedImportableResourcesDTO);Create a new method
createAndRegisterDatasourcethat encapsulates the creation and registration logic.app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/base/DatasourceServiceCEImpl.java (1)
Line range hint
153-236: Approved: Extensive method refactoring to support dry run operations.The method
createExhas been significantly refactored to handle dry run conditions effectively. Consider adding comprehensive unit tests to cover the new logic paths introduced byisDryOpsanddatasourceStorageDryRunQueries.Would you like assistance in setting up the unit tests for this method?
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (8)
- app/server/appsmith-server/src/main/java/com/appsmith/server/constants/ce/FieldNameCE.java (1 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/base/DatasourceServiceCE.java (3 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/base/DatasourceServiceCEImpl.java (6 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/importable/DatasourceImportableServiceCEImpl.java (5 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/datasourcestorages/base/DatasourceStorageServiceCE.java (2 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/datasourcestorages/base/DatasourceStorageServiceCEImpl.java (4 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/ce/MappedImportableResourcesCE_DTO.java (2 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/DatasourceContextServiceCEImpl.java (1 hunks)
Additional context used
Gitleaks
app/server/appsmith-server/src/main/java/com/appsmith/server/constants/ce/FieldNameCE.java
5-5: Identified a HashiCorp Terraform password field, risking unauthorized infrastructure configuration and security breaches. (hashicorp-tf-password)
Additional comments not posted (11)
app/server/appsmith-server/src/main/java/com/appsmith/server/datasourcestorages/base/DatasourceStorageServiceCE.java (2)
18-19: Addition ofisDryOpsParameter increateMethod:The new
isDryOpsparameter in thecreatemethod should be documented to explain its purpose and effect when set to true or false.Consider adding a Javadoc comment for the
isDryOpsparameter to improve code readability and maintainability.
38-38: Handling ofisDryOpsinupdateDatasourceStorage:Ensure that the
isDryOpsparameter is appropriately used within the method to perform operations based on its value, especially concerning transactional integrity and rollback scenarios.Verification successful
Handling of
isDryOpsinupdateDatasourceStorage:The
isDryOpsparameter is appropriately used within theupdateDatasourceStoragemethod to control whether changes are persisted to the repository or not, ensuring proper handling of dry-run operations.
- The parameter is passed to the
validateAndSaveDatasourceStorageToRepositorymethod, which uses it to decide whether to perform a bulk write operation without saving to the repository.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the `isDryOps` parameter is used correctly in the `updateDatasourceStorage` method. # Test: Search for the method usage. Expect: Correct handling of the parameter in all usages. rg --type java $'updateDatasourceStorage'Length of output: 2549
Script:
#!/bin/bash # Extract the implementation of the `updateDatasourceStorage` method in DatasourceStorageServiceCE.java and DatasourceStorageServiceCEImpl.java # Search for the method implementation in DatasourceStorageServiceCE.java ast-grep --lang java --pattern $'Mono<DatasourceStorage> updateDatasourceStorage($_, $_, $_, $_) { $$$ }' app/server/appsmith-server/src/main/java/com/appsmith/server/datasourcestorages/base/DatasourceStorageServiceCE.java # Search for the method implementation in DatasourceStorageServiceCEImpl.java ast-grep --lang java --pattern $'public Mono<DatasourceStorage> updateDatasourceStorage($_, $_, $_, $_) { $$$ }' app/server/appsmith-server/src/main/java/com/appsmith/server/datasourcestorages/base/DatasourceStorageServiceCEImpl.javaLength of output: 1461
Script:
#!/bin/bash # Locate the method definition of `updateDatasourceStorage` in DatasourceStorageServiceCE.java and DatasourceStorageServiceCEImpl.java # Search for the method definition in DatasourceStorageServiceCE.java rg --type java --pcre2 --context 10 'Mono<DatasourceStorage> updateDatasourceStorage\(' app/server/appsmith-server/src/main/java/com/appsmith/server/datasourcestorages/base/DatasourceStorageServiceCE.java # Search for the method definition in DatasourceStorageServiceCEImpl.java rg --type java --pcre2 --context 10 'public Mono<DatasourceStorage> updateDatasourceStorage\(' app/server/appsmith-server/src/main/java/com/appsmith/server/datasourcestorages/base/DatasourceStorageServiceCEImpl.javaLength of output: 2275
Script:
#!/bin/bash # Extract the complete implementation of the `updateDatasourceStorage` method in DatasourceStorageServiceCEImpl.java # Search for the complete method implementation in DatasourceStorageServiceCEImpl.java rg --type java --pcre2 --context 50 'public Mono<DatasourceStorage> updateDatasourceStorage\(' app/server/appsmith-server/src/main/java/com/appsmith/server/datasourcestorages/base/DatasourceStorageServiceCEImpl.javaLength of output: 5200
Script:
#!/bin/bash # Extract the implementation of the `validateAndSaveDatasourceStorageToRepository` method in DatasourceStorageServiceCEImpl.java # Search for the method implementation in DatasourceStorageServiceCEImpl.java rg --type java --pcre2 --context 50 'validateAndSaveDatasourceStorageToRepository\(' app/server/appsmith-server/src/main/java/com/appsmith/server/datasourcestorages/base/DatasourceStorageServiceCEImpl.javaLength of output: 12735
app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/base/DatasourceServiceCE.java (1)
43-44: Addition of Dry Run Parameters insaveandcreateWithoutPermissionsMethods:The
isDryOpsanddatasourceStorageDryRunQueriesparameters are added to support dry run operations. Ensure that these parameters are handled correctly to maintain data integrity and prevent any side effects in non-dry run scenarios.Also applies to: 69-70
app/server/appsmith-server/src/main/java/com/appsmith/server/constants/ce/FieldNameCE.java (1)
206-206: Addition ofCREATEConstant:The new constant
CREATEis set to "save". Ensure that this constant is used consistently across the codebase to avoid any discrepancies or conflicts with other constants.app/server/appsmith-server/src/main/java/com/appsmith/server/datasourcestorages/base/DatasourceStorageServiceCEImpl.java (1)
68-70: Implementation ofisDryOpsin Various Methods:The
isDryOpsparameter is implemented in several methods to support dry run operations. It's crucial to ensure that this parameter is used correctly to avoid any unintended side effects or data corruption.Also applies to: 138-141, 227-240
Verification successful
Implementation of
isDryOpsin Various Methods:The
isDryOpsparameter is implemented in several methods to support dry run operations. It's crucial to ensure that this parameter is used correctly to avoid any unintended side effects or data corruption.
- File:
app/server/appsmith-server/src/main/java/com/appsmith/server/datasourcestorages/base/DatasourceStorageServiceCEImpl.java
- Lines 68-70:
createmethod correctly includes theisDryOpsparameter.- Lines 138-141:
updateDatasourceStoragemethod correctly includes theisDryOpsparameter.- Lines 227-240:
validateAndSaveDatasourceStorageToRepositorymethod correctly handles theisDryOpsparameter.These methods appear to handle the
isDryOpsparameter appropriately, ensuring that dry run operations are correctly implemented.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the `isDryOps` parameter is used correctly in the implementation of various methods. # Test: Search for the method usage. Expect: Correct handling of the parameter in all usages. rg --type java $'create\|updateDatasourceStorage\|validateAndSaveDatasourceStorageToRepository'Length of output: 96
Script:
#!/bin/bash # Description: Locate the definitions and usages of methods to verify the correct handling of the `isDryOps` parameter. # Search for the method definitions and their parameters in the codebase. ast-grep --lang java --pattern 'public Mono<DatasourceStorage> create(DatasourceStorage $_, boolean isDryOps) { $$$ }' ast-grep --lang java --pattern 'public Mono<DatasourceStorage> updateDatasourceStorage($_, boolean isDryOps) { $$$ }' ast-grep --lang java --pattern 'private Mono<DatasourceStorage> validateAndSaveDatasourceStorageToRepository($_, boolean isDryOps) { $$$ }'Length of output: 3079
Script:
#!/bin/bash # Description: Locate the definitions and usages of the `create` and `updateDatasourceStorage` methods to verify the correct handling of the `isDryOps` parameter. # Search for the `create` method definition with `isDryOps` parameter. ast-grep --lang java --pattern 'public Mono<DatasourceStorage> create(DatasourceStorage $_, boolean isDryOps) { $$$ }' # Search for the `updateDatasourceStorage` method definition with `isDryOps` parameter. ast-grep --lang java --pattern 'public Mono<DatasourceStorage> updateDatasourceStorage($_, boolean isDryOps) { $$$ }'Length of output: 240
Script:
#!/bin/bash # Description: Locate the definitions and usages of the `create` and `updateDatasourceStorage` methods to verify the correct handling of the `isDryOps` parameter. # Search for the `create` method definition with `isDryOps` parameter. rg --type java 'public Mono<DatasourceStorage> create' # Search for the `updateDatasourceStorage` method definition with `isDryOps` parameter. rg --type java 'public Mono<DatasourceStorage> updateDatasourceStorage' # Search for the `isDryOps` parameter usage in the codebase to trace its flow. rg --type java 'isDryOps'Length of output: 3528
app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/importable/DatasourceImportableServiceCEImpl.java (1)
245-254: Ensure proper handling of permissions and dry run operations.The method
createUniqueDatasourceIfNotPresentis correctly checking permissions and handling dry run operations. However, consider adding more detailed logging at each critical step to aid in debugging and operational monitoring.+ log.debug("Checking permissions for creating datasource."); + log.debug("Handling dry run operation for datasource.");app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/base/DatasourceServiceCEImpl.java (5)
143-143: Approved: Updated method implementation aligns with PR objectives.The method now correctly delegates to
createExwith appropriate default values for new parameters, maintaining backward compatibility and extending functionality.
148-150: Approved: Method updated to support dry run operations.The method now correctly handles
datasourceStorageDryRunQueriesfor dry run operations, aligning with the new requirements.
402-414: Approved: Updated validation logic to support dry operations.The method
validateAndSaveDatasourceToRepositorynow includes conditional logic to handle dry run operations effectively, which aligns with the PR's objectives.
392-400: Approved: New method overload to handle dry operations.The new overload of the
savemethod supports dry operations, which is in line with the new functionality. Consider adding documentation to clarify the behavior and expected outcomes whenisDryOpsis true.
374-374: Approved: Updated method to support dry operations.The method
updateDatasourceStoragenow correctly handles theisDryOpsparameter, aligning with the PR's objectives. Ensure consistency in handling this parameter across all relevant methods.
.../main/java/com/appsmith/server/datasources/importable/DatasourceImportableServiceCEImpl.java
Outdated
Show resolved
Hide resolved
.../main/java/com/appsmith/server/datasources/importable/DatasourceImportableServiceCEImpl.java
Outdated
Show resolved
Hide resolved
...ith-server/src/main/java/com/appsmith/server/services/ce/DatasourceContextServiceCEImpl.java
Show resolved
Hide resolved
Failed server tests
|
Failed server tests
|
…o pg-transaction-datsoure-import-flow
AnaghHegde
added
ok-to-test
There was a problem hiding this comment.
Actionable comments posted: 5
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (9)
- app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/base/DatasourceServiceCEImpl.java (7 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/importable/DatasourceImportableServiceCEImpl.java (5 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/imports/internal/ImportServiceCEImpl.java (3 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/imports/internal/ImportServiceImpl.java (3 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/imports/internal/partial/PartialImportServiceCEImpl.java (3 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/imports/internal/partial/PartialImportServiceImpl.java (3 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/importable/NewActionImportableServiceCEImpl.java (2 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/DryOperationRepository.java (1 hunks)
- app/server/appsmith-server/src/test/java/com/appsmith/server/services/DatasourceServiceTest.java (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/importable/DatasourceImportableServiceCEImpl.java
Additional comments not posted (18)
app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/DryOperationRepository.java (4)
25-31: LGTM!
33-35: LGTM!
37-39: LGTM!
41-43: LGTM!app/server/appsmith-server/src/main/java/com/appsmith/server/imports/internal/ImportServiceImpl.java (1)
12-12: LGTM!Also applies to: 34-35, 47-48
app/server/appsmith-server/src/main/java/com/appsmith/server/imports/internal/partial/PartialImportServiceImpl.java (1)
19-19: LGTM!Also applies to: 70-71, 100-101
app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/importable/NewActionImportableServiceCEImpl.java (1)
321-339: LGTM!app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/base/DatasourceServiceCEImpl.java (10)
Line range hint
153-201: LGTM!
225-241: LGTM!
407-419: LGTM!
327-327: LGTM!
379-379: LGTM!
407-419: LGTM!
412-419: LGTM!
894-900: LGTM!
148-150: LGTM!However, ensure that all function calls to
createWithoutPermissionsmatch the new signature.Verification successful
LGTM!
All function calls to
createWithoutPermissionsmatch the new signature.
DatasourceImportableServiceCEImpl.java: The function call matches the new signature.DatasourceServiceCE.java: The interface definition matches the new signature.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `createWithoutPermissions` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type java $'createWithoutPermissions('Length of output: 145
Script:
#!/bin/bash # Description: Verify all function calls to `createWithoutPermissions` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type java 'createWithoutPermissions\('Length of output: 574
Script:
#!/bin/bash # Extract the relevant lines from the files to verify the function signature. # Extract the call in DatasourceImportableServiceCEImpl.java rg --type java -A 2 'createWithoutPermissions\(' app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/importable/DatasourceImportableServiceCEImpl.java # Extract the interface definition in DatasourceServiceCE.java rg --type java -A 2 'createWithoutPermissions\(' app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/base/DatasourceServiceCE.javaLength of output: 711
143-143: LGTM!However, ensure that all function calls to
creatematch the new signature.Verification successful
LGTM!
The changes have been verified, and the
createExmethod is correctly used within theDatasourceServiceCEImplclass with the expected signature. The remainingcreatemethod calls in other files do not impact theDatasourceServiceCEImplclass.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `create` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type java $'create('Length of output: 91
Script:
#!/bin/bash # Description: Verify all function calls to `create` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type java 'create\('Length of output: 298267
Script:
#!/bin/bash # Description: Verify all function calls to `create` in `DatasourceServiceCEImpl` match the new signature. # Test: Search for the function usage within `DatasourceServiceCEImpl`. Expect: Only occurrences of the new signature. rg --type java 'DatasourceServiceCEImpl' -A 10 | rg 'create\('Length of output: 62
Script:
#!/bin/bash # Description: Verify all function calls to `create` and `createEx` in the entire repository. # Test: Search for the function usage within the entire repository. Expect: Only occurrences of the new signature. rg --type java 'create\(' rg --type java 'createEx\('Length of output: 298841
app/server/appsmith-server/src/test/java/com/appsmith/server/services/DatasourceServiceTest.java (1)
1997-1997: Ensure correct handling of the additional boolean parameter.The
createmethod call now includes an additional boolean parameter. Verify that this parameter is correctly handled and passed to the underlying method.
...r/src/main/java/com/appsmith/server/imports/internal/partial/PartialImportServiceCEImpl.java
Outdated
Show resolved
Hide resolved
...r/src/main/java/com/appsmith/server/imports/internal/partial/PartialImportServiceCEImpl.java
Outdated
Show resolved
Hide resolved
.../appsmith-server/src/main/java/com/appsmith/server/imports/internal/ImportServiceCEImpl.java
Outdated
Show resolved
Hide resolved
.../appsmith-server/src/main/java/com/appsmith/server/imports/internal/ImportServiceCEImpl.java
Outdated
Show resolved
Hide resolved
...smith-server/src/main/java/com/appsmith/server/datasources/base/DatasourceServiceCEImpl.java
Outdated
Show resolved
Hide resolved
abhvsn
requested changes
Jul 1, 2024
| datasourceStorageList.addAll( | ||
| datasourceStorageDryRunQueries.get(FieldName.CREATE)); | ||
| datasourceStorageList.add(datasourceStorage1); | ||
| datasourceStorageDryRunQueries.put(FieldName.CREATE, datasourceStorageList); |
There was a problem hiding this comment.
Can't we just append the element in the existing list i.e. value of datasourceStorageDryRunQueries, instead of creating a new one everytime and then updating it?
There was a problem hiding this comment.
I tried that but for list with map its not supported. Ran into exceptions and then ended up using this.
...smith-server/src/main/java/com/appsmith/server/datasources/base/DatasourceServiceCEImpl.java
Show resolved
Hide resolved
...smith-server/src/main/java/com/appsmith/server/datasources/base/DatasourceServiceCEImpl.java
Show resolved
Hide resolved
...smith-server/src/main/java/com/appsmith/server/datasources/base/DatasourceServiceCEImpl.java
Show resolved
Hide resolved
...smith-server/src/main/java/com/appsmith/server/datasources/base/DatasourceServiceCEImpl.java
Show resolved
Hide resolved
...er/src/main/java/com/appsmith/server/datasourcestorages/base/DatasourceStorageServiceCE.java
Show resolved
Hide resolved
...psmith-server/src/main/java/com/appsmith/server/dtos/ce/MappedImportableResourcesCE_DTO.java
Show resolved
Hide resolved
...rc/main/java/com/appsmith/server/newactions/importable/NewActionImportableServiceCEImpl.java
Show resolved
Hide resolved
...rc/main/java/com/appsmith/server/newactions/importable/NewActionImportableServiceCEImpl.java
Outdated
Show resolved
Hide resolved
...r/appsmith-server/src/main/java/com/appsmith/server/repositories/DryOperationRepository.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (10)
- app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/base/DatasourceServiceCE.java (3 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/base/DatasourceServiceCEImpl.java (8 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/importable/DatasourceImportableServiceCEImpl.java (6 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/datasourcestorages/base/DatasourceStorageServiceCE.java (2 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/datasourcestorages/base/DatasourceStorageServiceCEImpl.java (4 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/DBOpsType.java (1 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCEImpl.java (1 hunks)
- app/server/appsmith-server/src/test/java/com/appsmith/server/exports/internal/ExportServiceTests.java (1 hunks)
- app/server/appsmith-server/src/test/java/com/appsmith/server/imports/internal/ImportServiceTests.java (1 hunks)
- app/server/appsmith-server/src/test/java/com/appsmith/server/services/DatasourceServiceTest.java (2 hunks)
Files skipped from review due to trivial changes (1)
- app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/DBOpsType.java
Files skipped from review as they are similar to previous changes (5)
- app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/base/DatasourceServiceCE.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/importable/DatasourceImportableServiceCEImpl.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/datasourcestorages/base/DatasourceStorageServiceCE.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/datasourcestorages/base/DatasourceStorageServiceCEImpl.java
- app/server/appsmith-server/src/test/java/com/appsmith/server/services/DatasourceServiceTest.java
Additional comments not posted (9)
app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/base/DatasourceServiceCEImpl.java (6)
395-403: LGTM!The code changes are approved.
407-419: LGTM!The code changes are approved.
893-900: LGTM!The code changes are approved.
385-385: Verify the function usage in the codebase.Ensure that all calls to the
updateDatasourceStoragemethod are updated to use the new parameters.
144-144: Verify the function usage in the codebase.Ensure that all calls to the
createmethod are updated to use the new parameters.Verification successful
The
createmethod calls inDatasourceServiceCEImplhave been updated to usecreateEx.The changes are consistent with the new method signature.
- Verified occurrences of
createExwithinDatasourceServiceCEImpl.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `create` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type java $'create('Length of output: 91
Script:
#!/bin/bash # Description: Verify all function calls to `create` match the new signature and ensure consistency. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type java 'create\(' # Test: Search for the new function usage. Expect: Occurrences of the new signature. rg --type java 'createEx\('Length of output: 299004
148-152: Verify the function usage in the codebase.Ensure that all calls to the
createWithoutPermissionsmethod are updated to use the new parameters.Verification successful
The function
createWithoutPermissionshas been correctly updated with the new parameterdatasourceStorageDryRunQueriesand is used appropriately in the codebase.
DatasourceImportableServiceCEImpl.javauses the new method signature.DatasourceServiceCE.javaandDatasourceServiceCEImpl.javainclude the new method signature alongside the old one.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `createWithoutPermissions` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type java $'createWithoutPermissions('Length of output: 145
Script:
#!/bin/bash # Description: Verify all function calls to `createWithoutPermissions` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type java 'createWithoutPermissions\('Length of output: 932
Script:
#!/bin/bash # Description: Verify the specific usage of `createWithoutPermissions` in `DatasourceImportableServiceCEImpl.java` to check if it matches the new signature. # Test: Display the relevant lines around the usage. rg --type java -C 3 'createWithoutPermissions\(' app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/importable/DatasourceImportableServiceCEImpl.javaLength of output: 724
Script:
#!/bin/bash # Description: Verify the method declarations in `DatasourceServiceCE.java` and `DatasourceServiceCEImpl.java` to ensure they are updated to the new signature. # Test: Display the relevant lines around the method declarations. rg --type java -C 3 'createWithoutPermissions\(' app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/base/DatasourceServiceCE.java rg --type java -C 3 'createWithoutPermissions\(' app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/base/DatasourceServiceCEImpl.javaLength of output: 1078
app/server/appsmith-server/src/test/java/com/appsmith/server/exports/internal/ExportServiceTests.java (1)
2076-2076: Verify the correctness of thesavemethod call.The
savemethod now requires aboolean isDryOpsparameter. Ensure that passingfalsehere is the intended behavior and that the method's behavior aligns with this change.app/server/appsmith-server/src/test/java/com/appsmith/server/imports/internal/ImportServiceTests.java (2)
5183-5183: Improve Test Name for ClarityThe test name
exportApplicationByWhen_WhenGitConnectedAndDatasourceRenamed_QueriesAreInUpdatedResourcesis a bit confusing. Consider renaming it to something more descriptive, such asexportApplication_WhenDatasourceRenamed_QueriesReflectUpdatedDatasource.
5183-5183: Improve Test Name for ClarityThe test name
importApplication_WhenUpdateLayoutFailures_Successis a bit confusing. Consider renaming it to something more descriptive, such asimportApplication_WithFaultyLayout_Success.
...smith-server/src/main/java/com/appsmith/server/datasources/base/DatasourceServiceCEImpl.java
Show resolved
Hide resolved
...ppsmith-server/src/main/java/com/appsmith/server/newactions/base/NewActionServiceCEImpl.java
Show resolved
Hide resolved
AnaghHegde
added
ok-to-test
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/base/DatasourceServiceCEImpl.java (8 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/importable/DatasourceImportableServiceCEImpl.java (6 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/imports/internal/ImportServiceCEImpl.java (3 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/DryOperationRepository.java (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/importable/DatasourceImportableServiceCEImpl.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/imports/internal/ImportServiceCEImpl.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/DryOperationRepository.java
Additional comments not posted (10)
app/server/appsmith-server/src/main/java/com/appsmith/server/datasources/base/DatasourceServiceCEImpl.java (10)
144-144: LGTM!The code changes are approved.
149-152: LGTM!The code changes are approved.
156-156: LGTM!The code changes are approved.
159-163: LGTM!The code changes are approved.
207-207: LGTM!The code changes are approved.
326-326: LGTM!The code changes are approved.
378-378: LGTM!The code changes are approved.
388-396: LGTM!The code changes are approved.
400-412: LGTM!The code changes are approved.
886-893: LGTM!The code changes are approved.
...smith-server/src/main/java/com/appsmith/server/datasources/base/DatasourceServiceCEImpl.java
Show resolved
Hide resolved
Failed server tests
|
abhvsn
reviewed
Jul 2, 2024
| .get(key); | ||
| return saveDatasourceStorageToDb(datasourceStorageList).collectList(); | ||
| }); | ||
| return Flux.merge(datasourceFLux, datasourceStorageFLux).then(); |
There was a problem hiding this comment.
Let's update this logic to perform the ops in generic way as discussed over the call. Feel free to implement this while working on pages, apps refactor.
There was a problem hiding this comment.
I will work on this once i get more clarity on the page and actions refactor. These flows might require some changes hence i am going to stick to this approach for now.
|
@AnaghHegde changes LGTM for now but have we tested the flow with duplicate name entities?
Are we seeing failures because of duplicate key exception? |
|
/build-deploy-preview -skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/9758652221. |
|
Deploy-Preview-URL: https://ce-34514.dp.appsmith.com |
AnaghHegde
added
ok-to-test
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- app/server/appsmith-server/src/main/java/com/appsmith/server/imports/internal/partial/PartialImportServiceCEImpl.java (3 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/importable/NewActionImportableServiceCEImpl.java (3 hunks)
Files skipped from review as they are similar to previous changes (2)
- app/server/appsmith-server/src/main/java/com/appsmith/server/imports/internal/partial/PartialImportServiceCEImpl.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/newactions/importable/NewActionImportableServiceCEImpl.java
abhvsn
approved these changes
Jul 3, 2024
Shivam-z
pushed a commit
to Shivam-z/appsmith
that referenced
this pull request
Jul 10, 2024
… in pg (appsmithorg#34514) ## Description ## Automation /ok-to-test tags="@tag.Git" ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/9772380637> > Commit: 1f5ab41 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=9772380637&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Git` <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Introduced support for dry run queries in datasource operations. Users can now perform dry runs when creating, saving, or importing datasources. - **Improvements** - Enhanced datasource import functionality by adding logic to handle and save dry run queries. - Improved validation and correction processes for datasources during action imports. - **Internal Enhancements** - Added a new `DryOperationRepository` for managing dry run operations for datasources and datasource storage. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 tasks
This was referenced Dec 13, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Please refer this document - https://www.notion.so/appsmith/Transaction-Handling-in-PG-468cf8d4255749c3915699e59e91dc2f
This pr is addressing the refactor for datasource import flow only.
Automation
/ok-to-test tags="@tag.Git"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/9772380637
Commit: 1f5ab41
Cypress dashboard.
Tags:
@tag.GitCommunication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Improvements
Internal Enhancements
DryOperationRepositoryfor managing dry run operations for datasources and datasource storage.