Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

chore: refactor method names #37252

Closed
wants to merge 1 commit into from
+26 −36
Closed

chore: refactor method names #37252

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions ...r/appsmith-interfaces/src/main/java/com/appsmith/external/models/DatasourceStructure.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

import com.appsmith.external.exceptions.BaseException;
import com.appsmith.external.exceptions.ErrorDTO;
import com.fasterxml.jackson.annotation.JsonSubTypes;
import com.fasterxml.jackson.annotation.JsonTypeInfo;
import com.google.gson.InstanceCreator;
import lombok.AllArgsConstructor;
import lombok.Data;
Expand Down Expand Up @@ -60,6 +62,11 @@ public int compareTo(Column other) {
}
}

@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, property = "type")
@JsonSubTypes({
@JsonSubTypes.Type(value = DatasourceStructure.PrimaryKey.class, name = "primary key"),
@JsonSubTypes.Type(value = DatasourceStructure.ForeignKey.class, name = "foreign key")
})
public interface Key extends Comparable<Key> {
String getType();

Expand Down
2 changes: 1 addition & 1 deletion ...-server/src/main/java/com/appsmith/server/applications/base/ApplicationServiceCEImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -902,7 +902,7 @@ public String getRandomAppCardColor() {
@Override
public Mono<Integer> setAppTheme(
String applicationId, String editModeThemeId, String publishedModeThemeId, AclPermission aclPermission) {
return repository.setAppTheme(applicationId, editModeThemeId, publishedModeThemeId, aclPermission);
return repository.updateAppTheme(applicationId, editModeThemeId, publishedModeThemeId, aclPermission);
}

@Override
Expand Down
2 changes: 1 addition & 1 deletion ...th-server/src/main/java/com/appsmith/server/datasources/base/DatasourceServiceCEImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ public Mono<Datasource> create(Datasource datasource) {
@Override
public Mono<Datasource> createWithoutPermissions(
Datasource datasource, Map<DBOpsType, List<DatasourceStorage>> datasourceStorageDryRunQueries) {
return createEx(datasource, null, true, datasourceStorageDryRunQueries);
return createEx(datasource, null, false, datasourceStorageDryRunQueries);
}

@Override
Expand Down
2 changes: 1 addition & 1 deletion ...in/java/com/appsmith/server/datasources/importable/DatasourceImportableServiceCEImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,7 @@ private Mono<Map<String, String>> importDatasources(
// Don't update the datasource configuration for already available datasources
existingDatasource.setDatasourceConfiguration(null);
return datasourceService
.save(existingDatasource, true)
.save(existingDatasource, false)
.map(createdDatasource -> {
// Add dry run queries for the datasource
addDryOpsForEntity(
Expand Down
2 changes: 1 addition & 1 deletion ...c/main/java/com/appsmith/server/jslibs/importable/CustomJSLibImportableServiceCEImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ public Mono<Void> importEntities(
customJSLib.setCreatedAt(null);
customJSLib.setUpdatedAt(null);
return customJSLibService.persistCustomJSLibMetaDataIfDoesNotExistAndGetDTO(
customJSLib, false, mappedImportableResourcesDTO.getCustomJSLibsDryOps(), true);
customJSLib, false, mappedImportableResourcesDTO.getCustomJSLibsDryOps(), false);
})
.collectList()
.doOnNext(mappedImportableResourcesDTO::setInstalledJsLibsList)
Expand Down
2 changes: 1 addition & 1 deletion ...rver/src/main/java/com/appsmith/server/repositories/ce/CustomApplicationRepositoryCE.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ Mono<Application> getApplicationByGitBranchAndBaseApplicationId(

Flux<Application> getApplicationByGitBaseApplicationId(String baseApplicationId, AclPermission permission);

Mono<Integer> setAppTheme(
Mono<Integer> updateAppTheme(
String applicationId, String editModeThemeId, String publishedModeThemeId, AclPermission aclPermission);
Comment on lines +56 to 57
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Method rename needs to be propagated across the codebase

The interface method was renamed from setAppTheme to updateAppTheme, but this change hasn't been consistently applied across the codebase. Found inconsistencies:

  • ApplicationServiceCE still defines setAppTheme
  • ApplicationServiceCEImpl implements setAppTheme but calls updateAppTheme
  • Multiple services still call setAppTheme:
    • ThemeImportableServiceCEImpl
    • ApplicationPageServiceCEImpl
    • ApplicationForkingServiceCEImpl
🔗 Analysis chain

LGTM! Method rename improves clarity.

The rename from setAppTheme to updateAppTheme better reflects the operation's intent and aligns with enterprise naming conventions.

Let's verify the implementation class has been updated:

The results show both method names are still in use, but we need to see the actual context to determine if this is an issue or if they serve different purposes.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the implementation class uses the new method name
# Expect: Only occurrences of updateAppTheme, no setAppTheme

rg -l "setAppTheme" --type java
rg -l "updateAppTheme" --type java

Length of output: 1046

Script:

#!/bin/bash
# Let's check the actual content of these files to understand the usage
rg "setAppTheme|updateAppTheme" --type java -C 2

Length of output: 10139


Mono<Long> getGitConnectedApplicationWithPrivateRepoCount(String workspaceId);
Expand Down
2 changes: 1 addition & 1 deletion .../src/main/java/com/appsmith/server/repositories/ce/CustomApplicationRepositoryCEImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,7 @@ public Mono<Application> getApplicationByBaseApplicationIdAndDefaultBranch(Strin
}

@Override
public Mono<Integer> setAppTheme(
public Mono<Integer> updateAppTheme(
String applicationId, String editModeThemeId, String publishedModeThemeId, AclPermission aclPermission) {
BridgeUpdate updateObj = Bridge.update();
if (StringUtils.hasLength(editModeThemeId)) {
Expand Down
10 changes: 5 additions & 5 deletions ...ver/appsmith-server/src/main/java/com/appsmith/server/themes/base/ThemeServiceCEImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,15 +177,15 @@ public Mono<Theme> changeCurrentTheme(String newThemeId, String branchedApplicat
// customizations
return repository
.delete(currentTheme)
.then(applicationRepository.setAppTheme(
.then(applicationRepository.updateAppTheme(
application.getId(),
savedTheme.getId(),
null,
applicationPermission.getEditPermission()))
.thenReturn(savedTheme);
} else {
return applicationRepository
.setAppTheme(
.updateAppTheme(
application.getId(),
savedTheme.getId(),
null,
Expand Down Expand Up @@ -254,7 +254,7 @@ public Mono<Theme> publishTheme(String applicationId) {
.then(
// Set the system theme id as edit and published mode theme id to
// application object
applicationRepository.setAppTheme(
applicationRepository.updateAppTheme(
applicationId,
editModeTheme.getId(),
editModeTheme.getId(),
Expand Down Expand Up @@ -321,7 +321,7 @@ private Mono<Theme> saveThemeForApplication(
if (savedThemeTuple.getT2()) { // new theme created, update the application
if (applicationMode == ApplicationMode.EDIT) {
return applicationRepository
.setAppTheme(
.updateAppTheme(
application.getId(),
theme.getId(),
null,
Expand All @@ -331,7 +331,7 @@ private Mono<Theme> saveThemeForApplication(
.thenReturn(theme);
} else {
return applicationRepository
.setAppTheme(
.updateAppTheme(
application.getId(),
null,
theme.getId(),
Expand Down
33 changes: 8 additions & 25 deletions ...ver/src/main/java/com/appsmith/server/themes/importable/ThemeImportableServiceCEImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,14 +94,11 @@ public Mono<Void> importEntities(
// this will update the theme in the application and will be updated to db in the dry ops
// execution

Application application = new Application();
application.setPublishedModeThemeId(publishedModeThemeId);
application.setUnpublishedThemeId(editModeThemeId);
application.setId(importableArtifact.getId());

addDryOpsForApplication(
mappedImportableResourcesDTO.getApplicationDryRunQueries(), application);
return Mono.just(importableArtifact);
return applicationService.setAppTheme(
importableArtifact.getId(),
editModeThemeId,
publishedModeThemeId,
applicationPermission.getEditPermission());
AnaghHegde marked this conversation as resolved.
Show resolved Hide resolved
})
.then();
});
Expand All @@ -114,7 +111,7 @@ private Mono<Theme> updateExistingAppThemeFromJSON(
MappedImportableResourcesDTO mappedImportableResourcesDTO) {
if (!StringUtils.hasLength(existingThemeId)) {
return themeService
.getOrSaveTheme(themeFromJson, (Application) destinationArtifact, true)
.getOrSaveTheme(themeFromJson, (Application) destinationArtifact, false)
.map(createdTheme -> {
addDryOpsForEntity(
DBOpsType.SAVE, mappedImportableResourcesDTO.getThemeDryRunQueries(), createdTheme);
Expand All @@ -128,7 +125,7 @@ private Mono<Theme> updateExistingAppThemeFromJSON(
.flatMap(existingTheme -> {
if (!StringUtils.hasLength(existingTheme.getId()) || existingTheme.isSystemTheme()) {
return themeService
.getOrSaveTheme(themeFromJson, (Application) destinationArtifact, true)
.getOrSaveTheme(themeFromJson, (Application) destinationArtifact, false)
.map(createdTheme -> {
addDryOpsForEntity(
DBOpsType.SAVE,
Expand All @@ -139,7 +136,7 @@ private Mono<Theme> updateExistingAppThemeFromJSON(
} else {
if (themeFromJson.isSystemTheme()) {
return themeService
.getOrSaveTheme(themeFromJson, (Application) destinationArtifact, true)
.getOrSaveTheme(themeFromJson, (Application) destinationArtifact, false)
.flatMap(importedTheme -> {
// need to delete the old existingTheme
addDryOpsForEntity(
Expand All @@ -154,10 +151,6 @@ private Mono<Theme> updateExistingAppThemeFromJSON(
});
} else {
themeFromJson.setId(existingThemeId);
addDryOpsForEntity(
DBOpsType.UPDATE,
mappedImportableResourcesDTO.getThemeDryRunQueries(),
themeFromJson);
return Mono.just(themeFromJson);
}
}
Expand Down Expand Up @@ -189,14 +182,4 @@ private void addDryOpsForEntity(DBOpsType queryType, Map<String, List<Theme>> dr
dryRunOpsMap.put(queryType.name(), themes);
}
}

private void addDryOpsForApplication(Map<String, List<Application>> dryRunOpsMap, Application application) {
if (dryRunOpsMap.containsKey(DBOpsType.UPDATE.name())) {
dryRunOpsMap.get(DBOpsType.UPDATE.name()).add(application);
} else {
List<Application> applicationList = new ArrayList<>();
applicationList.add(application);
dryRunOpsMap.put(DBOpsType.UPDATE.name(), applicationList);
}
}
}
Loading