Skip to content

Commit

Permalink
GEN 1931 - Fix entity link accepted chars (#18391)
Browse files Browse the repository at this point in the history 
* fix: entity link accepted characters

* fix: match all characters but `<>|::`

* fix: remove unnecessary negative lookbehind

* fix the test case search not working on add ingestion page in case of special characters in FQN

* style: ran java linting

* fix: failing testst

* style: ran java linting

---------

Co-authored-by: Aniket Katkar <aniketkatkar97@gmail.com>
  • Loading branch information
@TeddyCr @aniketkatkar97
TeddyCr and aniketkatkar97 authored Oct 29, 2024
1 parent 67a9e63 commit 4f8bf05
Show file tree
Hide file tree
Showing 8 changed files with 304 additions and 71 deletions.
4 changes: 2 additions & 2 deletions ...metadata-service/src/test/java/org/openmetadata/service/resources/EntityResourceTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,7 @@ public abstract class EntityResourceTest<T extends EntityInterface, K extends Cr
protected final boolean supportsEmptyDescription;

// Special characters supported in the entity name
protected String supportedNameCharacters = "_'-.&()" + RANDOM_STRING_GENERATOR.generate(1);
protected String supportedNameCharacters = "_'-.&()[]" + RANDOM_STRING_GENERATOR.generate(1);

protected final boolean supportsCustomExtension;

Expand All @@ -252,7 +252,7 @@ public abstract class EntityResourceTest<T extends EntityInterface, K extends Cr
public static final String DATA_CONSUMER_ROLE_NAME = "DataConsumer";

public static final String ENTITY_LINK_MATCH_ERROR =
"[entityLink must match \"(?U)^<#E::\\w+::[\\w'\\- .&/:+\"\\\\()$#%]+>$\"]";
"[entityLink must match \"(?U)^<#E::\\w+::(?:[^:<>|]|:[^:<>|])+(?:::(?:[^:<>|]|:[^:<>|])+)*>$\"]";

// Random unicode string generator to test entity name accepts all the unicode characters
protected static final RandomStringGenerator RANDOM_STRING_GENERATOR =
Expand Down
47 changes: 47 additions & 0 deletions ...ervice/src/test/java/org/openmetadata/service/resources/dqtests/TestCaseResourceTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import static org.junit.jupiter.api.Assertions.assertNotEquals;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertNull;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.openmetadata.common.utils.CommonUtil.listOf;
import static org.openmetadata.schema.type.ColumnDataType.BIGINT;
Expand Down Expand Up @@ -3188,6 +3189,52 @@ void test_listTestCaseFromSearch(TestInfo testInfo) throws HttpResponseException
});
}

@Test
void test_testCaseInvalidEntityLinkTest(TestInfo testInfo) throws IOException {
// Invalid entity link as not parsable by antlr parser
String entityLink = "<#E::table::special!@#$%^&*()_+[]{}|;:\\'\",./?>";
CreateTestCase create = createRequest(testInfo);
create
.withEntityLink(entityLink)
.withTestSuite(TEST_SUITE1.getFullyQualifiedName())
.withTestDefinition(TEST_DEFINITION3.getFullyQualifiedName())
.withParameterValues(
List.of(new TestCaseParameterValue().withValue("100").withName("missingCountValue")));

assertThrows(
HttpResponseException.class,
() -> createAndCheckEntity(create, ADMIN_AUTH_HEADERS),
"entityLink must match \"(?U)^<#E::\\w+::[\\w'\\- .&/:+\"\\\\()$#%]+>$\"");

entityLink = "<#E::table::user<name>::column>";
create.setEntityLink(entityLink);
assertThrows(
HttpResponseException.class,
() -> createAndCheckEntity(create, ADMIN_AUTH_HEADERS),
"entityLink must match \"(?U)^<#E::\\w+::[\\w'\\- .&/:+\"\\\\()$#%]+>$\"");

entityLink = "<#E::table::user>name::column>";
create.setEntityLink(entityLink);
assertThrows(
HttpResponseException.class,
() -> createAndCheckEntity(create, ADMIN_AUTH_HEADERS),
"entityLink must match \"(?U)^<#E::\\w+::[\\w'\\- .&/:+\"\\\\()$#%]+>$\"");

entityLink = "<#E::table::foo<>bar::baz>\");";
create.setEntityLink(entityLink);
assertThrows(
HttpResponseException.class,
() -> createAndCheckEntity(create, ADMIN_AUTH_HEADERS),
"entityLink must match \"(?U)^<#E::\\w+::[\\w'\\- .&/:+\"\\\\()$#%]+>$\"");

entityLink = "<#E::table::::baz>";
create.setEntityLink(entityLink);
assertThrows(
HttpResponseException.class,
() -> createAndCheckEntity(create, ADMIN_AUTH_HEADERS),
"entityLink must match \"(?U)^<#E::\\w+::[\\w'\\- .&/:+\"\\\\()$#%]+>$\"");
}

private void putInspectionQuery(TestCase testCase, String sql) throws IOException {
TestCase putResponse = putInspectionQuery(testCase.getId(), sql, ADMIN_AUTH_HEADERS);
assertEquals(sql, putResponse.getInspectionQuery());
Expand Down
146 changes: 82 additions & 64 deletions ...rc/test/java/org/openmetadata/service/resources/events/EventSubscriptionResourceTest.java
View file

Large diffs are not rendered by default.

3 changes: 2 additions & 1 deletion ...data-service/src/test/java/org/openmetadata/service/resources/feeds/FeedResourceTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,8 @@ void post_feedWithInvalidAbout_4xx() {
// Create thread without addressed to entity in the request
CreateThread create = create().withFrom(USER.getName()).withAbout("<>"); // Invalid EntityLink

String failureReason = "[about must match \"(?U)^<#E::\\w+::[\\w'\\- .&/:+\"\\\\()$#%]+>$\"]";
String failureReason =
"[about must match \"(?U)^<#E::\\w+::(?:[^:<>|]|:[^:<>|])+(?:::(?:[^:<>|]|:[^:<>|])+)*>$\"]";
assertResponseContains(
() -> createThread(create, USER_AUTH_HEADERS), BAD_REQUEST, failureReason);

Expand Down
2 changes: 1 addition & 1 deletion ...rvice/src/test/java/org/openmetadata/service/resources/feeds/SuggestionsResourceTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ void post_suggestionWithInvalidAbout_4xx() {
CreateSuggestion create = create().withEntityLink("<>"); // Invalid EntityLink

String failureReason =
"[entityLink must match \"(?U)^<#E::\\w+::[\\w'\\- .&/:+\"\\\\()$#%]+>$\"]";
"[entityLink must match \"(?U)^<#E::\\w+::(?:[^:<>|]|:[^:<>|])+(?:::(?:[^:<>|]|:[^:<>|])+)*>$\"]";
assertResponseContains(
() -> createSuggestion(create, USER_AUTH_HEADERS), BAD_REQUEST, failureReason);

Expand Down
166 changes: 166 additions & 0 deletions openmetadata-service/src/test/java/org/openmetadata/service/util/EntityUtilTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,12 @@

import static org.junit.jupiter.api.Assertions.*;

import java.util.HashMap;
import java.util.Map;
import org.junit.jupiter.api.Test;
import org.openmetadata.schema.entity.data.GlossaryTerm;
import org.openmetadata.schema.entity.data.Table;
import org.openmetadata.service.resources.feeds.MessageParser;

class EntityUtilTest {
@Test
Expand All @@ -15,4 +18,167 @@ void test_isDescriptionRequired() {
EntityUtil.isDescriptionRequired(
GlossaryTerm.class)); // GlossaryTerm entity requires description
}

@Test
void test_entityLinkParser() {

// Valid entity links
Map<String, String> expected = new HashMap<>();
expected.put("entityLink", "<#E::table::users>");
expected.put("arrayFieldName", null);
expected.put("arrayFieldValue", null);
expected.put("fieldName", null);
expected.put("entityFQN", "users");
expected.put("entityType", "table");
expected.put("linkType", "ENTITY");
expected.put("fullyQualifiedFieldType", "table");
expected.put("fullyQualifiedFieldValue", "users");
verifyEntityLinkParser(expected);

expected.clear();
expected.put("entityLink", "<#E::table::users.foo.\"bar.baz\">");
expected.put("arrayFieldName", null);
expected.put("arrayFieldValue", null);
expected.put("fieldName", null);
expected.put("entityFQN", "users.foo.\"bar.baz\"");
expected.put("entityType", "table");
expected.put("linkType", "ENTITY");
expected.put("fullyQualifiedFieldType", "table");
expected.put("fullyQualifiedFieldValue", "users.foo.\"bar.baz\"");
verifyEntityLinkParser(expected);

expected.clear();
expected.put("entityLink", "<#E::db::customers>");
expected.put("arrayFieldName", null);
expected.put("arrayFieldValue", null);
expected.put("fieldName", null);
expected.put("entityFQN", "customers");
expected.put("entityType", "db");
expected.put("linkType", "ENTITY");
expected.put("fullyQualifiedFieldType", "db");
expected.put("fullyQualifiedFieldValue", "customers");
verifyEntityLinkParser(expected);

expected.clear();
expected.put("entityLink", "<#E::table::users::column::id>");
expected.put("arrayFieldName", "id");
expected.put("arrayFieldValue", null);
expected.put("fieldName", "column");
expected.put("entityFQN", "users");
expected.put("entityType", "table");
expected.put("linkType", "ENTITY_ARRAY_FIELD");
expected.put("fullyQualifiedFieldType", "table.column.member");
expected.put("fullyQualifiedFieldValue", "users.id");
verifyEntityLinkParser(expected);

expected.clear();
expected.put("entityLink", "<#E::table::orders::column::status::type::enum>");
expected.put("arrayFieldName", "status");
expected.put("arrayFieldValue", "type::enum");
expected.put("fieldName", "column");
expected.put("entityFQN", "orders");
expected.put("entityType", "table");
expected.put("linkType", "ENTITY_ARRAY_FIELD");
expected.put("fullyQualifiedFieldType", "table.column.member");
expected.put("fullyQualifiedFieldValue", "orders.status.type::enum");
verifyEntityLinkParser(expected);

expected.clear();
expected.put("entityLink", "<#E::db::schema::table::view::column>");
expected.put("arrayFieldName", "view");
expected.put("arrayFieldValue", "column");
expected.put("fieldName", "table");
expected.put("entityFQN", "schema");
expected.put("entityType", "db");
expected.put("linkType", "ENTITY_ARRAY_FIELD");
expected.put("fullyQualifiedFieldType", "db.table.member");
expected.put("fullyQualifiedFieldValue", "schema.view.column");
verifyEntityLinkParser(expected);

expected.clear();
expected.put("entityLink", "<#E::table::foo@bar>");
expected.put("arrayFieldName", null);
expected.put("arrayFieldValue", null);
expected.put("fieldName", null);
expected.put("entityFQN", "foo@bar");
expected.put("entityType", "table");
expected.put("linkType", "ENTITY");
expected.put("fullyQualifiedFieldType", "table");
expected.put("fullyQualifiedFieldValue", "foo@bar");
verifyEntityLinkParser(expected);

expected.clear();
expected.put("entityLink", "<#E::table::foo[bar]>");
expected.put("arrayFieldName", null);
expected.put("arrayFieldValue", null);
expected.put("fieldName", null);
expected.put("entityFQN", "foo[bar]");
expected.put("entityType", "table");
expected.put("linkType", "ENTITY");
expected.put("fullyQualifiedFieldType", "table");
expected.put("fullyQualifiedFieldValue", "foo[bar]");
verifyEntityLinkParser(expected);

expected.clear();
expected.put("entityLink", "<#E::table::special!@#$%^&*()_+[]{};:\\'\",./?>");
expected.put("arrayFieldName", null);
expected.put("arrayFieldValue", null);
expected.put("fieldName", null);
expected.put("entityFQN", "special!@#$%^&*()_+[]{};:\\'\",./?");
expected.put("entityType", "table");
expected.put("linkType", "ENTITY");
expected.put("fullyQualifiedFieldType", "table");
expected.put("fullyQualifiedFieldValue", "special!@#$%^&*()_+[]{};:\\'\",./?");
verifyEntityLinkParser(expected);

// Invalid entity link
expected.clear();
expected.put("entityLink", "<#E::table::special!@#$%^&*()_+[]{}|;:\\'\",./?>");
// EntityLink with `|` character will not be parsed correctly and everything after `|` will be
// ignored
org.opentest4j.AssertionFailedError exception =
assertThrows(
org.opentest4j.AssertionFailedError.class, () -> verifyEntityLinkParser(expected));
assertEquals(
"expected: <<#E::table::special!@#$%^&*()_+[]{}|;:\\'\",./?>> but was: <<#E::table::special!@#$%^&*()_+[]{}>>",
exception.getMessage());

expected.clear();
expected.put("entityLink", "<#E::table::user<name>::column>");
IllegalArgumentException argException =
assertThrows(IllegalArgumentException.class, () -> verifyEntityLinkParser(expected));
assertEquals(
"Entity link was not found in <#E::table::user<name>::column>", argException.getMessage());

expected.clear();
expected.put("entityLink", "<#E::table::user>name::column>");
exception =
assertThrows(
org.opentest4j.AssertionFailedError.class, () -> verifyEntityLinkParser(expected));
assertEquals(
"expected: <<#E::table::user>name::column>> but was: <<#E::table::user>>",
exception.getMessage());

expected.clear();
expected.put("entityLink", "<#E::table::foo<>bar::baz>");
argException =
assertThrows(IllegalArgumentException.class, () -> verifyEntityLinkParser(expected));
assertEquals(
"Entity link was not found in <#E::table::foo<>bar::baz>", argException.getMessage());
}

void verifyEntityLinkParser(Map<String, String> expected) {
MessageParser.EntityLink entityLink =
MessageParser.EntityLink.parse(expected.get("entityLink"));
assertEquals(expected.get("entityLink"), entityLink.getLinkString());
assertEquals(expected.get("arrayFieldName"), entityLink.getArrayFieldName());
assertEquals(expected.get("arrayFieldValue"), entityLink.getArrayFieldValue());
assertEquals(expected.get("entityType"), entityLink.getEntityType());
assertEquals(expected.get("fieldName"), entityLink.getFieldName());
assertEquals(expected.get("entityFQN"), entityLink.getEntityFQN());
assertEquals(expected.get("linkType"), entityLink.getLinkType().toString());
assertEquals(expected.get("fullyQualifiedFieldType"), entityLink.getFullyQualifiedFieldType());
assertEquals(
expected.get("fullyQualifiedFieldValue"), entityLink.getFullyQualifiedFieldValue());
}
}
2 changes: 1 addition & 1 deletion openmetadata-spec/src/main/resources/json/schema/type/basic.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@
"entityLink": {
"description": "Link to an entity or field within an entity using this format `<#E::{entities}::{entityType}::{field}::{arrayFieldName}::{arrayFieldValue}`.",
"type": "string",
"pattern": "(?U)^<#E::\\w+::[\\w'\\- .&/:+\"\\\\()$#%]+>$"
"pattern": "(?U)^<#E::\\w+::(?:[^:<>|]|:[^:<>|])+(?:::(?:[^:<>|]|:[^:<>|])+)*>$"
},
"entityName": {
"description": "Name that identifies an entity.",
Expand Down
5 changes: 3 additions & 2 deletions ...rces/ui/src/components/DataQuality/AddDataQualityTest/components/AddTestSuitePipeline.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import {
FormItemLayout,
} from '../../../../interface/FormUtils.interface';
import { generateFormFields } from '../../../../utils/formUtils';
import { escapeESReservedCharacters } from '../../../../utils/StringsUtils';
import ScheduleInterval from '../../../Settings/Services/AddIngestion/Steps/ScheduleInterval';
import { WorkflowExtraConfig } from '../../../Settings/Services/AddIngestion/Steps/ScheduleInterval.interface';
import { AddTestCaseList } from '../../AddTestCaseList/AddTestCaseList.component';
Expand Down Expand Up @@ -152,9 +153,9 @@ const AddTestSuitePipeline = ({
]}
valuePropName="selectedTest">
<AddTestCaseList
filters={`testSuite.fullyQualifiedName:${
filters={`testSuite.fullyQualifiedName:${escapeESReservedCharacters(
testSuiteFQN ?? fqn
}`}
)}`}
showButton={false}
/>
</Form.Item>
Expand Down

0 comments on commit 4f8bf05

Please sign in to comment.