Revert "fix: fixed failing queries using aggregation pipeline (#26132)" #1
Conversation
…thorg#26132)" This reverts commit 66d5027.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
15 file(s) reviewed, 8 comment(s)
Edit PR Review Bot Settings | Greptile
| return mongoOperations.find( | ||
| query.cursorBatchSize(10000), | ||
| entityInformation.getJavaType(), | ||
| entityInformation.getCollectionName()); | ||
| query, entityInformation.getJavaType(), entityInformation.getCollectionName()); |
There was a problem hiding this comment.
logic: Removing cursorBatchSize(10000) could cause performance issues with large result sets. Consider keeping the batch size configuration to prevent potential memory issues.
| @@ -78,7 +78,7 @@ Flux<NewAction> findAllNonJsActionsByNameAndPageIdsAndViewMode( | |||
|
|
|||
| Mono<List<BulkWriteResult>> bulkUpdate(List<NewAction> newActions); | |||
|
|
|||
| Mono<List<BulkWriteResult>> publishActions(String applicationId, AclPermission permission); | |||
| Mono<UpdateResult> publishActions(String applicationId, AclPermission permission); | |||
There was a problem hiding this comment.
logic: Return type change from Mono<List> to Mono> means we lose granular information about individual write operations. UpdateResult only provides aggregate success/failure, while BulkWriteResult provided details for each operation.
| Mono<List<BulkWriteResult>> publishPages(Collection<String> pageIds, AclPermission permission); | ||
|
|
||
| Mono<List<BulkWriteResult>> bulkUpdate(List<NewPage> newPages); | ||
| Mono<UpdateResult> publishPages(Collection<String> pageIds, AclPermission permission); |
There was a problem hiding this comment.
logic: Changing from BulkWriteResult to UpdateResult means losing per-document update status information. Ensure this doesn't break error handling for partial page publish failures.
| public Mono<UpdateResult> publishPages(Collection<String> pageIds, AclPermission permission) { | ||
| Criteria applicationIdCriteria = where(fieldName(QNewPage.newPage.id)).in(pageIds); | ||
| // using aggregation update instead of regular update here | ||
| // it's required to set a field to a value of another field from the same domain | ||
| AggregationUpdate aggregationUpdate = AggregationUpdate.update() | ||
| .set(fieldName(QNewPage.newPage.publishedPage)) | ||
| .toValue("$" + fieldName(QNewPage.newPage.unpublishedPage)); | ||
|
|
||
| Mono<Set<String>> permissionGroupsMono = | ||
| getCurrentUserPermissionGroupsIfRequired(Optional.ofNullable(permission)); | ||
|
|
||
| return permissionGroupsMono.flatMap(permissionGroups -> { | ||
| AggregationOperation matchAggregationWithPermission = null; | ||
| if (permission == null) { | ||
| matchAggregationWithPermission = Aggregation.match(new Criteria().andOperator(notDeleted())); | ||
| } else { | ||
| matchAggregationWithPermission = Aggregation.match( | ||
| new Criteria().andOperator(notDeleted(), userAcl(permissionGroups, permission))); | ||
| } | ||
| AggregationOperation matchAggregation = Aggregation.match(applicationIdCriteria); | ||
| AggregationOperation wholeProjection = Aggregation.project(NewPage.class); | ||
| AggregationOperation addFieldsOperation = Aggregation.addFields() | ||
| .addField(fieldName(QNewPage.newPage.publishedPage)) | ||
| .withValueOf(Fields.field(fieldName(QNewPage.newPage.unpublishedPage))) | ||
| .build(); | ||
| Aggregation combinedAggregation = Aggregation.newAggregation( | ||
| matchAggregation, matchAggregationWithPermission, wholeProjection, addFieldsOperation); | ||
| AggregationResults<NewPage> updatedResults = | ||
| mongoTemplate.aggregate(combinedAggregation, NewPage.class, NewPage.class); | ||
| return bulkUpdate(updatedResults.getMappedResults()); | ||
| }); | ||
| } | ||
|
|
||
| @Override | ||
| public Mono<List<BulkWriteResult>> bulkUpdate(List<NewPage> newPages) { | ||
| if (CollectionUtils.isEmpty(newPages)) { | ||
| return Mono.just(Collections.emptyList()); | ||
| } | ||
|
|
||
| // convert the list of new pages to a list of DBObjects | ||
| List<WriteModel<Document>> dbObjects = newPages.stream() | ||
| .map(newPage -> { | ||
| assert newPage.getId() != null; | ||
| Document document = new Document(); | ||
| mongoOperations.getConverter().write(newPage, document); | ||
| document.remove("_id"); | ||
| return (WriteModel<Document>) new UpdateOneModel<Document>( | ||
| new Document("_id", new ObjectId(newPage.getId())), new Document("$set", document)); | ||
| }) | ||
| .collect(Collectors.toList()); | ||
|
|
||
| return mongoOperations | ||
| .getCollection(mongoOperations.getCollectionName(NewPage.class)) | ||
| .flatMapMany(documentMongoCollection -> documentMongoCollection.bulkWrite(dbObjects)) | ||
| .collectList(); | ||
| return updateByCriteria(List.of(applicationIdCriteria), aggregationUpdate, permission); | ||
| } |
There was a problem hiding this comment.
logic: Moving from bulk writes to individual updates could significantly impact performance when publishing multiple pages simultaneously. Consider keeping bulk operations or documenting why they were problematic.
| AggregationUpdate aggregationUpdate = AggregationUpdate.update() | ||
| .set(fieldName(QNewPage.newPage.publishedPage)) | ||
| .toValue("$" + fieldName(QNewPage.newPage.unpublishedPage)); |
There was a problem hiding this comment.
style: Using $-prefixed field references in aggregation updates can be fragile if field names change. Consider using a more robust field reference mechanism.
| Mono<UpdateResult> publishPagesMono = | ||
| newPageService.publishPages(editedPageIds, pagePermission.getEditPermission()); |
There was a problem hiding this comment.
logic: Reverting from BulkWriteResult to UpdateResult may reduce performance by not using MongoDB's bulk operation capabilities. Consider keeping bulk operations if possible.
| public Mono<UpdateResult> publishActions(String applicationId, AclPermission permission) { | ||
| // delete the actions that were deleted in edit mode |
There was a problem hiding this comment.
logic: Return type changed from List to UpdateResult - ensure all callers handle the new return type correctly
| @@ -93,5 +93,5 @@ Mono<NewPage> findByGitSyncIdAndDefaultApplicationId( | |||
|
|
|||
| Flux<NewPage> findPageSlugsByApplicationIds(List<String> applicationIds, AclPermission aclPermission); | |||
|
|
|||
| Mono<List<BulkWriteResult>> publishPages(Collection<String> pageIds, AclPermission permission); | |||
| Mono<UpdateResult> publishPages(Collection<String> pageIds, AclPermission permission); | |||
There was a problem hiding this comment.
logic: Changing return type from BulkWriteResult to UpdateResult means losing granular information about individual write operations. UpdateResult only provides aggregate counts, while BulkWriteResult includes details about each modified document. Verify this doesn't break any code that needs per-document results.
This reverts commit 66d5027.
Description
Tip
Add a TL;DR when the description is longer than 500 words or extremely technical (helps the content, marketing, and DevRel team).
Please also include relevant motivation and context. List any dependencies that are required for this change. Add links to Notion, Figma or any other documents that might be relevant to the PR.
Fixes #
Issue Numberor
Fixes
Issue URLWarning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
Automation
/ok-to-test tags=""
🔍 Cypress test results
Caution
If you modify the content in this section, you are likely to disrupt the CI result for your PR.
Communication
Should the DevRel and Marketing teams inform users about this change?
Greptile Summary
Based on the provided information, I'll create a comprehensive summary of this pull request that reverts changes related to MongoDB query implementations.
This reversion of PR appsmithorg#26132 makes significant changes to MongoDB query handling across multiple repository and service layers:
BulkWriteResulttoUpdateResultfor operation feedbackMongoTemplatedependencies from several repository implementationsKey Impact Points:
BulkWriteResulttoUpdateResultThis appears to be addressing compatibility issues with DocumentDB, though care should be taken to monitor query performance and data integrity after these changes.