Skip to content

Commit

Permalink
Minor: Fix context evaluation to use the class as context (#14490)
Browse files Browse the repository at this point in the history
  • Loading branch information
@harshach
harshach committed Dec 22, 2023
1 parent 3f0ee44 commit 3dde9f4
Show file tree
Hide file tree
Showing 7 changed files with 38 additions and 69 deletions.
4 changes: 1 addition & 3 deletions ...etadata-service/src/main/java/org/openmetadata/service/events/subscription/AlertUtil.java
View file
Expand Up @@ -49,7 +49,6 @@
import org.openmetadata.service.resources.CollectionRegistry;
import org.openmetadata.service.search.models.IndexMapping;
import org.springframework.expression.Expression;
import org.springframework.expression.spel.support.StandardEvaluationContext;

@Slf4j
public final class AlertUtil {
Expand Down Expand Up @@ -169,9 +168,8 @@ public static boolean evaluateAlertConditions(ChangeEvent changeEvent, List<Even
boolean result;
String completeCondition = buildCompleteCondition(alertFilterRules);
AlertsRuleEvaluator ruleEvaluator = new AlertsRuleEvaluator(changeEvent);
StandardEvaluationContext evaluationContext = new StandardEvaluationContext(ruleEvaluator);
Expression expression = parseExpression(completeCondition);
result = Boolean.TRUE.equals(expression.getValue(evaluationContext, Boolean.class));
result = Boolean.TRUE.equals(expression.getValue(ruleEvaluator, Boolean.class));
LOG.debug("Alert evaluated as Result : {}", result);
return result;
} else {
Expand Down
25 changes: 3 additions & 22 deletions ...ava/org/openmetadata/service/resources/events/subscription/EventSubscriptionResource.java
View file
Expand Up @@ -58,7 +58,6 @@
import org.openmetadata.schema.entity.events.SubscriptionStatus;
import org.openmetadata.schema.type.EntityHistory;
import org.openmetadata.schema.type.Function;
import org.openmetadata.schema.type.Include;
import org.openmetadata.schema.type.MetadataOperation;
import org.openmetadata.schema.type.SubscriptionResourceDescriptor;
import org.openmetadata.service.Entity;
Expand Down Expand Up @@ -304,27 +303,6 @@ public Response createOrUpdateEventSubscription(
return response;
}

@PUT
@Path("/trigger/{id}")
@Operation(
operationId = "triggerDataInsightJob",
summary = "Trigger a existing Data Insight Report Job to run",
description = "Trigger a existing Data Insight Report Job to run",
responses = {
@ApiResponse(responseCode = "200", description = "Trigger a Data Insight Job"),
@ApiResponse(responseCode = "400", description = "Bad request")
})
public Response triggerDataInsightJob(
@Context UriInfo uriInfo,
@Context SecurityContext securityContext,
@Parameter(description = "Id of the event Subscription", schema = @Schema(type = "UUID")) @PathParam("id")
UUID id)
throws SchedulerException {
authorizer.authorizeAdmin(securityContext);
EventSubscription eventSub = repository.find(id, Include.NON_DELETED);
return ReportsHandler.getInstance().triggerExistingDataInsightJob(eventSub);
}

@PATCH
@Path("/{id}")
@Operation(
Expand Down Expand Up @@ -507,6 +485,7 @@ public SubscriptionStatus getEventSubscriptionStatusById(
description = "Get list of Event Subscription functions used in filtering conditions in Event Subscriptions")
public List<Function> listEventSubscriptionFunctions(
@Context UriInfo uriInfo, @Context SecurityContext securityContext) {
authorizer.authorizeAdmin(securityContext);
return new ArrayList<>(AlertUtil.getAlertFilterFunctions().values());
}

Expand All @@ -518,6 +497,7 @@ public List<Function> listEventSubscriptionFunctions(
description = "Get list of EventSubscription functions used in filtering conditions in Event Subscription")
public ResultList<SubscriptionResourceDescriptor> listEventSubResources(
@Context UriInfo uriInfo, @Context SecurityContext securityContext) {
authorizer.authorizeAdmin(securityContext);
return new ResultList<>(EventsSubscriptionRegistry.listResourceDescriptors());
}

Expand All @@ -536,6 +516,7 @@ public void validateCondition(
@Context SecurityContext securityContext,
@Parameter(description = "Expression to validate", schema = @Schema(type = "string")) @PathParam("expression")
String expression) {
authorizer.authorizeAdmin(securityContext);
AlertUtil.validateExpression(expression, Boolean.class);
}

Expand Down
6 changes: 4 additions & 2 deletions openmetadata-service/src/main/java/org/openmetadata/service/security/JwtFilter.java
View file
Expand Up @@ -65,7 +65,9 @@ public class JwtFilter implements ContainerRequestFilter {
private AuthProvider providerType;
public static final List<String> EXCLUDED_ENDPOINTS =
List.of(
"v1/system/config",
"v1/system/config/jwks",
"v1/system/config/authorizer",
"v1/system/config/customLogoConfiguration",
"v1/users/signup",
"v1/system/version",
"v1/users/registrationConfirmation",
Expand Down Expand Up @@ -110,7 +112,7 @@ public JwtFilter(
@Override
public void filter(ContainerRequestContext requestContext) {
UriInfo uriInfo = requestContext.getUriInfo();
if (EXCLUDED_ENDPOINTS.stream().anyMatch(endpoint -> uriInfo.getPath().contains(endpoint))) {
if (EXCLUDED_ENDPOINTS.stream().anyMatch(endpoint -> uriInfo.getPath().equalsIgnoreCase(endpoint))) {
return;
}

Expand Down
7 changes: 2 additions & 5 deletions ...service/src/main/java/org/openmetadata/service/security/policyevaluator/CompiledRule.java
View file
Expand Up @@ -18,7 +18,6 @@
import org.openmetadata.service.security.policyevaluator.SubjectContext.PolicyContext;
import org.springframework.expression.Expression;
import org.springframework.expression.spel.standard.SpelExpressionParser;
import org.springframework.expression.spel.support.StandardEvaluationContext;

/** This class is used in a single threaded model and hence does not have concurrency support */
@Slf4j
Expand Down Expand Up @@ -54,9 +53,8 @@ public static <T> void validateExpression(String condition, Class<T> clz) {
}
Expression expression = parseExpression(condition);
RuleEvaluator ruleEvaluator = new RuleEvaluator();
StandardEvaluationContext evaluationContext = new StandardEvaluationContext(ruleEvaluator);
try {
expression.getValue(evaluationContext, clz);
expression.getValue(ruleEvaluator, clz);
} catch (Exception exception) {
// Remove unnecessary class details in the exception message
String message = exception.getMessage().replaceAll("on type .*$", "").replaceAll("on object .*$", "");
Expand Down Expand Up @@ -207,8 +205,7 @@ private boolean matchExpression(
return true;
}
RuleEvaluator ruleEvaluator = new RuleEvaluator(policyContext, subjectContext, resourceContext);
StandardEvaluationContext evaluationContext = new StandardEvaluationContext(ruleEvaluator);
return Boolean.TRUE.equals(expr.getValue(evaluationContext, Boolean.class));
return Boolean.TRUE.equals(expr.getValue(ruleEvaluator, Boolean.class));
}

public static boolean overrideAccess(Access newAccess, Access currentAccess) {
Expand Down
46 changes: 18 additions & 28 deletions ...e/src/test/java/org/openmetadata/service/events/subscription/AlertsRuleEvaluatorTest.java
View file
Expand Up @@ -27,8 +27,6 @@
import org.openmetadata.service.OpenMetadataApplicationTest;
import org.openmetadata.service.resources.EntityResourceTest;
import org.openmetadata.service.resources.databases.TableResourceTest;
import org.springframework.expression.EvaluationContext;
import org.springframework.expression.spel.support.StandardEvaluationContext;

class AlertsRuleEvaluatorResourceTest extends OpenMetadataApplicationTest {
private static TableResourceTest tableResourceTest;
Expand All @@ -45,9 +43,8 @@ void test_matchAnySource() {
ChangeEvent changeEvent = new ChangeEvent();
changeEvent.setEntityType("alert");
AlertsRuleEvaluator alertsRuleEvaluator = new AlertsRuleEvaluator(changeEvent);
EvaluationContext evaluationContext = new StandardEvaluationContext(alertsRuleEvaluator);
assertTrue(evaluateExpression("matchAnySource('alert')", evaluationContext));
assertFalse(evaluateExpression("matchAnySource('bot')", evaluationContext));
assertTrue(evaluateExpression("matchAnySource('alert')", alertsRuleEvaluator));
assertFalse(evaluateExpression("matchAnySource('bot')", alertsRuleEvaluator));
}

@Test
Expand All @@ -65,10 +62,9 @@ void test_matchAnyOwnerName(TestInfo test) throws IOException {

// Test Owner Name Present in list and not present in list
AlertsRuleEvaluator alertsRuleEvaluator = new AlertsRuleEvaluator(changeEvent);
EvaluationContext evaluationContext = new StandardEvaluationContext(alertsRuleEvaluator);
assertTrue(
evaluateExpression("matchAnyOwnerName('" + EntityResourceTest.USER1.getName() + "')", evaluationContext));
assertFalse(evaluateExpression("matchAnyOwnerName('tempName')", evaluationContext));
evaluateExpression("matchAnyOwnerName('" + EntityResourceTest.USER1.getName() + "')", alertsRuleEvaluator));
assertFalse(evaluateExpression("matchAnyOwnerName('tempName')", alertsRuleEvaluator));
}

// issue: https://github.com/open-metadata/OpenMetadata/issues/10376
Expand All @@ -85,10 +81,9 @@ void test_matchAnyEntityFqn(TestInfo test) throws IOException {

// Test Entity Fqn in List of match and not present in list
AlertsRuleEvaluator alertsRuleEvaluator = new AlertsRuleEvaluator(changeEvent);
EvaluationContext evaluationContext = new StandardEvaluationContext(alertsRuleEvaluator);
String fqn = createdTable.getFullyQualifiedName();
assertTrue(evaluateExpression("matchAnyEntityFqn('" + fqn + "')", evaluationContext));
assertFalse(evaluateExpression("matchAnyEntityFqn('testFQN1')", evaluationContext));
assertTrue(evaluateExpression("matchAnyEntityFqn('" + fqn + "')", alertsRuleEvaluator));
assertFalse(evaluateExpression("matchAnyEntityFqn('testFQN1')", alertsRuleEvaluator));
}

@Test
Expand All @@ -105,10 +100,9 @@ void test_matchAnyEntityId(TestInfo test) throws IOException {

// Test Entity Id in List of match and not present in list
AlertsRuleEvaluator alertsRuleEvaluator = new AlertsRuleEvaluator(changeEvent);
EvaluationContext evaluationContext = new StandardEvaluationContext(alertsRuleEvaluator);
String id = createdTable.getId().toString();
assertTrue(evaluateExpression("matchAnyEntityId('" + id + "')", evaluationContext));
assertFalse(evaluateExpression("matchAnyEntityId('" + UUID.randomUUID() + "')", evaluationContext));
assertTrue(evaluateExpression("matchAnyEntityId('" + id + "')", alertsRuleEvaluator));
assertFalse(evaluateExpression("matchAnyEntityId('" + UUID.randomUUID() + "')", alertsRuleEvaluator));
}

@Test
Expand All @@ -119,9 +113,8 @@ void test_matchAnyEventType() {

// Check if eventType present in list or absent from the list
AlertsRuleEvaluator alertsRuleEvaluator = new AlertsRuleEvaluator(changeEvent);
EvaluationContext evaluationContext = new StandardEvaluationContext(alertsRuleEvaluator);
assertTrue(evaluateExpression("matchAnyEventType('entityCreated')", evaluationContext));
assertFalse(evaluateExpression("matchAnyEventType('entityUpdated')", evaluationContext));
assertTrue(evaluateExpression("matchAnyEventType('entityCreated')", alertsRuleEvaluator));
assertFalse(evaluateExpression("matchAnyEventType('entityUpdated')", alertsRuleEvaluator));
}

@Test
Expand All @@ -142,9 +135,8 @@ void test_matchTestResult() {

// Test If Test Result status matches in list and if status not matches
AlertsRuleEvaluator alertsRuleEvaluator = new AlertsRuleEvaluator(changeEvent);
EvaluationContext evaluationContext = new StandardEvaluationContext(alertsRuleEvaluator);
assertTrue(evaluateExpression("matchTestResult('Success')", evaluationContext));
assertFalse(evaluateExpression("matchTestResult('Failed')", evaluationContext));
assertTrue(evaluateExpression("matchTestResult('Success')", alertsRuleEvaluator));
assertFalse(evaluateExpression("matchTestResult('Failed')", alertsRuleEvaluator));
}

@Test
Expand All @@ -155,9 +147,8 @@ void test_matchUpdatedBy() {

// Test if the username is in list or not
AlertsRuleEvaluator alertsRuleEvaluator = new AlertsRuleEvaluator(changeEvent);
EvaluationContext evaluationContext = new StandardEvaluationContext(alertsRuleEvaluator);
assertTrue(evaluateExpression("matchUpdatedBy('test')", evaluationContext));
assertFalse(evaluateExpression("matchUpdatedBy('test1')", evaluationContext));
assertTrue(evaluateExpression("matchUpdatedBy('test')", alertsRuleEvaluator));
assertFalse(evaluateExpression("matchUpdatedBy('test1')", alertsRuleEvaluator));
}

@Test
Expand All @@ -172,12 +163,11 @@ void test_matchAnyFieldChange() {

// Test if the updated field matches from the list or not
AlertsRuleEvaluator alertsRuleEvaluator = new AlertsRuleEvaluator(changeEvent);
EvaluationContext evaluationContext = new StandardEvaluationContext(alertsRuleEvaluator);
assertTrue(evaluateExpression("matchAnyFieldChange('test')", evaluationContext));
assertFalse(evaluateExpression("matchAnyFieldChange('temp')", evaluationContext));
assertTrue(evaluateExpression("matchAnyFieldChange('test')", alertsRuleEvaluator));
assertFalse(evaluateExpression("matchAnyFieldChange('temp')", alertsRuleEvaluator));
}

private Boolean evaluateExpression(String condition, EvaluationContext evaluationContext) {
return parseExpression(condition).getValue(evaluationContext, Boolean.class);
private Boolean evaluateExpression(String condition, AlertsRuleEvaluator alertsRuleEvaluator) {
return parseExpression(condition).getValue(alertsRuleEvaluator, Boolean.class);
}
}
6 changes: 3 additions & 3 deletions ...rvice/src/test/java/org/openmetadata/service/resources/glossary/GlossaryResourceTest.java
View file
Expand Up @@ -380,7 +380,7 @@ void testGlossaryImportExport() throws IOException {
List<String> createRecords =
listOf(
String.format(
",g1,dsp1,\"dsc1,1\",h1;h2;h3,,term1;http://term1,Tier.Tier1,%s;%s,user;%s,%s",
",g1,dsp1,\"dsc1,1\",h1;h2;h3,,term1;https://term1,Tier.Tier1,%s;%s,user;%s,%s",
user1, user2, user1, "Approved"),
String.format(
",g2,dsp2,dsc3,h1;h3;h3,,term2;https://term2,Tier.Tier2,%s,user;%s,%s", user1, user2, "Approved"),
Expand All @@ -390,14 +390,14 @@ void testGlossaryImportExport() throws IOException {
List<String> updateRecords =
listOf(
String.format(
",g1,dsp1,new-dsc1,h1;h2;h3,,term1;http://term1,Tier.Tier1,%s;%s,user;%s,%s",
",g1,dsp1,new-dsc1,h1;h2;h3,,term1;https://term1,Tier.Tier1,%s;%s,user;%s,%s",
user1, user2, user1, "Approved"),
String.format(
",g2,dsp2,new-dsc3,h1;h3;h3,,term2;https://term2,Tier.Tier2,%s,user;%s,%s", user1, user2, "Approved"),
String.format("importExportTest.g1,g11,dsp2,new-dsc11,h1;h3;h3,,,,%s,team;%s,%s", user1, team11, "Draft"));

// Add new row to existing rows
List<String> newRecords = listOf(",g3,dsp0,dsc0,h1;h2;h3,,term0;http://term0,Tier.Tier3,,,Approved");
List<String> newRecords = listOf(",g3,dsp0,dsc0,h1;h2;h3,,term0;https://term0,Tier.Tier3,,,Approved");
testImportExport(glossary.getName(), GlossaryCsv.HEADERS, createRecords, updateRecords, newRecords);
}

Expand Down
13 changes: 7 additions & 6 deletions ...ce/src/test/java/org/openmetadata/service/security/policyevaluator/RuleEvaluatorTest.java
View file
Expand Up @@ -34,7 +34,7 @@
import org.openmetadata.service.jdbi3.TeamRepository;
import org.openmetadata.service.security.policyevaluator.SubjectContext.PolicyContext;
import org.springframework.expression.EvaluationContext;
import org.springframework.expression.spel.support.StandardEvaluationContext;
import org.springframework.expression.spel.support.SimpleEvaluationContext;

class RuleEvaluatorTest {
private static final Table table = new Table().withName("table");
Expand All @@ -43,6 +43,8 @@ class RuleEvaluatorTest {
private static SubjectContext subjectContext;
private static ResourceContext resourceContext;

private static RuleEvaluator ruleEvaluator;

@BeforeAll
public static void setup() {
TeamRepository teamRepository = mock(TeamRepository.class);
Expand Down Expand Up @@ -73,8 +75,8 @@ public static void setup() {
user = new User().withId(UUID.randomUUID()).withName("user");
resourceContext = new ResourceContext("table", table, mock(TableRepository.class));
subjectContext = new SubjectContext(user);
RuleEvaluator ruleEvaluator = new RuleEvaluator(null, subjectContext, resourceContext);
evaluationContext = new StandardEvaluationContext(ruleEvaluator);
ruleEvaluator = new RuleEvaluator(null, subjectContext, resourceContext);
evaluationContext = SimpleEvaluationContext.forReadOnlyDataBinding().withRootObject(ruleEvaluator).build();
}

@Test
Expand Down Expand Up @@ -249,7 +251,7 @@ void test_hasAnyRole() {
}

private Boolean evaluateExpression(String condition) {
return parseExpression(condition).getValue(evaluationContext, Boolean.class);
return parseExpression(condition).getValue(ruleEvaluator, Boolean.class);
}

private List<TagLabel> getTags(String... tags) {
Expand Down Expand Up @@ -294,7 +296,6 @@ private Role createRole(String roleName) {

private void updatePolicyContext(String team) {
PolicyContext policyContext = new PolicyContext(Entity.TEAM, team, null, null, null);
RuleEvaluator ruleEvaluator = new RuleEvaluator(policyContext, subjectContext, resourceContext);
evaluationContext = new StandardEvaluationContext(ruleEvaluator);
ruleEvaluator = new RuleEvaluator(policyContext, subjectContext, resourceContext);
}
}

0 comments on commit 3dde9f4

Please sign in to comment.