From e478d3977f5912dbba2d037852150c0bd5db1be2 Mon Sep 17 00:00:00 2001 From: Terence Date: Tue, 1 Sep 2020 18:02:21 +0800 Subject: [PATCH 1/2] Fix invalid characters for project creation --- .../core/validators/FeatureSetValidator.java | 4 ++-- .../java/feast/core/validators/Matchers.java | 6 ++--- .../feast/core/service/SpecServiceIT.java | 23 +++++++++++++++++++ .../feast/core/validators/MatchersTest.java | 4 ++-- 4 files changed, 30 insertions(+), 7 deletions(-) diff --git a/core/src/main/java/feast/core/validators/FeatureSetValidator.java b/core/src/main/java/feast/core/validators/FeatureSetValidator.java index 3433523661..549f5e3d55 100644 --- a/core/src/main/java/feast/core/validators/FeatureSetValidator.java +++ b/core/src/main/java/feast/core/validators/FeatureSetValidator.java @@ -44,8 +44,8 @@ public static void validateSpec(FeatureSet featureSet) { throw new IllegalArgumentException("Feature set label keys must not be empty"); } - checkValidCharacters(featureSet.getSpec().getProject(), "project"); - checkValidCharacters(featureSet.getSpec().getName(), "name"); + checkValidCharacters(featureSet.getSpec().getProject(), "project::name"); + checkValidCharacters(featureSet.getSpec().getName(), "featureset::name"); checkUniqueColumns( featureSet.getSpec().getEntitiesList(), featureSet.getSpec().getFeaturesList()); checkReservedColumns(featureSet.getSpec().getFeaturesList()); diff --git a/core/src/main/java/feast/core/validators/Matchers.java b/core/src/main/java/feast/core/validators/Matchers.java index 87e2b126f0..d13316b7c8 100644 --- a/core/src/main/java/feast/core/validators/Matchers.java +++ b/core/src/main/java/feast/core/validators/Matchers.java @@ -22,11 +22,11 @@ public class Matchers { private static Pattern UPPER_SNAKE_CASE_REGEX = Pattern.compile("^[A-Z0-9]+(_[A-Z0-9]+)*$"); private static Pattern LOWER_SNAKE_CASE_REGEX = Pattern.compile("^[a-z0-9]+(_[a-z0-9]+)*$"); - private static Pattern VALID_CHARACTERS_REGEX = Pattern.compile("^[a-zA-Z0-9\\-_]*$"); + private static Pattern VALID_CHARACTERS_REGEX = Pattern.compile("^[a-zA-Z_][a-zA-Z0-9_]*$"); private static Pattern VALID_CHARACTERS_REGEX_WITH_ASTERISK_WILDCARD = Pattern.compile("^[a-zA-Z0-9\\-_*]*$"); - private static String ERROR_MESSAGE_TEMPLATE = "invalid value for field %s: %s"; + private static String ERROR_MESSAGE_TEMPLATE = "invalid value for %s: %s"; public static void checkUpperSnakeCase(String input, String fieldName) throws IllegalArgumentException { @@ -57,7 +57,7 @@ public static void checkValidCharacters(String input, String fieldName) String.format( ERROR_MESSAGE_TEMPLATE, fieldName, - "argument must only contain alphanumeric characters, dashes and underscores.")); + "argument must only contain alphanumeric characters and underscores.")); } } diff --git a/core/src/test/java/feast/core/service/SpecServiceIT.java b/core/src/test/java/feast/core/service/SpecServiceIT.java index 75933d3ee2..440d4dd0cb 100644 --- a/core/src/test/java/feast/core/service/SpecServiceIT.java +++ b/core/src/test/java/feast/core/service/SpecServiceIT.java @@ -228,6 +228,29 @@ public void shouldThrowExceptionGivenReservedFeatureName() { reservedNamesString, "event_timestamp"))); } + @Test + public void shouldThrowExceptionGivenFeatureSetWithDash() { + StatusRuntimeException exc = + assertThrows( + StatusRuntimeException.class, + () -> + apiClient.simpleApplyFeatureSet( + DataGenerator.createFeatureSet( + DataGenerator.getDefaultSource(), + "project", + "dash-name", + ImmutableMap.of("entity", ValueProto.ValueType.Enum.STRING), + ImmutableMap.of("test_string", ValueProto.ValueType.Enum.STRING)))); + + assertThat( + exc.getMessage(), + equalTo( + String.format( + "INTERNAL: invalid value for %s: %s", + "featureset::name", + "argument must only contain alphanumeric characters and underscores."))); + } + @Test public void shouldReturnFeatureSetIfFeatureSetHasNotChanged() { FeatureSetProto.FeatureSet featureSet = apiClient.getFeatureSet("default", "fs1"); diff --git a/core/src/test/java/feast/core/validators/MatchersTest.java b/core/src/test/java/feast/core/validators/MatchersTest.java index 774e58c7a8..d312300e88 100644 --- a/core/src/test/java/feast/core/validators/MatchersTest.java +++ b/core/src/test/java/feast/core/validators/MatchersTest.java @@ -44,7 +44,7 @@ public void checkUpperSnakeCaseShouldThrowIllegalArgumentExceptionWithFieldForIn exception.expect(IllegalArgumentException.class); exception.expectMessage( Strings.lenientFormat( - "invalid value for field %s: %s", + "invalid value for %s: %s", "someField", "argument must be in upper snake case, and cannot include any special characters.")); String in = "redis"; @@ -62,7 +62,7 @@ public void checkLowerSnakeCaseShouldThrowIllegalArgumentExceptionWithFieldForIn exception.expect(IllegalArgumentException.class); exception.expectMessage( Strings.lenientFormat( - "invalid value for field %s: %s", + "invalid value for %s: %s", "someField", "argument must be in lower snake case, and cannot include any special characters.")); String in = "Invalid_feature name"; From 26a8f0d9d828bb050f672a648bc05c116ed636f8 Mon Sep 17 00:00:00 2001 From: Terence Date: Wed, 2 Sep 2020 00:29:51 +0800 Subject: [PATCH 2/2] Make error message clearer --- .../java/feast/core/service/SpecService.java | 8 +++---- .../core/validators/FeatureSetValidator.java | 8 +++---- .../java/feast/core/validators/Matchers.java | 22 +++++++++++-------- .../feast/core/service/SpecServiceIT.java | 5 +++-- .../feast/core/validators/MatchersTest.java | 20 +++++++++-------- 5 files changed, 35 insertions(+), 28 deletions(-) diff --git a/core/src/main/java/feast/core/service/SpecService.java b/core/src/main/java/feast/core/service/SpecService.java index e4b1ce5c43..e7a317b13c 100644 --- a/core/src/main/java/feast/core/service/SpecService.java +++ b/core/src/main/java/feast/core/service/SpecService.java @@ -98,7 +98,7 @@ public GetFeatureSetResponse getFeatureSet(GetFeatureSetRequest request) private FeatureSet getFeatureSet(String projectName, String featureSetName) { // Validate input arguments - checkValidCharacters(featureSetName, "featureSetName"); + checkValidCharacters(featureSetName, "featureset"); if (featureSetName.isEmpty()) { throw new IllegalArgumentException("No feature set name provided"); @@ -151,8 +151,8 @@ public ListFeatureSetsResponse listFeatureSets(ListFeatureSetsRequest.Filter fil "Invalid listFeatureSetRequest, missing arguments. Must provide feature set name:"); } - checkValidCharactersAllowAsterisk(name, "featureSetName"); - checkValidCharactersAllowAsterisk(project, "projectName"); + checkValidCharactersAllowAsterisk(name, "featureset"); + checkValidCharactersAllowAsterisk(project, "project"); // Autofill default project if project not specified if (project.isEmpty()) { @@ -239,7 +239,7 @@ public ListFeaturesResponse listFeatures(ListFeaturesRequest.Filter filter) { List entities = filter.getEntitiesList(); Map labels = filter.getLabelsMap(); - checkValidCharactersAllowAsterisk(project, "projectName"); + checkValidCharactersAllowAsterisk(project, "project"); // Autofill default project if project not specified if (project.isEmpty()) { diff --git a/core/src/main/java/feast/core/validators/FeatureSetValidator.java b/core/src/main/java/feast/core/validators/FeatureSetValidator.java index 549f5e3d55..8787d75f69 100644 --- a/core/src/main/java/feast/core/validators/FeatureSetValidator.java +++ b/core/src/main/java/feast/core/validators/FeatureSetValidator.java @@ -44,16 +44,16 @@ public static void validateSpec(FeatureSet featureSet) { throw new IllegalArgumentException("Feature set label keys must not be empty"); } - checkValidCharacters(featureSet.getSpec().getProject(), "project::name"); - checkValidCharacters(featureSet.getSpec().getName(), "featureset::name"); + checkValidCharacters(featureSet.getSpec().getProject(), "project"); + checkValidCharacters(featureSet.getSpec().getName(), "featureset"); checkUniqueColumns( featureSet.getSpec().getEntitiesList(), featureSet.getSpec().getFeaturesList()); checkReservedColumns(featureSet.getSpec().getFeaturesList()); for (EntitySpec entitySpec : featureSet.getSpec().getEntitiesList()) { - checkValidCharacters(entitySpec.getName(), "entities::name"); + checkValidCharacters(entitySpec.getName(), "entity"); } for (FeatureSpec featureSpec : featureSet.getSpec().getFeaturesList()) { - checkValidCharacters(featureSpec.getName(), "features::name"); + checkValidCharacters(featureSpec.getName(), "feature"); if (featureSpec.getLabelsMap().containsKey("")) { throw new IllegalArgumentException("Feature label keys must not be empty"); } diff --git a/core/src/main/java/feast/core/validators/Matchers.java b/core/src/main/java/feast/core/validators/Matchers.java index d13316b7c8..ba12a83a79 100644 --- a/core/src/main/java/feast/core/validators/Matchers.java +++ b/core/src/main/java/feast/core/validators/Matchers.java @@ -26,48 +26,52 @@ public class Matchers { private static Pattern VALID_CHARACTERS_REGEX_WITH_ASTERISK_WILDCARD = Pattern.compile("^[a-zA-Z0-9\\-_*]*$"); - private static String ERROR_MESSAGE_TEMPLATE = "invalid value for %s: %s"; + private static String ERROR_MESSAGE_TEMPLATE = "invalid value for %s resource, %s: %s"; - public static void checkUpperSnakeCase(String input, String fieldName) + public static void checkUpperSnakeCase(String input, String resource) throws IllegalArgumentException { if (!UPPER_SNAKE_CASE_REGEX.matcher(input).matches()) { throw new IllegalArgumentException( String.format( ERROR_MESSAGE_TEMPLATE, - fieldName, + resource, + input, "argument must be in upper snake case, and cannot include any special characters.")); } } - public static void checkLowerSnakeCase(String input, String fieldName) + public static void checkLowerSnakeCase(String input, String resource) throws IllegalArgumentException { if (!LOWER_SNAKE_CASE_REGEX.matcher(input).matches()) { throw new IllegalArgumentException( String.format( ERROR_MESSAGE_TEMPLATE, - fieldName, + resource, + input, "argument must be in lower snake case, and cannot include any special characters.")); } } - public static void checkValidCharacters(String input, String fieldName) + public static void checkValidCharacters(String input, String resource) throws IllegalArgumentException { if (!VALID_CHARACTERS_REGEX.matcher(input).matches()) { throw new IllegalArgumentException( String.format( ERROR_MESSAGE_TEMPLATE, - fieldName, + resource, + input, "argument must only contain alphanumeric characters and underscores.")); } } - public static void checkValidCharactersAllowAsterisk(String input, String fieldName) + public static void checkValidCharactersAllowAsterisk(String input, String resource) throws IllegalArgumentException { if (!VALID_CHARACTERS_REGEX_WITH_ASTERISK_WILDCARD.matcher(input).matches()) { throw new IllegalArgumentException( String.format( ERROR_MESSAGE_TEMPLATE, - fieldName, + resource, + input, "argument must only contain alphanumeric characters, dashes, underscores, or an asterisk.")); } } diff --git a/core/src/test/java/feast/core/service/SpecServiceIT.java b/core/src/test/java/feast/core/service/SpecServiceIT.java index 440d4dd0cb..697ad424d3 100644 --- a/core/src/test/java/feast/core/service/SpecServiceIT.java +++ b/core/src/test/java/feast/core/service/SpecServiceIT.java @@ -246,8 +246,9 @@ public void shouldThrowExceptionGivenFeatureSetWithDash() { exc.getMessage(), equalTo( String.format( - "INTERNAL: invalid value for %s: %s", - "featureset::name", + "INTERNAL: invalid value for %s resource, %s: %s", + "featureset", + "dash-name", "argument must only contain alphanumeric characters and underscores."))); } diff --git a/core/src/test/java/feast/core/validators/MatchersTest.java b/core/src/test/java/feast/core/validators/MatchersTest.java index d312300e88..eb9d987cbd 100644 --- a/core/src/test/java/feast/core/validators/MatchersTest.java +++ b/core/src/test/java/feast/core/validators/MatchersTest.java @@ -30,13 +30,13 @@ public class MatchersTest { @Test public void checkUpperSnakeCaseShouldPassForLegitUpperSnakeCase() { String in = "REDIS_DB"; - checkUpperSnakeCase(in, "someField"); + checkUpperSnakeCase(in, "featureset"); } @Test public void checkUpperSnakeCaseShouldPassForLegitUpperSnakeCaseWithNumbers() { String in = "REDIS1"; - checkUpperSnakeCase(in, "someField"); + checkUpperSnakeCase(in, "featureset"); } @Test @@ -44,17 +44,18 @@ public void checkUpperSnakeCaseShouldThrowIllegalArgumentExceptionWithFieldForIn exception.expect(IllegalArgumentException.class); exception.expectMessage( Strings.lenientFormat( - "invalid value for %s: %s", - "someField", + "invalid value for %s resource, %s: %s", + "featureset", + "redis", "argument must be in upper snake case, and cannot include any special characters.")); String in = "redis"; - checkUpperSnakeCase(in, "someField"); + checkUpperSnakeCase(in, "featureset"); } @Test public void checkLowerSnakeCaseShouldPassForLegitLowerSnakeCase() { String in = "feature_name_v1"; - checkLowerSnakeCase(in, "someField"); + checkLowerSnakeCase(in, "feature"); } @Test @@ -62,10 +63,11 @@ public void checkLowerSnakeCaseShouldThrowIllegalArgumentExceptionWithFieldForIn exception.expect(IllegalArgumentException.class); exception.expectMessage( Strings.lenientFormat( - "invalid value for %s: %s", - "someField", + "invalid value for %s resource, %s: %s", + "feature", + "Invalid_feature name", "argument must be in lower snake case, and cannot include any special characters.")); String in = "Invalid_feature name"; - checkLowerSnakeCase(in, "someField"); + checkLowerSnakeCase(in, "feature"); } }