Skip to content

Commit

Permalink
fix: Git discarding null properties (appsmithorg#25068)
Browse files Browse the repository at this point in the history
## Description
> The git discard flow only discards the properties which were set in
the last commit. If the properties are not present in the application
json in the last commit and the user changes these properties to some
value and after that choose to discard, the changes are not overwritten
to null values.

> This happens because git uses a function `copyNestedNonNullProperties`
to overwrite the values to the last commit ones which ignores the
properties if those are null.

> This PR explicitly sets the null properties in the target json, so
that when the user discards the initially set null properties, these get
discarded.

#### PR fixes following issue(s)
Fixes appsmithorg#24920 

#### Media
> A video or a GIF is preferred. when using Loom, don’t embed because it
looks like it’s a GIF. instead, just link to the video
>
>
#### Type of change
> Please delete options that are not relevant.
- Bug fix (non-breaking change which fixes an issue)

## Testing
>
#### How Has This Been Tested?
> Please describe the tests that you ran to verify your changes. Also
list any relevant details for your test configuration.
> Delete anything that is not relevant
- [ ] Manual
- [ ] Jest
- [ ] Cypress
>
>
#### Test Plan
> Add Testsmith test cases links that relate to this PR
>
>
#### Issues raised during DP testing
> Link issues raised during DP testing for better visiblity and tracking
(copy link from comments dropped on this PR)
>
>
>
## Checklist:
#### Dev activity
- [x] My code follows the style guidelines of this project
- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [ ] I have made corresponding changes to the documentation
- [x] My changes generate no new warnings
- [ ] I have added tests that prove my fix is effective or that my
feature works
- [x] New and existing unit tests pass locally with my changes
- [ ] PR is being merged under a feature flag


#### QA activity:
- [ ] [Speedbreak
features](https://github.com/appsmithorg/TestSmith/wiki/Guidelines-for-test-plans#speedbreakers-)
have been covered
- [ ] Test plan covers all impacted features and [areas of
interest](https://github.com/appsmithorg/TestSmith/wiki/Guidelines-for-test-plans#areas-of-interest-)
- [ ] Test plan has been peer reviewed by project stakeholders and other
QA members
- [ ] Manually tested functionality on DP
- [ ] We had an implementation alignment call with stakeholders post QA
Round 2
- [ ] Cypress test cases have been added and approved by SDET/manual QA
- [ ] Added `Test Plan Approved` label after Cypress tests were reviewed
- [ ] Added `Test Plan Approved` label after JUnit tests were reviewed
  • Loading branch information
NilanshBansal authored Jul 4, 2023
1 parent b15087a commit 081d417
Show file tree
Hide file tree
Showing 5 changed files with 461 additions and 30 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,16 @@

import com.appsmith.external.models.ActionDTO;
import com.appsmith.external.models.Datasource;
import com.appsmith.server.domains.Application;
import lombok.extern.slf4j.Slf4j;
import org.springframework.dao.InvalidDataAccessApiUsageException;
import org.springframework.data.mongodb.MongoTransactionException;
import org.springframework.transaction.TransactionException;

import java.util.Map;

import static com.appsmith.external.helpers.AppsmithBeanUtils.copyNestedNonNullProperties;

@Slf4j
public class ImportExportUtils {

Expand Down Expand Up @@ -39,10 +42,10 @@ public static String getErrorMessage(Throwable throwable) {
* @return
*/
public static String sanitizeDatasourceInActionDTO(ActionDTO actionDTO,
Map<String, String> datasourceMap,
Map<String, String> pluginMap,
String workspaceId,
boolean isExporting) {
Map<String, String> datasourceMap,
Map<String, String> pluginMap,
String workspaceId,
boolean isExporting) {

if (actionDTO != null && actionDTO.getDatasource() != null) {

Expand All @@ -69,4 +72,33 @@ public static String sanitizeDatasourceInActionDTO(ActionDTO actionDTO,

return "";
}

public static void setPropertiesToExistingApplication(Application importedApplication, Application existingApplication) {
importedApplication.setId(existingApplication.getId());
// For the existing application we don't need to default
// value of the flag
// The isPublic flag has a default value as false and this
// would be confusing to user
// when it is reset to false during importing where the
// application already is present in DB
importedApplication.setIsPublic(null);
importedApplication.setPolicies(null);
// These properties are not present in the application when it is created, hence the initial commit
// to git doesn't contain these keys and if we want to discard the changes, the function
// copyNestedNonNullProperties ignore these properties and the changes are not discarded
if (importedApplication.getUnpublishedApplicationDetail() == null) {
existingApplication.setUnpublishedApplicationDetail(null);
}
if (importedApplication.getPublishedApplicationDetail() == null) {
existingApplication.setPublishedApplicationDetail(null);
}
if (importedApplication.getPublishedAppLayout() == null) {
existingApplication.setPublishedAppLayout(null);
}
if (importedApplication.getUnpublishedAppLayout() == null) {
existingApplication.setUnpublishedAppLayout(null);
}

copyNestedNonNullProperties(importedApplication, existingApplication);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@
import static com.appsmith.server.constants.ResourceModes.EDIT;
import static com.appsmith.server.constants.ResourceModes.VIEW;
import static com.appsmith.server.helpers.ImportExportUtils.sanitizeDatasourceInActionDTO;
import static com.appsmith.server.helpers.ImportExportUtils.setPropertiesToExistingApplication;
import static java.lang.Boolean.TRUE;

@Slf4j
Expand Down Expand Up @@ -622,13 +623,13 @@ public Mono<ApplicationImportDTO> extractFileAndSaveApplication(String workspace
@Override
public Mono<ApplicationImportDTO> extractFileAndSaveApplication(String workspaceId, Part filePart, String applicationId) {
// workspace id must be present and valid
if(StringUtils.isEmpty(workspaceId)) {
if (StringUtils.isEmpty(workspaceId)) {
return Mono.error(new AppsmithException(AppsmithError.INVALID_PARAMETER, FieldName.WORKSPACE_ID));
}

Mono<ApplicationImportDTO> importedApplicationMono = extractApplicationJson(filePart)
.flatMap(applicationJson -> {
if(StringUtils.isEmpty(applicationId)){
if (StringUtils.isEmpty(applicationId)) {
return importNewApplicationInWorkspaceFromJson(workspaceId, applicationJson);
} else {
return updateNonGitConnectedAppFromJson(workspaceId, applicationId, applicationJson);
Expand Down Expand Up @@ -682,9 +683,9 @@ private Mono<ImportApplicationPermissionProvider> getPermissionProviderForUpdate
* This function will take the Json filepart and updates/creates the application in workspace depending on presence
* of applicationId field
*
* @param workspaceId Workspace to which the application needs to be hydrated
* @param applicationJson Json file which contains the entire application object
* @param applicationId Optional field for application ref which needs to be overridden by the incoming JSON file
* @param workspaceId Workspace to which the application needs to be hydrated
* @param applicationJson Json file which contains the entire application object
* @param applicationId Optional field for application ref which needs to be overridden by the incoming JSON file
* @return saved application in DB
*/
private Mono<Application> updateNonGitConnectedAppFromJson(String workspaceId, String applicationId, ApplicationJson applicationJson) {
Expand Down Expand Up @@ -750,7 +751,7 @@ private Mono<Application> updateNonGitConnectedAppFromJson(String workspaceId, S
@Override
public Mono<Application> importNewApplicationInWorkspaceFromJson(String workspaceId, ApplicationJson importedDoc) {
// workspace id must be present and valid
if(StringUtils.isEmpty(workspaceId)) {
if (StringUtils.isEmpty(workspaceId)) {
return Mono.error(new AppsmithException(AppsmithError.INVALID_PARAMETER, FieldName.WORKSPACE_ID));
}

Expand All @@ -769,10 +770,11 @@ public Mono<Application> importNewApplicationInWorkspaceFromJson(String workspac

/**
* This function will update an existing application. The application is connected to Git.
* @param workspaceId workspace to which application is going to be stored
* @param importedDoc application resource which contains necessary information to save the application
* @param applicationId application which needs to be saved with the updated resources
* @param branchName name of the git branch. null if not connected to git.
*
* @param workspaceId workspace to which application is going to be stored
* @param importedDoc application resource which contains necessary information to save the application
* @param applicationId application which needs to be saved with the updated resources
* @param branchName name of the git branch. null if not connected to git.
* @return saved application in DB
*/
@Override
Expand Down Expand Up @@ -878,7 +880,7 @@ private Mono<Application> importApplicationInWorkspace(String workspaceId,
List<DatasourceStorage> importedDatasourceList = importedDoc.getDatasourceList();
List<NewPage> importedNewPageList = importedDoc.getPageList();
List<NewAction> importedNewActionList = importedDoc.getActionList();
List<ActionCollection> importedActionCollectionList = CollectionUtils.isEmpty(importedDoc.getActionCollectionList())?
List<ActionCollection> importedActionCollectionList = CollectionUtils.isEmpty(importedDoc.getActionCollectionList()) ?
new ArrayList<>() : importedDoc.getActionCollectionList();

Mono<Workspace> workspaceMono = workspaceService.findById(workspaceId, permissionProvider.getRequiredPermissionOnTargetWorkspace())
Expand Down Expand Up @@ -979,7 +981,7 @@ private Mono<Application> importApplicationInWorkspace(String workspaceId,

// Since the resource is already present in DB, just update resource
Datasource existingDatasource = savedDatasourcesGitIdToDatasourceMap.get(datasourceStorage.getGitSyncId());
if(!permissionProvider.hasEditPermission(existingDatasource)) {
if (!permissionProvider.hasEditPermission(existingDatasource)) {
log.error("Trying to update datasource {} without edit permission", existingDatasource.getName());
return Mono.error(new AppsmithException(AppsmithError.ACL_NO_RESOURCE_FOUND, FieldName.DATASOURCE, existingDatasource.getId()));
}
Expand Down Expand Up @@ -1019,7 +1021,7 @@ private Mono<Application> importApplicationInWorkspace(String workspaceId,
workspace,
environmentId,
permissionProvider
);
);
});
})
.collectMap(Datasource::getName, Datasource::getId)
Expand Down Expand Up @@ -1053,13 +1055,7 @@ private Mono<Application> importApplicationInWorkspace(String workspaceId,
unpublishedPages.addAll(existingApplication.getPages());
return Mono.just(existingApplication);
}
importedApplication.setId(existingApplication.getId());
// For the existing application we don't need to default value of the flag
// The isPublic flag has a default value as false and this would be confusing to user
// when it is reset to false during importing where the application already is present in DB
importedApplication.setIsPublic(null);
importedApplication.setPolicies(null);
copyNestedNonNullProperties(importedApplication, existingApplication);
setPropertiesToExistingApplication(importedApplication, existingApplication);
// We are expecting the changes present in DB are committed to git directory
// so that these won't be lost when we are pulling changes from remote and
// rehydrate the application. We are now rehydrating the application with/without
Expand Down Expand Up @@ -1658,7 +1654,7 @@ private Mono<NewPage> mapActionAndCollectionIdWithPageLayout(NewPage newPage,
*
* @param existingDatasourceFlux already present datasource in the workspace
* @param datasourceStorage which will be checked against existing datasources
* @param workspace workspace where duplicate datasource should be checked
* @param workspace workspace where duplicate datasource should be checked
* @return already present or brand new datasource depending upon the equality check
*/
private Mono<Datasource> createUniqueDatasourceIfNotPresent(Flux<Datasource> existingDatasourceFlux,
Expand All @@ -1685,7 +1681,7 @@ private Mono<Datasource> createUniqueDatasourceIfNotPresent(Flux<Datasource> exi
.next() // Get the first matching datasource, we don't need more than one here.
.switchIfEmpty(Mono.defer(() -> {
// check if user has permission to create datasource
if(!permissionProvider.canCreateDatasource(workspace)) {
if (!permissionProvider.canCreateDatasource(workspace)) {
log.error("Unauthorized to create datasource: {} in workspace: {}", datasourceStorage.getName(), workspace.getName());
return Mono.error(new AppsmithException(AppsmithError.ACL_NO_RESOURCE_FOUND, FieldName.DATASOURCE, datasourceStorage.getName()));
}
Expand Down Expand Up @@ -1963,8 +1959,8 @@ private Mono<Map<String, String>> updateNewPagesBeforeMerge(Mono<List<NewPage>>
/**
* To send analytics event for import and export of application
*
* @param application Application object imported or exported
* @param event AnalyticsEvents event
* @param application Application object imported or exported
* @param event AnalyticsEvents event
* @return The application which is imported or exported
*/

Expand Down Expand Up @@ -1997,4 +1993,4 @@ private Mono<Application> sendImportExportApplicationAnalyticsEvent(String appli
return applicationService.findById(applicationId, Optional.empty())
.flatMap(application -> sendImportExportApplicationAnalyticsEvent(application, event));
}
}
}
Loading

0 comments on commit 081d417

Please sign in to comment.