chore: Rename base-getById where appropriate (#33253)

The `getById` method in `CrudService`/`BaseService` gets an item from
the DB without checking for permissions. But a few services
(`Application` and `Workspace`) have overridden this method to run the
query _with_ permission check. This gives the false impression that this
method has a permission check for _all_ services, which is not the case.

So we're renaming the base method to `getByIdWithoutPermissionCheck`,
and make the overridden versions as `getById`. This should make it a lot
more obvious where we're querying with permissions and where we're
ignoring them, and make an informed choice of when what is needed.

## Review tips

1. The new `getById` does a permission check. The new
`getByIdWithoutPermissionCheck` doesn't do a permission check.
2. Since only calls to `getById` for the application and workspace
service were using permission check, we need to ensure that:
1. All calls to `getById` on application service and workspace service,
are untouched.
2. All calls to `getById` on any other service are changed to
`getByIdWithoutPermissionCheck`. Any remaining call to `getById` would
throw a compile error since we removed it from `BaseService` anyway.


EE PR at https://github.com/appsmithorg/appsmith-ee/pull/4136.

/ok-to-test tags="@tag.All"

<!-- This is an auto-generated comment: Cypress test results  -->
> [!TIP]
> 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
> Workflow run:
<https://github.com/appsmithorg/appsmith/actions/runs/9111401344>
> Commit: 96df2feee613a0e3628f6ec9f91313c2c4b84360
> Cypress dashboard url: <a
href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=9111401344&attempt=1"
target="_blank">Click here!</a>

<!-- end of auto-generated comment: Cypress test results  -->
This commit is contained in:
Shrikant Sharat Kandula 2024-05-16 19:37:01 +05:30 committed by GitHub
parent a712a4e27e
commit 8aeb900a1c
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
22 changed files with 64 additions and 47 deletions

View File

@ -27,6 +27,8 @@ public interface ActionCollectionServiceCE extends CrudService<ActionCollection,
Flux<ActionCollection> saveAll(List<ActionCollection> collections);
Mono<ActionCollection> findByIdAndBranchName(String id, String branchName);
Flux<ActionCollectionDTO> getPopulatedActionCollectionsByViewMode(
MultiValueMap<String, String> params, Boolean viewMode);

View File

@ -17,6 +17,10 @@ import java.util.Optional;
public interface ApplicationServiceCE extends CrudService<Application, String> {
Mono<Application> getById(String id);
Mono<Application> findByIdAndBranchName(String id, String branchName);
Mono<Application> findByIdAndBranchName(String id, List<String> projectionFieldNames, String branchName);
Mono<Application> findById(String id);

View File

@ -75,7 +75,7 @@ public class UpdateLayoutServiceCEImpl implements UpdateLayoutServiceCE {
boolean isSuccess,
Throwable error,
CreatorContextType creatorType) {
return Mono.zip(sessionUserService.getCurrentUser(), newPageService.getById(creatorId))
return Mono.zip(sessionUserService.getCurrentUser(), newPageService.getByIdWithoutPermissionCheck(creatorId))
.flatMap(tuple -> {
User t1 = tuple.getT1();
NewPage t2 = tuple.getT2();

View File

@ -29,6 +29,8 @@ public interface NewActionServiceCE extends CrudService<NewAction, String> {
void setCommonFieldsFromActionDTOIntoNewAction(ActionDTO action, NewAction newAction);
Mono<NewAction> findByIdAndBranchName(String id, String branchName);
ActionDTO generateActionByViewMode(NewAction newAction, Boolean viewMode);
void generateAndSetActionPolicies(NewPage page, NewAction action);

View File

@ -653,7 +653,7 @@ public class NewActionServiceCEImpl extends BaseService<NewActionRepository, New
}
private Mono<NewAction> extractAndSetNativeQueryFromFormData(NewAction action) {
Mono<Plugin> pluginMono = pluginService.getById(action.getPluginId());
Mono<Plugin> pluginMono = pluginService.getByIdWithoutPermissionCheck(action.getPluginId());
Mono<PluginExecutor> pluginExecutorMono = pluginExecutorHelper.getPluginExecutor(pluginMono);
return pluginExecutorMono
@ -1175,7 +1175,7 @@ public class NewActionServiceCEImpl extends BaseService<NewActionRepository, New
@Override
public Mono<ActionDTO> fillSelfReferencingDataPaths(ActionDTO actionDTO) {
Mono<Plugin> pluginMono = pluginService.getById(actionDTO.getPluginId());
Mono<Plugin> pluginMono = pluginService.getByIdWithoutPermissionCheck(actionDTO.getPluginId());
Mono<PluginExecutor> pluginExecutorMono = pluginExecutorHelper.getPluginExecutor(pluginMono);
return pluginExecutorMono.map(pluginExecutor -> {

View File

@ -30,6 +30,8 @@ public interface NewPageServiceCE extends CrudService<NewPage, String> {
Flux<NewPage> findNewPagesByApplicationId(String applicationId, AclPermission permission);
Mono<NewPage> findByIdAndBranchName(String id, String branchName);
Mono<PageDTO> saveUnpublishedPage(PageDTO page);
Mono<PageDTO> createDefault(PageDTO object);

View File

@ -204,7 +204,7 @@ public class RefactoringServiceCEImpl implements RefactoringServiceCE {
Map<String, String> analyticsProperties) {
return contextIdMono.flatMap(branchedPageId -> {
refactorEntityNameDTO.setPageId(branchedPageId);
return newPageService.getById(branchedPageId).map(page -> {
return newPageService.getByIdWithoutPermissionCheck(branchedPageId).map(page -> {
analyticsProperties.put(FieldName.APPLICATION_ID, page.getApplicationId());
analyticsProperties.put(FieldName.PAGE_ID, refactorEntityNameDTO.getPageId());
return analyticsProperties;

View File

@ -62,7 +62,7 @@ public abstract class BaseService<
}
@Override
public Mono<T> getById(ID id) {
public Mono<T> getByIdWithoutPermissionCheck(ID id) {
if (id == null) {
return Mono.error(new AppsmithException(AppsmithError.INVALID_PARAMETER, FieldName.ID));
}

View File

@ -16,11 +16,7 @@ public interface CrudService<T extends BaseDomain, ID> {
Mono<T> update(ID id, T resource);
Mono<T> getById(ID id);
default Mono<T> findByIdAndBranchName(ID id, String branchName) {
return this.getById(id);
}
Mono<T> getByIdWithoutPermissionCheck(ID id);
Map<String, Object> getAnalyticsProperties(T savedResource);

View File

@ -78,7 +78,7 @@ public class LayoutActionServiceCEImpl implements LayoutActionServiceCE {
} else if (action.getCollectionId().length() == 0) {
// The Action has been removed from existing collection.
return newActionService
.getById(id)
.getByIdWithoutPermissionCheck(id)
.flatMap(action1 -> collectionService.removeSingleActionFromCollection(
action1.getUnpublishedAction().getCollectionId(), Mono.just(action1)))
.flatMap(action1 -> {
@ -93,7 +93,7 @@ public class LayoutActionServiceCEImpl implements LayoutActionServiceCE {
// collection.
// Remove the action from previous collection and add it to the new collection.
return newActionService
.getById(id)
.getByIdWithoutPermissionCheck(id)
.flatMap(action1 -> {
if (action1.getUnpublishedAction().getCollectionId() != null) {
return collectionService.removeSingleActionFromCollection(

View File

@ -21,6 +21,8 @@ public interface WorkspaceServiceCE extends CrudService<Workspace, String> {
Mono<Workspace> create(Workspace workspace, User user, Boolean isDefault);
Mono<Workspace> getById(String id);
Mono<Workspace> findById(String id, AclPermission permission);
Mono<Workspace> findById(String id, Optional<AclPermission> permission);

View File

@ -1000,7 +1000,7 @@ public class ActionExecutionSolutionCEImpl implements ActionExecutionSolutionCE
Mono.just(application),
sessionUserService.getCurrentUser(),
newPageService.getNameByPageId(actionDTO.getPageId(), executeActionDto.getViewMode()),
pluginService.getById(actionDTO.getPluginId()),
pluginService.getByIdWithoutPermissionCheck(actionDTO.getPluginId()),
datasourceStorageService.getEnvironmentNameFromEnvironmentIdForAnalytics(
datasourceStorage.getEnvironmentId())))
.flatMap(tuple -> {

View File

@ -61,7 +61,7 @@ public class ThemeServiceCEImpl extends BaseService<ThemeRepository, Theme, Stri
}
@Override
public Mono<Theme> getById(String s) {
public Mono<Theme> getByIdWithoutPermissionCheck(String s) {
// we don't allow to get a theme by id from DB
throw new AppsmithException(AppsmithError.UNSUPPORTED_OPERATION);
}

View File

@ -1952,7 +1952,7 @@ public class ImportServiceTests {
action.setPageId(application.getPages().get(0).getId());
return layoutActionService.createAction(action);
})
.flatMap(actionDTO -> newActionService.getById(actionDTO.getId()))
.flatMap(actionDTO -> newActionService.getByIdWithoutPermissionCheck(actionDTO.getId()))
.flatMap(newAction -> applicationRepository.findById(newAction.getApplicationId()))
.cache();
@ -2055,7 +2055,8 @@ public class ImportServiceTests {
return layoutCollectionService.createCollection(actionCollectionDTO1, null);
})
.flatMap(actionCollectionDTO -> actionCollectionService.getById(actionCollectionDTO.getId()))
.flatMap(actionCollectionDTO ->
actionCollectionService.getByIdWithoutPermissionCheck(actionCollectionDTO.getId()))
.flatMap(actionCollection -> applicationRepository.findById(actionCollection.getApplicationId()))
.cache();

View File

@ -168,7 +168,8 @@ class RefactoringServiceCEImplTest {
layout1.setId("testLayoutId");
layout1.setDsl(jsonObject);
pageDTO.setLayouts(List.of(layout1));
Mockito.when(newPageService.getById(Mockito.anyString())).thenReturn(Mono.just(newPage));
Mockito.when(newPageService.getByIdWithoutPermissionCheck(Mockito.anyString()))
.thenReturn(Mono.just(newPage));
Mockito.when(newPageService.findPageById(Mockito.anyString(), Mockito.any(), Mockito.anyBoolean()))
.thenReturn(Mono.just(pageDTO));
@ -242,7 +243,8 @@ class RefactoringServiceCEImplTest {
newPage.setUnpublishedPage(pageDTO);
Mockito.when(newPageService.findPageById(Mockito.anyString(), Mockito.any(), Mockito.anyBoolean()))
.thenReturn(Mono.just(pageDTO));
Mockito.when(newPageService.getById(Mockito.anyString())).thenReturn(Mono.just(newPage));
Mockito.when(newPageService.getByIdWithoutPermissionCheck(Mockito.anyString()))
.thenReturn(Mono.just(newPage));
final Mono<LayoutDTO> layoutDTOMono =
refactoringServiceCE.refactorEntityName(refactorActionCollectionNameDTO, null);
@ -295,7 +297,8 @@ class RefactoringServiceCEImplTest {
layout1.setId("testLayoutId");
layout1.setDsl(new JSONObject());
pageDTO.setLayouts(List.of(layout1));
Mockito.when(newPageService.getById(Mockito.anyString())).thenReturn(Mono.just(newPage));
Mockito.when(newPageService.getByIdWithoutPermissionCheck(Mockito.anyString()))
.thenReturn(Mono.just(newPage));
Mockito.when(newPageService.findPageById(Mockito.anyString(), Mockito.any(), Mockito.anyBoolean()))
.thenReturn(Mono.just(pageDTO));

View File

@ -812,7 +812,7 @@ class RefactoringServiceCETest {
assert createdActionCollectionDTO1 != null;
final Mono<ActionCollection> actionCollectionMono =
actionCollectionService.getById(createdActionCollectionDTO1.getId());
actionCollectionService.getByIdWithoutPermissionCheck(createdActionCollectionDTO1.getId());
final Mono<NewAction> actionMono = newActionService
.findByCollectionIdAndViewMode(createdActionCollectionDTO1.getId(), false, null)
.next();
@ -881,7 +881,7 @@ class RefactoringServiceCETest {
final Mono<Tuple2<ActionCollection, NewAction>> tuple2Mono = refactoringService
.refactorEntityName(refactorActionNameInCollectionDTO, null)
.then(actionCollectionService
.getById(dto.getId())
.getByIdWithoutPermissionCheck(dto.getId())
.zipWith(newActionService.findById(
dto.getActions().get(0).getId())));

View File

@ -467,7 +467,7 @@ public class ActionCollectionServiceTest {
assert createdActionCollectionDTO2 != null;
final Mono<ActionCollection> actionCollectionMono =
actionCollectionService.getById(createdActionCollectionDTO2.getId());
actionCollectionService.getByIdWithoutPermissionCheck(createdActionCollectionDTO2.getId());
StepVerifier.create(actionCollectionMono)
.assertNext(actionCollection -> {
@ -477,10 +477,11 @@ public class ActionCollectionServiceTest {
})
.verifyComplete();
final Mono<NewAction> actionMono = newActionService.getById(createdActionCollectionDTO2.getActions().stream()
.findFirst()
.get()
.getId());
final Mono<NewAction> actionMono =
newActionService.getByIdWithoutPermissionCheck(createdActionCollectionDTO2.getActions().stream()
.findFirst()
.get()
.getId());
StepVerifier.create(actionMono)
.assertNext(action -> {
@ -561,7 +562,7 @@ public class ActionCollectionServiceTest {
assert createdActionCollectionDTO2 != null;
final Mono<ActionCollection> actionCollectionMono =
actionCollectionService.getById(createdActionCollectionDTO2.getId());
actionCollectionService.getByIdWithoutPermissionCheck(createdActionCollectionDTO2.getId());
StepVerifier.create(actionCollectionMono)
.assertNext(actionCollection -> {
@ -571,10 +572,11 @@ public class ActionCollectionServiceTest {
})
.verifyComplete();
final Mono<NewAction> actionMono = newActionService.getById(createdActionCollectionDTO2.getActions().stream()
.findFirst()
.get()
.getId());
final Mono<NewAction> actionMono =
newActionService.getByIdWithoutPermissionCheck(createdActionCollectionDTO2.getActions().stream()
.findFirst()
.get()
.getId());
StepVerifier.create(actionMono)
.assertNext(action -> {

View File

@ -190,8 +190,8 @@ public class ApplicationPageServiceTest {
String uuid = UUID.randomUUID().toString();
NewPage newPage = createApplication("App_" + uuid)
.flatMap(application ->
newPageService.getById(application.getPages().get(0).getId()))
.flatMap(application -> newPageService.getByIdWithoutPermissionCheck(
application.getPages().get(0).getId()))
.block();
// mock the dsMigrationUtils to return the current DSL version as the latest DSL version
@ -220,8 +220,8 @@ public class ApplicationPageServiceTest {
public void getPageAndMigrateDslByBranchAndDefaultPageId_WhenEditModeDslIsNotLatest_EditModeDslMigrated() {
String uuid = UUID.randomUUID().toString();
NewPage newPage = createApplication("App_" + uuid)
.flatMap(application ->
newPageService.getById(application.getPages().get(0).getId()))
.flatMap(application -> newPageService.getByIdWithoutPermissionCheck(
application.getPages().get(0).getId()))
.block();
// mock the dsMigrationUtils to return the (current DSL version-1) as the latest DSL version
@ -246,7 +246,7 @@ public class ApplicationPageServiceTest {
Mono<NewPage> newPageMono = applicationPageService
.getPageAndMigrateDslByBranchAndDefaultPageId(newPage.getId(), null, false, true)
.then(newPageService.getById(newPage.getId()));
.then(newPageService.getByIdWithoutPermissionCheck(newPage.getId()));
StepVerifier.create(newPageMono)
.assertNext(newpage -> {
@ -279,8 +279,8 @@ public class ApplicationPageServiceTest {
public void getPageAndMigrateDslByBranchAndDefaultPageId_WhenDSLHasNotVersion_DslMigratedToLatest() {
String uuid = UUID.randomUUID().toString();
NewPage newPage = createApplication("App_" + uuid)
.flatMap(application ->
newPageService.getById(application.getPages().get(0).getId()))
.flatMap(application -> newPageService.getByIdWithoutPermissionCheck(
application.getPages().get(0).getId()))
.flatMap(page -> {
Layout layout = page.getUnpublishedPage().getLayouts().get(0);
JSONObject unpublishedDsl = layout.getDsl();
@ -306,7 +306,7 @@ public class ApplicationPageServiceTest {
Mono<NewPage> newPageMono = applicationPageService
.getPageAndMigrateDslByBranchAndDefaultPageId(newPage.getId(), null, false, true)
.then(newPageService.getById(newPage.getId()));
.then(newPageService.getByIdWithoutPermissionCheck(newPage.getId()));
StepVerifier.create(newPageMono)
.assertNext(newpage -> {
@ -328,8 +328,8 @@ public class ApplicationPageServiceTest {
public void getPageAndMigrateDslByBranchAndDefaultPageId_WhenViewModeDslIsNotLatest_ViewModeDslMigrated() {
String uuid = UUID.randomUUID().toString();
NewPage newPage = createApplication("App_" + uuid)
.flatMap(application ->
newPageService.getById(application.getPages().get(0).getId()))
.flatMap(application -> newPageService.getByIdWithoutPermissionCheck(
application.getPages().get(0).getId()))
.block();
// mock the dsMigrationUtils to return the (current DSL version-1) as the latest DSL version
@ -354,7 +354,7 @@ public class ApplicationPageServiceTest {
Mono<NewPage> newPageMono = applicationPageService
.getPageAndMigrateDslByBranchAndDefaultPageId(newPage.getId(), null, true, true)
.then(newPageService.getById(newPage.getId()));
.then(newPageService.getByIdWithoutPermissionCheck(newPage.getId()));
StepVerifier.create(newPageMono)
.assertNext(newpage -> {

View File

@ -422,7 +422,8 @@ public class CurlImporterServiceTest {
command, null, page.getId(), "actionName", workspaceId, "main"))
.cache();
Mono<NewAction> savedActionMono = resultMono.flatMap(actionDTO -> newActionService.getById(actionDTO.getId()));
Mono<NewAction> savedActionMono =
resultMono.flatMap(actionDTO -> newActionService.getByIdWithoutPermissionCheck(actionDTO.getId()));
StepVerifier.create(Mono.zip(resultMono, defaultPageMono, savedActionMono))
.assertNext(tuple -> {

View File

@ -305,7 +305,8 @@ public class PageServiceTest {
.verifyComplete();
// Check if defaultResources
Mono<NewPage> newPageMono = pageMono.flatMap(pageDTO -> newPageService.getById(pageDTO.getId()));
Mono<NewPage> newPageMono =
pageMono.flatMap(pageDTO -> newPageService.getByIdWithoutPermissionCheck(pageDTO.getId()));
StepVerifier.create(newPageMono)
.assertNext(newPage -> {
assertThat(newPage.getDefaultResources()).isNotNull();

View File

@ -496,8 +496,9 @@ public class UserDataServiceTest {
Mono<GitProfile> gitConfigMono = gitService.getDefaultGitProfileOrCreateIfEmpty();
Mono<User> userData =
userDataService.getForCurrentUser().flatMap(userData1 -> userService.getById(userData1.getUserId()));
Mono<User> userData = userDataService
.getForCurrentUser()
.flatMap(userData1 -> userService.getByIdWithoutPermissionCheck(userData1.getUserId()));
StepVerifier.create(gitConfigMono.zipWhen(gitProfile -> userData))
.assertNext(tuple -> {

View File

@ -310,7 +310,7 @@ class TenantServiceCETest {
.verify();
// Verify that the tenant is updated for the feature flag migration failure
StepVerifier.create(tenantService.getById(tenant.getId()))
StepVerifier.create(tenantService.getByIdWithoutPermissionCheck(tenant.getId()))
.assertNext(updatedTenant -> {
assertThat(updatedTenant.getTenantConfiguration().getFeaturesWithPendingMigration())
.hasSize(1);