From a35a3dddd3ffc2085e31ee7728043e02634dbb5f Mon Sep 17 00:00:00 2001 From: Sriharsha Chintalapani Date: Mon, 30 Sep 2024 16:24:13 -0700 Subject: [PATCH 1/2] Minor: Search RBAC, fix condition evaluation for single policy multiple rules --- .../security/RBACConditionEvaluator.java | 95 ++++++++--- ...asticSearchRBACConditionEvaluatorTest.java | 153 ++++++++++++++---- 2 files changed, 193 insertions(+), 55 deletions(-) diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/search/security/RBACConditionEvaluator.java b/openmetadata-service/src/main/java/org/openmetadata/service/search/security/RBACConditionEvaluator.java index 6bcb33bc8748..e0be841525a0 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/search/security/RBACConditionEvaluator.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/search/security/RBACConditionEvaluator.java @@ -26,7 +26,7 @@ public class RBACConditionEvaluator { private final ExpressionParser spelParser = new SpelExpressionParser(); private final StandardEvaluationContext spelContext; private static final Set SEARCH_RELEVANT_OPS = - Set.of(MetadataOperation.VIEW_BASIC, MetadataOperation.VIEW_ALL); + Set.of(MetadataOperation.VIEW_BASIC, MetadataOperation.VIEW_ALL, MetadataOperation.ALL); public RBACConditionEvaluator(QueryBuilderFactory queryBuilderFactory) { this.queryBuilderFactory = queryBuilderFactory; @@ -36,34 +36,85 @@ public RBACConditionEvaluator(QueryBuilderFactory queryBuilderFactory) { public OMQueryBuilder evaluateConditions(SubjectContext subjectContext) { User user = subjectContext.user(); spelContext.setVariable("user", user); - ConditionCollector collector = new ConditionCollector(queryBuilderFactory); + + ConditionCollector finalCollector = new ConditionCollector(queryBuilderFactory); for (Iterator it = subjectContext.getPolicies(List.of(user.getEntityReference())); it.hasNext(); ) { SubjectContext.PolicyContext context = it.next(); + + ConditionCollector policyCollector = new ConditionCollector(queryBuilderFactory); + List allowRuleQueries = new ArrayList<>(); + List denyRuleQueries = new ArrayList<>(); + for (CompiledRule rule : context.getRules()) { - if (rule.getCondition() != null - && rule.getOperations().stream().anyMatch(SEARCH_RELEVANT_OPS::contains)) { - ConditionCollector ruleCollector = new ConditionCollector(queryBuilderFactory); - if (!rule.getResources().isEmpty() && !rule.getResources().contains("All")) { - OMQueryBuilder indexFilter = getIndexFilter(rule.getResources()); - ruleCollector.addMust(indexFilter); - } - SpelExpression parsedExpression = - (SpelExpression) spelParser.parseExpression(rule.getCondition()); - preprocessExpression(parsedExpression.getAST(), ruleCollector); - OMQueryBuilder ruleQuery = ruleCollector.buildFinalQuery(); - if (rule.getEffect().toString().equalsIgnoreCase("DENY")) { - collector.addMustNot(ruleQuery); - } else { - collector.addMust(ruleQuery); - } + boolean isDenyRule = rule.getEffect().toString().equalsIgnoreCase("DENY"); + + if (isDenyRule && SEARCH_RELEVANT_OPS.stream().noneMatch(rule.getOperations()::contains)) { + continue; + } + + OMQueryBuilder ruleQuery = buildRuleQuery(rule, user); + if (ruleQuery == null || ruleQuery.isEmpty()) { + continue; + } + if (isDenyRule) { + denyRuleQueries.add(ruleQuery); + } else { + allowRuleQueries.add(ruleQuery); } } + + if (!denyRuleQueries.isEmpty()) { + if (denyRuleQueries.size() == 1) { + policyCollector.addMustNot(denyRuleQueries.get(0)); + } else { + OMQueryBuilder denyQuery = queryBuilderFactory.boolQuery().should(denyRuleQueries); + policyCollector.addMustNot(denyQuery); + } + } + + if (!allowRuleQueries.isEmpty()) { + if (allowRuleQueries.size() == 1) { + policyCollector.addMust(allowRuleQueries.get(0)); + } else { + OMQueryBuilder allowQuery = queryBuilderFactory.boolQuery().should(allowRuleQueries); + policyCollector.addMust(allowQuery); + } + } else { + policyCollector.addMust(queryBuilderFactory.matchAllQuery()); + } + + OMQueryBuilder policyFinalQuery = policyCollector.buildFinalQuery(); + if (policyFinalQuery != null && !policyFinalQuery.isEmpty()) { + finalCollector.addMust(policyFinalQuery); + } + } + + return finalCollector.buildFinalQuery(); + } + + private OMQueryBuilder buildRuleQuery(CompiledRule rule, User user) { + ConditionCollector ruleCollector = new ConditionCollector(queryBuilderFactory); + spelContext.setVariable("user", user); + + // Apply index filtering if resources are specified and not "All" + if (!rule.getResources().isEmpty() && !rule.getResources().contains("All")) { + OMQueryBuilder indexFilter = getIndexFilter(rule.getResources()); + ruleCollector.addMust(indexFilter); + } + + if (rule.getCondition() != null && !rule.getCondition().trim().isEmpty()) { + SpelExpression parsedExpression = + (SpelExpression) spelParser.parseExpression(rule.getCondition()); + preprocessExpression(parsedExpression.getAST(), ruleCollector); + } else { + // If the condition is empty, treat it as always true + ruleCollector.addMust(queryBuilderFactory.matchAllQuery()); } - return collector.buildFinalQuery(); + return ruleCollector.buildFinalQuery(); } private void preprocessExpression(SpelNode node, ConditionCollector collector) { @@ -187,11 +238,11 @@ public void matchAllTags(List tags, ConditionCollector collector) { public void isOwner(User user, ConditionCollector collector) { List ownerQueries = new ArrayList<>(); - ownerQueries.add(queryBuilderFactory.termQuery("owner.id", user.getId().toString())); + ownerQueries.add(queryBuilderFactory.termQuery("owners.id", user.getId().toString())); if (user.getTeams() != null) { for (EntityReference team : user.getTeams()) { - ownerQueries.add(queryBuilderFactory.termQuery("owner.id", team.getId().toString())); + ownerQueries.add(queryBuilderFactory.termQuery("owners.id", team.getId().toString())); } } @@ -201,7 +252,7 @@ public void isOwner(User user, ConditionCollector collector) { } public void noOwner(ConditionCollector collector) { - OMQueryBuilder existsQuery = queryBuilderFactory.existsQuery("owner.id"); + OMQueryBuilder existsQuery = queryBuilderFactory.existsQuery("owners.id"); collector.addMustNot(existsQuery); } diff --git a/openmetadata-service/src/test/java/org/openmetadata/service/search/security/ElasticSearchRBACConditionEvaluatorTest.java b/openmetadata-service/src/test/java/org/openmetadata/service/search/security/ElasticSearchRBACConditionEvaluatorTest.java index 30da785f07d6..549d72401e92 100644 --- a/openmetadata-service/src/test/java/org/openmetadata/service/search/security/ElasticSearchRBACConditionEvaluatorTest.java +++ b/openmetadata-service/src/test/java/org/openmetadata/service/search/security/ElasticSearchRBACConditionEvaluatorTest.java @@ -2,7 +2,6 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; -import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.*; @@ -127,7 +126,7 @@ void testIsOwner() { QueryBuilder elasticQuery = ((ElasticQueryBuilder) finalQuery).build(); String generatedQuery = elasticQuery.toString(); - assertTrue(generatedQuery.contains("owner.id"), "The query should contain 'owner.id'."); + assertTrue(generatedQuery.contains("owners.id"), "The query should contain 'owners.id'."); assertTrue( generatedQuery.contains(mockUser.getId().toString()), "The query should contain the user's ID."); @@ -144,8 +143,8 @@ void testNegationWithIsOwner() { assertTrue( generatedQuery.contains("must_not"), "The query should contain 'must_not' for negation."); assertTrue( - generatedQuery.contains("owner.id"), - "The query should contain 'owner.id' in the negation."); + generatedQuery.contains("owners.id"), + "The query should contain 'owners.id' in the negation."); assertTrue( generatedQuery.contains(mockUser.getId().toString()), "The negation should contain the user's ID."); @@ -182,7 +181,7 @@ void testComplexCondition() { generatedQuery.contains("PII.Sensitive"), "The query should contain 'PII.Sensitive' tag."); assertTrue(generatedQuery.contains("Test.Test1"), "The query should contain 'Test.Test1' tag."); assertTrue(generatedQuery.contains("Test.Test2"), "The query should contain 'Test.Test2' tag."); - assertTrue(generatedQuery.contains("owner.id"), "The query should contain 'owner.id'."); + assertTrue(generatedQuery.contains("owners.id"), "The query should contain 'owners.id'."); assertTrue( generatedQuery.contains("must_not"), "The query should contain a negation (must_not)."); @@ -221,13 +220,13 @@ void testComplexQueryStructure() throws IOException { // Check for the presence of owner.id in the "must_not" clause for the negation assertFieldExists( jsonContext, - "$.bool.should[2].bool.must_not[0].bool.should[?(@.term['owner.id'])]", - "owner.id in must_not"); + "$.bool.should[2].bool.must_not[0].bool.should[?(@.term['owners.id'])]", + "owners.id in must_not"); // Check for the presence of must_not for the case where there is no owner assertFieldExists( jsonContext, - "$.bool.should[3].bool.must_not[?(@.exists.field=='owner.id')]", + "$.bool.should[3].bool.must_not[?(@.exists.field=='owners.id')]", "no owner must_not clause"); // Count the number of bool clauses @@ -388,7 +387,7 @@ void testConditionUserLacksDomain() { // Check for owner ID and Public tag in the query assertFieldExists( jsonContext, - "$.bool.should[?(@.term['owner.id'].value=='" + mockUser.getId().toString() + "')]", + "$.bool.should[?(@.term['owners.id'].value=='" + mockUser.getId().toString() + "')]", "owner.id"); assertFieldExists( jsonContext, "$.bool.should[?(@.term['tags.tagFQN'].value=='Public')]", "Public tag"); @@ -415,7 +414,7 @@ void testNestedLogicalOperators() { assertTrue(generatedQuery.contains("tags.tagFQN"), "The query should contain 'tags.tagFQN'."); assertTrue(generatedQuery.contains("Sensitive"), "The query should contain 'Sensitive' tag."); - assertTrue(generatedQuery.contains("owner.id"), "The query should contain 'owner.id'."); + assertTrue(generatedQuery.contains("owners.id"), "The query should contain 'owners.id'."); assertFalse(generatedQuery.contains("match_none"), "The query should not be match_none."); } @@ -476,7 +475,7 @@ void testComplexNestedConditions() { // Ensure that the query does not check for noOwner since inAnyTeam('HR') is false assertFieldDoesNotExist( - jsonContext, "$.bool.must_not[?(@.exists.field=='owner.id')]", "noOwner clause"); + jsonContext, "$.bool.must_not[?(@.exists.field=='owners.id')]", "noOwner clause"); // Ensure the query does not contain a match_none condition assertFieldDoesNotExist(jsonContext, "$.bool[?(@.match_none)]", "match_none"); @@ -505,8 +504,8 @@ void testMultipleNestedNotOperators() { "Internal"); assertFieldExists( jsonContext, - "$.bool.must_not[0].bool.should[1].bool.should[?(@.term['owner.id'])]", - "owner.id"); + "$.bool.must_not[0].bool.should[1].bool.should[?(@.term['owners.id'])]", + "owners.id"); } @Test @@ -573,7 +572,7 @@ void testNestedAndOrWithMultipleMethods() { assertTrue( generatedQuery.contains("\"domain.id\""), "The query should contain 'domain.id' term."); assertTrue( - generatedQuery.contains("\"owner.id\""), "The query should contain 'owner.id' term."); + generatedQuery.contains("\"owners.id\""), "The query should contain 'owners.id' term."); assertFalse( generatedQuery.contains("\"must_not\" : [ { \"exists\""), "The query should not contain 'must_not' clause for 'noOwner()'."); @@ -634,7 +633,7 @@ void testComplexConditionWithAllMethods() { // Check for the presence of owner.id in the second should block assertFieldExists( - jsonContext, "$.bool.should[1].bool.should[?(@.term['owner.id'])]", "owner.id"); + jsonContext, "$.bool.should[1].bool.should[?(@.term['owners.id'])]", "owners.id"); } @Test @@ -740,7 +739,7 @@ void testComplexConditionWithMultipleNegations() { "Confidential"); // Ownership (isOwner condition) should be in `should` - assertFieldExists(jsonContext, "$.bool.should[?(@.term['owner.id'])]", "owner.id"); + assertFieldExists(jsonContext, "$.bool.should[?(@.term['owners.id'])]", "owners.id"); } @Test @@ -761,8 +760,8 @@ void testNotHasDomainWhenUserHasNoDomain() { "must_not for hasDomain"); assertFieldExists( jsonContext, - "$.bool.should[?(@.term['owner.id'].value=='" + mockUser.getId().toString() + "')]", - "owner.id"); + "$.bool.should[?(@.term['owners.id'].value=='" + mockUser.getId().toString() + "')]", + "owners.id"); assertFieldDoesNotExist(jsonContext, "$.bool[?(@.match_none)]", "match_none should not exist"); } @@ -861,33 +860,33 @@ void testMultipleRulesInPolicy() { // Check for the "Table" index condition assertFieldExists( jsonContext, - "$.bool.must[0].bool.must[?(@.terms._index && @.terms._index[?(@ == 'table')])]", + "$.bool.should[0].bool.must[?(@.terms._index && @.terms._index[?(@ == 'table')])]", "Index filtering for 'Table' resource"); assertFieldExists( jsonContext, - "$.bool.must[0].bool.must[?(@.match_all)]", + "$.bool.should[0].bool.must[?(@.match_all)]", "match_all for hasAnyRole 'Admin'"); assertFieldExists( jsonContext, - "$.bool.must[0].bool.should[?(@.term['tags.tagFQN'].value=='Sensitive')]", + "$.bool.should[0].bool.should[?(@.term['tags.tagFQN'].value=='Sensitive')]", "Sensitive tag"); // Check for the "Dashboard" index condition assertFieldExists( jsonContext, - "$.bool.must[1].bool.must[?(@.terms._index && @.terms._index[?(@ == 'database')])]", + "$.bool.should[1].bool.must[?(@.terms._index && @.terms._index[?(@ == 'database')])]", "Index filtering for 'Database' resource"); assertFieldExists( jsonContext, - "$.bool.must[1].bool.must[?(@.match_all)]", + "$.bool.should[1].bool.must[?(@.match_all)]", "match_all for inAnyTeam 'Engineering'"); assertFieldExists( jsonContext, - "$.bool.must[1].bool.must[?(@.term['tags.tagFQN'].value=='Confidential')]", + "$.bool.should[1].bool.must[?(@.term['tags.tagFQN'].value=='Confidential')]", "Confidential tag"); assertFieldExists( jsonContext, - "$.bool.must[1].bool.must[?(@.term['tags.tagFQN'].value=='Internal')]", + "$.bool.should[1].bool.must[?(@.term['tags.tagFQN'].value=='Internal')]", "Internal tag"); } @@ -929,7 +928,7 @@ void testMultiplePoliciesInRole() { // Check for domain filtering in the "Table" index clause assertFieldExists( jsonContext, - "$.bool.must[0].bool.must[?(@.term['domain.id'].value=='" + "$.bool.should[0].bool.must[?(@.term['domain.id'].value=='" + domain.getId().toString() + "')]", "user's domain ID"); @@ -937,7 +936,7 @@ void testMultiplePoliciesInRole() { // Check for the matchAnyTag clause for Public tag in the "Table" index clause assertFieldExists( jsonContext, - "$.bool.must[0].bool.should[?(@.term['tags.tagFQN'].value=='Public')]", + "$.bool.should[0].bool.should[?(@.term['tags.tagFQN'].value=='Public')]", "Public tag"); } @@ -979,7 +978,7 @@ void testRoleAndPolicyInheritanceFromTeams() { // Adjust the assertion for the hasDomain clause assertFieldExists( jsonContext, - "$.bool.must[0].bool.must[?(@.term['domain.id'].value=='" + "$.bool.should[0].bool.must[?(@.term['domain.id'].value=='" + domain.getId().toString() + "')]", "user's domain ID"); @@ -987,13 +986,13 @@ void testRoleAndPolicyInheritanceFromTeams() { // Check for the inAnyTeam('Engineering') clause assertFieldExists( jsonContext, - "$.bool.must[1].bool.must[?(@.match_all)]", + "$.bool.should[1].bool.must[?(@.match_all)]", "match_all for inAnyTeam 'Engineering'"); // Check for the matchAnyTag clause for Critical tag assertFieldExists( jsonContext, - "$.bool.must[1].bool.should[?(@.term['tags.tagFQN'].value=='Critical')]", + "$.bool.should[1].bool.should[?(@.term['tags.tagFQN'].value=='Critical')]", "Critical tag"); } @@ -1007,11 +1006,16 @@ void testRuleWithNonViewOperationIgnored() { List.of(List.of(MetadataOperation.EDIT_DESCRIPTION))); // Mock user ownership - when(mockUser.getId()).thenReturn(UUID.randomUUID()); + UUID userId = UUID.randomUUID(); + when(mockUser.getId()).thenReturn(userId); // Evaluate the condition OMQueryBuilder finalQuery = evaluator.evaluateConditions(mockSubjectContext); - assertNull(finalQuery, "The query should be null since the rule is ignored"); + QueryBuilder elasticQuery = ((ElasticQueryBuilder) finalQuery).build(); + String generatedQuery = elasticQuery.toString(); + assertTrue(generatedQuery.contains("owners.id"), "The query should contain 'owner.id'"); + assertTrue( + generatedQuery.contains(userId.toString()), "The query should contain the user's ID"); } @Test @@ -1033,8 +1037,91 @@ void testRuleWithViewBasicOperationApplied() { String generatedQuery = elasticQuery.toString(); // The rule should affect the search query - assertTrue(generatedQuery.contains("owner.id"), "The query should contain 'owner.id'"); + assertTrue(generatedQuery.contains("owners.id"), "The query should contain 'owner.id'"); assertTrue( generatedQuery.contains(userId.toString()), "The query should contain the user's ID"); } + + @Test + void testDenyAllOperationsOnTableResource() { + setupMockPolicies( + List.of(""), "DENY", List.of(List.of("Table")), List.of(List.of(MetadataOperation.ALL))); + + UUID userId = UUID.randomUUID(); + when(mockUser.getId()).thenReturn(userId); + + OMQueryBuilder finalQuery = evaluator.evaluateConditions(mockSubjectContext); + QueryBuilder elasticQuery = ((ElasticQueryBuilder) finalQuery).build(); + String generatedQuery = elasticQuery.toString(); + DocumentContext jsonContext = JsonPath.parse(generatedQuery); + + assertFieldExists( + jsonContext, + "$.bool.must_not[0].bool.must[?(@.terms._index && @.terms._index[?(@ == 'table')])]", + "must_not clause excluding 'table'"); + + // Assertions to ensure 'table' is not included in 'must' or 'should' clauses + assertFieldDoesNotExist( + jsonContext, + "$.bool.must[?(@.terms._index && @.terms._index[?(@ == 'table')])]", + "'table' should not be included in must clauses"); + assertFieldDoesNotExist( + jsonContext, + "$.bool.should[?(@.terms._index && @.terms._index[?(@ == 'table')])]", + "'table' should not be included in should clauses"); + } + + @Test + void testUserSeesOwnedAndUnownedResourcesIncludingTeamOwnership() { + // Set up a policy with three rules + setupMockPolicies( + List.of( + "noOwner()", // Rule 1 condition (ViewBasic) + "isOwner()" // Rule 2 condition (All operations) + ), + "ALLOW", + List.of( + List.of("All"), // Rule 1 resources + List.of("All") // Rule 2 resources + ), + List.of(List.of(MetadataOperation.EDIT_OWNERS), List.of(MetadataOperation.ALL))); + + UUID userId = UUID.randomUUID(); + when(mockUser.getId()).thenReturn(userId); + + UUID teamId1 = UUID.randomUUID(); + UUID teamId2 = UUID.randomUUID(); + EntityReference team1 = new EntityReference(); + team1.setId(teamId1); + team1.setName("TeamA"); + + EntityReference team2 = new EntityReference(); + team2.setId(teamId2); + team2.setName("TeamB"); + + when(mockUser.getTeams()).thenReturn(List.of(team1, team2)); + + OMQueryBuilder finalQuery = evaluator.evaluateConditions(mockSubjectContext); + QueryBuilder elasticQuery = ((ElasticQueryBuilder) finalQuery).build(); + String generatedQuery = elasticQuery.toString(); + DocumentContext jsonContext = JsonPath.parse(generatedQuery); + + assertFieldExists( + jsonContext, + "$.bool.should[1].bool.should[?(@.term['owners.id'].value=='" + userId.toString() + "')]", + "The query should allow resources where the user is the owner"); + + assertFieldExists( + jsonContext, + "$.bool.should[1].bool.should[?(@.term['owners.id'].value=='" + teamId1.toString() + "')]", + "The query should allow resources where TeamA is the owner"); + assertFieldExists( + jsonContext, + "$.bool.should[1].bool.should[?(@.term['owners.id'].value=='" + teamId2.toString() + "')]", + "The query should allow resources where TeamB is the owner"); + + assertFalse( + generatedQuery.contains("noOwner()"), + "The query should not contain 'noOwner()' as a string"); + } } From 07353588504e899dac90d7479e04614bd5e0e161 Mon Sep 17 00:00:00 2001 From: Sriharsha Chintalapani Date: Mon, 30 Sep 2024 22:40:31 -0700 Subject: [PATCH 2/2] Minor: Search RBAC, fix condition evaluation for single policy multiple rules --- .../security/RBACConditionEvaluator.java | 44 +++++++++++-------- 1 file changed, 26 insertions(+), 18 deletions(-) diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/search/security/RBACConditionEvaluator.java b/openmetadata-service/src/main/java/org/openmetadata/service/search/security/RBACConditionEvaluator.java index e0be841525a0..d61b9dfc206c 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/search/security/RBACConditionEvaluator.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/search/security/RBACConditionEvaluator.java @@ -50,8 +50,20 @@ public OMQueryBuilder evaluateConditions(SubjectContext subjectContext) { for (CompiledRule rule : context.getRules()) { boolean isDenyRule = rule.getEffect().toString().equalsIgnoreCase("DENY"); - - if (isDenyRule && SEARCH_RELEVANT_OPS.stream().noneMatch(rule.getOperations()::contains)) { + List mappedOperations = + rule.getOperations().stream() + .map( + op -> { + if (op.toString().equalsIgnoreCase("Create") + || op.toString().equalsIgnoreCase("Delete") + || op.toString().toLowerCase().startsWith("edit")) { + return MetadataOperation.VIEW_BASIC; + } + return op; + }) + .collect(Collectors.toList()); + + if (isDenyRule && SEARCH_RELEVANT_OPS.stream().noneMatch(mappedOperations::contains)) { continue; } @@ -110,7 +122,6 @@ private OMQueryBuilder buildRuleQuery(CompiledRule rule, User user) { (SpelExpression) spelParser.parseExpression(rule.getCondition()); preprocessExpression(parsedExpression.getAST(), ruleCollector); } else { - // If the condition is empty, treat it as always true ruleCollector.addMust(queryBuilderFactory.matchAllQuery()); } @@ -118,7 +129,6 @@ private OMQueryBuilder buildRuleQuery(CompiledRule rule, User user) { } private void preprocessExpression(SpelNode node, ConditionCollector collector) { - // Delay this check until after processing necessary expressions if (collector.isMatchNothing()) { return; } @@ -133,35 +143,33 @@ private void preprocessExpression(SpelNode node, ConditionCollector collector) { } else if (node instanceof OpOr) { List orQueries = new ArrayList<>(); boolean allMatchNothing = true; - boolean hasTrueCondition = false; // Track if any condition evaluated to true + boolean hasTrueCondition = false; for (int i = 0; i < node.getChildCount(); i++) { ConditionCollector childCollector = new ConditionCollector(queryBuilderFactory); preprocessExpression(node.getChild(i), childCollector); if (childCollector.isMatchNothing()) { - continue; // If this child evaluates to match nothing, skip it + continue; } if (childCollector.isMatchAllQuery()) { - hasTrueCondition = true; // If any condition evaluates to true, mark it - break; // Short-circuit: if any condition in OR evaluates to true, the whole OR is true + hasTrueCondition = true; + break; } OMQueryBuilder childQuery = childCollector.buildFinalQuery(); if (childQuery != null) { - allMatchNothing = - false; // If at least one child query is valid, it’s not all match nothing + allMatchNothing = false; orQueries.add(childQuery); } } if (hasTrueCondition) { - collector.addMust(queryBuilderFactory.matchAllQuery()); // OR is true, add match_all + collector.addMust(queryBuilderFactory.matchAllQuery()); } else if (allMatchNothing) { - collector.setMatchNothing(true); // OR is false + collector.setMatchNothing(true); } else { - // Add the valid OR queries to the collector for (OMQueryBuilder orQuery : orQueries) { collector.addShould(orQuery); } @@ -177,7 +185,7 @@ private void preprocessExpression(SpelNode node, ConditionCollector collector) { } else { OMQueryBuilder subQuery = subCollector.buildFinalQuery(); if (subQuery != null && !subQuery.isEmpty()) { - collector.addMustNot(subQuery); // Add must_not without extra nesting + collector.addMustNot(subQuery); } } } else if (node instanceof MethodReference) { @@ -216,7 +224,7 @@ private List extractMethodArguments(MethodReference methodRef) { List args = new ArrayList<>(); for (int i = 0; i < methodRef.getChildCount(); i++) { SpelNode childNode = methodRef.getChild(i); - String value = childNode.toStringAST().replace("'", ""); // Remove single quotes + String value = childNode.toStringAST().replace("'", ""); args.add(value); } return args; @@ -232,7 +240,7 @@ public void matchAnyTag(List tags, ConditionCollector collector) { public void matchAllTags(List tags, ConditionCollector collector) { for (String tag : tags) { OMQueryBuilder tagQuery = queryBuilderFactory.termQuery("tags.tagFQN", tag); - collector.addMust(tagQuery); // Add directly to the collector's must clause + collector.addMust(tagQuery); } } @@ -247,7 +255,7 @@ public void isOwner(User user, ConditionCollector collector) { } for (OMQueryBuilder ownerQuery : ownerQueries) { - collector.addShould(ownerQuery); // Add directly to the collector's should clause + collector.addShould(ownerQuery); } } @@ -304,7 +312,7 @@ private OMQueryBuilder getIndexFilter(List resources) { List indices = resources.stream() .map(resource -> Entity.getSearchRepository().getIndexOrAliasName(resource)) - .collect(Collectors.toList()); + .toList(); return queryBuilderFactory.termsQuery("_index", indices); }