Skip to content

Fix invalid characters for project, featureset, entity and features creation #976

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
feast-ci-bot merged 2 commits into from
Sep 2, 2020
Merged

Fix invalid characters for project, featureset, entity and features creation #976

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e478d39
Fix invalid characters for project creation
terryyylim Sep 1, 2020
26a8f0d
Make error message clearer
terryyylim Sep 1, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions core/src/main/java/feast/core/service/SpecService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down Expand Up @@ -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()) {
Expand Down Expand Up @@ -239,7 +239,7 @@ public ListFeaturesResponse listFeatures(ListFeaturesRequest.Filter filter) {
List<String> entities = filter.getEntitiesList();
Map<String, String> labels = filter.getLabelsMap();

checkValidCharactersAllowAsterisk(project, "projectName");
checkValidCharactersAllowAsterisk(project, "project");

// Autofill default project if project not specified
if (project.isEmpty()) {
Expand Down
6 changes: 3 additions & 3 deletions core/src/main/java/feast/core/validators/FeatureSetValidator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,15 +45,15 @@ public static void validateSpec(FeatureSet featureSet) {
}

checkValidCharacters(featureSet.getSpec().getProject(), "project");
checkValidCharacters(featureSet.getSpec().getName(), "name");
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");
}
Expand Down
26 changes: 15 additions & 11 deletions core/src/main/java/feast/core/validators/Matchers.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,52 +22,56 @@ 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 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,
"argument must only contain alphanumeric characters, dashes and underscores."));
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."));
}
}
Expand Down
24 changes: 24 additions & 0 deletions core/src/test/java/feast/core/service/SpecServiceIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,6 +228,30 @@ 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 resource, %s: %s",
"featureset",
"dash-name",
"argument must only contain alphanumeric characters and underscores.")));
}

@Test
public void shouldReturnFeatureSetIfFeatureSetHasNotChanged() {
FeatureSetProto.FeatureSet featureSet = apiClient.getFeatureSet("default", "fs1");
Expand Down
20 changes: 11 additions & 9 deletions core/src/test/java/feast/core/validators/MatchersTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,42 +30,44 @@ 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
public void checkUpperSnakeCaseShouldThrowIllegalArgumentExceptionWithFieldForInvalidString() {
exception.expect(IllegalArgumentException.class);
exception.expectMessage(
Strings.lenientFormat(
"invalid value for field %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
public void checkLowerSnakeCaseShouldThrowIllegalArgumentExceptionWithFieldForInvalidString() {
exception.expect(IllegalArgumentException.class);
exception.expectMessage(
Strings.lenientFormat(
"invalid value for field %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");
}
}