Skip to content

Added new operator to check IP within range #185

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
merged 6 commits into from
Jun 16, 2023
Merged

Added new operator to check IP within range #185

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
582bb08
Added new operator to check ip within range
sanket-mundra Jun 12, 2023
59ea812
Updated enums
sanket-mundra Jun 16, 2023
8f89d9a
Updated ht config-service version
sanket-mundra Jun 16, 2023
79280c6
Updated converters for StringCondition value changes
sanket-mundra Jun 16, 2023
fd2eca9
Reverted breaking changes | addressed review comment
sanket-mundra Jun 16, 2023
d7258e5
Removed redundant deserializer
sanket-mundra Jun 16, 2023
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
27 changes: 25 additions & 2 deletions ...ls-schema-api/src/main/java/org/hypertrace/graphql/label/schema/rule/StringCondition.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,28 +3,51 @@
import graphql.annotations.annotationTypes.GraphQLField;
import graphql.annotations.annotationTypes.GraphQLName;
import graphql.annotations.annotationTypes.GraphQLNonNull;
import java.util.List;
import javax.annotation.Nullable;

@GraphQLName(StringCondition.TYPE_NAME)
public interface StringCondition {
String TYPE_NAME = "LabelApplicationStringCondition";

String OPERATOR_KEY = "operator";
String VALUE_KEY = "value";
String VALUES_KEY = "values";
String STRING_CONDITION_VALUE_TYPE_KEY = "stringConditionValueType";

@GraphQLName(Operator.TYPE_NAME)
enum Operator {
OPERATOR_EQUALS,
OPERATOR_MATCHES_REGEX;
OPERATOR_MATCHES_REGEX,
OPERATOR_MATCHES_IPS,
OPERATOR_NOT_MATCHES_IPS;
private static final String TYPE_NAME = "StringConditionOperator";
}

@GraphQLName(StringConditionValueType.TYPE_NAME)
enum StringConditionValueType {
VALUE,
VALUES;
private static final String TYPE_NAME = "StringConditionValueType";
}

@GraphQLField
@GraphQLNonNull
@GraphQLName(OPERATOR_KEY)
Operator operator();

@GraphQLField
@GraphQLNonNull
@Nullable
Copy link
Contributor

@aaron-steinfeld aaron-steinfeld Jun 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another alternative would be to have value be typed Object and accept either a string or list of strings in it. We're not consistent, we've taken both approaches in different places. Up to you

Copy link
Contributor Author

@sanket-mundra sanket-mundra Jun 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If no harm in this approach, will keep it this way.

Copy link
Contributor

@aaron-steinfeld aaron-steinfeld Jun 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no harm, the difference is how the callers would use it. In the read direction: whether they have to ask for 3 fields (value, values, type), then use type to determine which field should be read; or they ask for one field (value) and inspect the runtime value and use it accordingly. write direction: whether they have to set two fields (either value or values, then assigning type correctly depending on), or just assigning value with either one (in which case GQL now has to inspect it and determine which it is.

So the explicit is more verbose, but they both accomplish same thing.

@GraphQLName(VALUE_KEY)
String value();

@GraphQLField
@Nullable
@GraphQLName(VALUES_KEY)
List<String> values();

@GraphQLField
@Nullable
@GraphQLName(STRING_CONDITION_VALUE_TYPE_KEY)
StringConditionValueType stringConditionValueType();
}
21 changes: 17 additions & 4 deletions .../src/main/java/org/hypertrace/graphql/label/dao/LabelApplicationRuleRequestConverter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,10 +130,19 @@ LeafCondition convertLeafCondition(

StringCondition convertStringCondition(
org.hypertrace.graphql.label.schema.rule.StringCondition stringCondition) {
return StringCondition.newBuilder()
.setOperator(convertStringConditionOperator(stringCondition.operator()))
.setValue(stringCondition.value())
.build();
StringCondition.Builder stringConditionBuilder =
StringCondition.newBuilder()
.setOperator(convertStringConditionOperator(stringCondition.operator()));
switch (stringCondition.stringConditionValueType()) {
case VALUES:
return stringConditionBuilder
.setValues(
StringCondition.StringList.newBuilder().addAllValues(stringCondition.values()))
.build();
case VALUE:
default:
return stringConditionBuilder.setValue(stringCondition.value()).build();
}
}

UnaryCondition convertUnaryCondition(
Expand All @@ -150,6 +159,10 @@ StringCondition.Operator convertStringConditionOperator(
return StringCondition.Operator.OPERATOR_EQUALS;
case OPERATOR_MATCHES_REGEX:
return StringCondition.Operator.OPERATOR_MATCHES_REGEX;
case OPERATOR_MATCHES_IPS:
return StringCondition.Operator.OPERATOR_MATCHES_IPS;
case OPERATOR_NOT_MATCHES_IPS:
return StringCondition.Operator.OPERATOR_NOT_MATCHES_IPS;
default:
throw new IllegalArgumentException("Unsupported String Condition Operator");
}
Expand Down
29 changes: 28 additions & 1 deletion ...src/main/java/org/hypertrace/graphql/label/dao/LabelApplicationRuleResponseConverter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,28 @@ private Optional<StringCondition> convertStringCondition(
.StringCondition
stringCondition) {
Optional<StringCondition.Operator> operator = convertOperatorInStringCondition(stringCondition);
return operator.map(op -> new ConvertedStringCondition(op, stringCondition.getValue()));
switch (stringCondition.getKindCase()) {
case VALUE:
return operator.map(
op ->
new ConvertedStringCondition(
op,
stringCondition.getValue(),
null,
StringCondition.StringConditionValueType.VALUE));
case VALUES:
return operator.map(
op ->
new ConvertedStringCondition(
op,
null,
stringCondition.getValues().getValuesList(),
StringCondition.StringConditionValueType.VALUES));
default:
log.error(
"Unrecognized String Condition Value Type {}", stringCondition.getKindCase().name());
return Optional.empty();
}
}

private Optional<StringCondition.Operator> convertOperatorInStringCondition(
Expand All @@ -228,6 +249,10 @@ private Optional<StringCondition.Operator> convertOperatorInStringCondition(
return Optional.of(StringCondition.Operator.OPERATOR_EQUALS);
case OPERATOR_MATCHES_REGEX:
return Optional.of(StringCondition.Operator.OPERATOR_MATCHES_REGEX);
case OPERATOR_MATCHES_IPS:
return Optional.of(StringCondition.Operator.OPERATOR_MATCHES_IPS);
case OPERATOR_NOT_MATCHES_IPS:
return Optional.of(StringCondition.Operator.OPERATOR_NOT_MATCHES_IPS);
default:
log.error(
"Unrecognized Operator Type in String Condition {}",
Expand Down Expand Up @@ -316,6 +341,8 @@ private static class ConvertedUnaryCondition implements UnaryCondition {
private static class ConvertedStringCondition implements StringCondition {
Operator operator;
String value;
List<String> values;
StringConditionValueType stringConditionValueType;
}

@Value
Expand Down
146 changes: 0 additions & 146 deletions ...pertrace/graphql/label/deserialization/LabelApplicationRuleDataDeserializationConfig.java
View file
Open in desktop

This file was deleted.

6 changes: 6 additions & 0 deletions ...g/hypertrace/graphql/label/deserialization/LabelApplicationRuleDeserializationConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,12 @@ private static class StringConditionArgument implements StringCondition {

@JsonProperty(VALUE_KEY)
String value;

@JsonProperty(VALUES_KEY)
Copy link
Contributor

@aaron-steinfeld aaron-steinfeld Jun 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it's pre-existing, but why did this class get implemented twice? Seems like a mistake here.

Copy link
Contributor Author

@sanket-mundra sanket-mundra Jun 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

private static class LabelApplicationRuleArgument is missing in one of them. Should I remove one of them, the one without LabelApplicationRuleArgument class?

Copy link
Contributor

@aaron-steinfeld aaron-steinfeld Jun 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. I'm guessing there's a misunderstanding on how deserialization works, but there's only one jackson mapper so no interface needs to be implemented twice

List<String> values;

@JsonProperty(STRING_CONDITION_VALUE_TYPE_KEY)
StringConditionValueType stringConditionValueType;
}

@Value
Expand Down
3 changes: 0 additions & 3 deletions ...g/hypertrace/graphql/label/deserialization/LabelApplicationRuleDeserializationModule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,6 @@ protected void configure() {
Multibinder<ArgumentDeserializationConfig> deserializationConfigBinder =
Multibinder.newSetBinder(binder(), ArgumentDeserializationConfig.class);

deserializationConfigBinder
.addBinding()
.to(LabelApplicationRuleDataDeserializationConfig.class);
deserializationConfigBinder.addBinding().to(LabelApplicationRuleDeserializationConfig.class);

deserializationConfigBinder
Expand Down
2 changes: 1 addition & 1 deletion hypertrace-graphql-platform/build.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ dependencies {
api("org.hypertrace.entity.service:entity-type-service-rx-client:0.5.6")
api("org.hypertrace.config.service:spaces-config-service-api:0.1.1")
api("org.hypertrace.config.service:labels-config-service-api:0.1.15")
api("org.hypertrace.config.service:label-application-rule-config-service-api:0.1.16")
api("org.hypertrace.config.service:label-application-rule-config-service-api:0.1.52")
api("org.hypertrace.config.service:span-processing-config-service-api:0.1.41")
}
}