Skip to content

Add MatchRegistry for custom match implementations. #390

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 15 commits into from
Sep 15, 2020
Merged

Add MatchRegistry for custom match implementations. #390

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
7aeb262
Add registry.
Aug 7, 2020
5a76aae
Fix remaining tests.
Aug 7, 2020
1ad8d43
Update doc string.
Aug 7, 2020
fc73e8e
Refactor semver.
Sep 4, 2020
26557fd
Add UnknownValueTypeExceptioni.
Sep 8, 2020
df8ea55
Remove null check.
Sep 9, 2020
0d3eea6
Add and refactor tests.
Sep 9, 2020
d51cc8b
Move SemanticVersionTest.java.
Sep 9, 2020
49fabda
Add missing unit tests.
Sep 9, 2020
74c13e9
Add unit test for matcher registry.
Sep 10, 2020
fd41198
Fix linter.
Sep 10, 2020
f3ec462
Restore protected constructors.
Sep 15, 2020
95fc33c
Remove constructors add doc strings.
Sep 15, 2020
b656b0c
Address PR comments.
Sep 15, 2020
2bf7f0a
Add doc strings for the Match exceptions.
Sep 15, 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
56 changes: 27 additions & 29 deletions core-api/src/main/java/com/optimizely/ab/config/audience/UserAttribute.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,7 @@
import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
import com.fasterxml.jackson.annotation.JsonProperty;
import com.optimizely.ab.config.ProjectConfig;
import com.optimizely.ab.config.audience.match.Match;
import com.optimizely.ab.config.audience.match.MatchType;
import com.optimizely.ab.config.audience.match.UnexpectedValueTypeException;
import com.optimizely.ab.config.audience.match.UnknownMatchTypeException;
import com.optimizely.ab.config.audience.match.*;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

Expand Down Expand Up @@ -87,35 +84,36 @@ public Boolean evaluate(ProjectConfig config, Map<String, ?> attributes) {
}
// check user attribute value is equal
try {
Match matchType = MatchType.getMatchType(match, value).getMatcher();
Boolean result = matchType.eval(userAttributeValue);

Match matcher = MatchRegistry.getMatch(match);
Boolean result = matcher.eval(value, userAttributeValue);
if (result == null) {
if (!attributes.containsKey(name)) {
//Missing attribute value
logger.debug("Audience condition \"{}\" evaluated to UNKNOWN because no value was passed for user attribute \"{}\"", this, name);
throw new UnknownValueTypeException();
}

return result;
} catch(UnknownValueTypeException e) {
if (!attributes.containsKey(name)) {
//Missing attribute value
logger.debug("Audience condition \"{}\" evaluated to UNKNOWN because no value was passed for user attribute \"{}\"", this, name);
} else {
//if attribute value is not valid
if (userAttributeValue != null) {
logger.warn(
"Audience condition \"{}\" evaluated to UNKNOWN because a value of type \"{}\" was passed for user attribute \"{}\"",
this,
userAttributeValue.getClass().getCanonicalName(),
name);
} else {
//if attribute value is not valid
if (userAttributeValue != null) {
logger.warn(
"Audience condition \"{}\" evaluated to UNKNOWN because a value of type \"{}\" was passed for user attribute \"{}\"",
this,
userAttributeValue.getClass().getCanonicalName(),
name);
} else {
logger.debug(
"Audience condition \"{}\" evaluated to UNKNOWN because a null value was passed for user attribute \"{}\"",
this,
name);
}
logger.debug(
"Audience condition \"{}\" evaluated to UNKNOWN because a null value was passed for user attribute \"{}\"",
this,
name);
}
}
return result;
} catch (UnknownMatchTypeException | UnexpectedValueTypeException ex) {
logger.warn("Audience condition \"{}\" " + ex.getMessage(),
this);
} catch (NullPointerException np) {
logger.error("attribute or value null for match {}", match != null ? match : "legacy condition", np);
} catch (UnknownMatchTypeException | UnexpectedValueTypeException e) {
logger.warn("Audience condition \"{}\" " + e.getMessage(), this);
} catch (NullPointerException e) {
logger.error("attribute or value null for match {}", match != null ? match : "legacy condition", e);
}
return null;
}
Expand Down
17 changes: 10 additions & 7 deletions ...rc/main/java/com/optimizely/ab/config/audience/match/DefaultMatchForLegacyAttributes.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,17 +21,20 @@
/**
* This is a temporary class. It mimics the current behaviour for
* legacy custom attributes. This will be dropped for ExactMatch and the unit tests need to be fixed.
* @param <T>
*/
class DefaultMatchForLegacyAttributes<T> extends AttributeMatch<T> {
T value;
class DefaultMatchForLegacyAttributes implements Match {

protected DefaultMatchForLegacyAttributes(T value) {
this.value = value;
protected DefaultMatchForLegacyAttributes() {
}

@Nullable
public Boolean eval(Object attributeValue) {
return value.equals(castToValueType(attributeValue, value));
public Boolean eval(Object conditionValue, Object attributeValue) throws UnexpectedValueTypeException {
if (!(conditionValue instanceof String)) {
throw new UnexpectedValueTypeException();
}
if (attributeValue == null) {
return false;
}
return conditionValue.toString().equals(attributeValue.toString());
}
}
28 changes: 20 additions & 8 deletions core-api/src/main/java/com/optimizely/ab/config/audience/match/ExactMatch.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,18 +18,30 @@

import javax.annotation.Nullable;

class ExactMatch<T> extends AttributeMatch<T> {
T value;
import static com.optimizely.ab.internal.AttributesUtil.isValidNumber;

protected ExactMatch(T value) {
this.value = value;
class ExactMatch implements Match {

protected ExactMatch() {
}

@Nullable
public Boolean eval(Object attributeValue) {
T converted = castToValueType(attributeValue, value);
if (value != null && converted == null) return null;
public Boolean eval(Object conditionValue, Object attributeValue) throws UnexpectedValueTypeException {
if (isValidNumber(attributeValue)) {
if (isValidNumber(conditionValue)) {
return NumberComparator.compareUnsafe(attributeValue, conditionValue) == 0;
}
return null;
}

if (!(conditionValue instanceof String || conditionValue instanceof Boolean)) {
throw new UnexpectedValueTypeException();
}

if (attributeValue == null || attributeValue.getClass() != conditionValue.getClass()) {
return null;
}

return value == null ? attributeValue == null : value.equals(converted);
return conditionValue.equals(attributeValue);
}
}
47 changes: 0 additions & 47 deletions core-api/src/main/java/com/optimizely/ab/config/audience/match/ExactNumberMatch.java
View file
Open in desktop

This file was deleted.

9 changes: 2 additions & 7 deletions core-api/src/main/java/com/optimizely/ab/config/audience/match/ExistsMatch.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,20 +16,15 @@
*/
package com.optimizely.ab.config.audience.match;

import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;

import javax.annotation.Nullable;

class ExistsMatch implements Match {
@SuppressFBWarnings("URF_UNREAD_FIELD")
Object value;

protected ExistsMatch(Object value) {
this.value = value;
protected ExistsMatch() {
}

@Nullable
public Boolean eval(Object attributeValue) {
public Boolean eval(Object conditionValue, Object attributeValue) {
return attributeValue != null;
}
}
19 changes: 4 additions & 15 deletions core-api/src/main/java/com/optimizely/ab/config/audience/match/GEMatch.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,24 +18,13 @@

import javax.annotation.Nullable;

import static com.optimizely.ab.internal.AttributesUtil.isValidNumber;
class GEMatch implements Match {

class GEMatch extends AttributeMatch<Number> {
Number value;

protected GEMatch(Number value) {
this.value = value;
protected GEMatch() {
}

@Nullable
public Boolean eval(Object attributeValue) {
try {
if(isValidNumber(attributeValue)) {
return castToValueType(attributeValue, value).doubleValue() >= value.doubleValue();
}
} catch (Exception e) {
return null;
}
return null;
public Boolean eval(Object conditionValue, Object attributeValue) throws UnknownValueTypeException {
return NumberComparator.compare(attributeValue, conditionValue) >= 0;
}
}
21 changes: 5 additions & 16 deletions core-api/src/main/java/com/optimizely/ab/config/audience/match/GTMatch.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,24 +18,13 @@

import javax.annotation.Nullable;

import static com.optimizely.ab.internal.AttributesUtil.isValidNumber;
class GTMatch implements Match {

class GTMatch extends AttributeMatch<Number> {
Number value;

protected GTMatch(Number value) {
this.value = value;
protected GTMatch() {
}

@Nullable
public Boolean eval(Object attributeValue) {
try {
if(isValidNumber(attributeValue)) {
return castToValueType(attributeValue, value).doubleValue() > value.doubleValue();
}
} catch (Exception e) {
return null;
}
return null;
public Boolean eval(Object conditionValue, Object attributeValue) throws UnknownValueTypeException {
return NumberComparator.compare(attributeValue, conditionValue) > 0;
}
}
}
Copy link
Contributor

@msohailhussain msohailhussain Sep 15, 2020

Choose a reason for hiding this comment

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

eol

19 changes: 4 additions & 15 deletions core-api/src/main/java/com/optimizely/ab/config/audience/match/LEMatch.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,25 +18,14 @@

import javax.annotation.Nullable;

import static com.optimizely.ab.internal.AttributesUtil.isValidNumber;
class LEMatch implements Match {

class LEMatch extends AttributeMatch<Number> {
Number value;

protected LEMatch(Number value) {
this.value = value;
protected LEMatch() {
}

@Nullable
public Boolean eval(Object attributeValue) {
try {
if(isValidNumber(attributeValue)) {
return castToValueType(attributeValue, value).doubleValue() <= value.doubleValue();
}
} catch (Exception e) {
return null;
}
return null;
public Boolean eval(Object conditionValue, Object attributeValue) throws UnknownValueTypeException {
return NumberComparator.compare(attributeValue, conditionValue) <= 0;
}
}

20 changes: 4 additions & 16 deletions core-api/src/main/java/com/optimizely/ab/config/audience/match/LTMatch.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,25 +18,13 @@

import javax.annotation.Nullable;

import static com.optimizely.ab.internal.AttributesUtil.isValidNumber;
class LTMatch implements Match {

class LTMatch extends AttributeMatch<Number> {
Number value;

protected LTMatch(Number value) {
this.value = value;
protected LTMatch() {
}

@Nullable
public Boolean eval(Object attributeValue) {
try {
if(isValidNumber(attributeValue)) {
return castToValueType(attributeValue, value).doubleValue() < value.doubleValue();
}
} catch (Exception e) {
return null;
}
return null;
public Boolean eval(Object conditionValue, Object attributeValue) throws UnknownValueTypeException {
return NumberComparator.compare(attributeValue, conditionValue) < 0;
}
}

2 changes: 1 addition & 1 deletion core-api/src/main/java/com/optimizely/ab/config/audience/match/Match.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,5 +20,5 @@

public interface Match {
@Nullable
Boolean eval(Object attributeValue);
Boolean eval(Object conditionValue, Object attributeValue) throws UnexpectedValueTypeException, UnknownValueTypeException;
Copy link
Contributor

@jaeopt jaeopt Sep 15, 2020

Choose a reason for hiding this comment

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

Do we need separate exception types for UnexpectedValueTypeException and UnknownValueTypeException?
They're similar and not much gain for additional complexity to differentiate them.

Copy link
Contributor Author

@mikecdavis mikecdavis Sep 15, 2020

Choose a reason for hiding this comment

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

This is for backwards compatibility, albeit I can update the names perhaps. The difference is in the logging behavior. One error is communicated as an SDK compatibility issue warning to upgrade on the condition value, while the other is saying that the attribute value type is unknown.

Copy link
Contributor Author

@mikecdavis mikecdavis Sep 15, 2020

Choose a reason for hiding this comment

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

The distinction is since the conditionValue comes from the datafile, so if a value is of an "unexpected" type then its likely the result of an out of date SDK. Since the attributeValue is provided at runtime via the user attributes, then we log that the type is "unknown".

Copy link
Contributor

@jaeopt jaeopt Sep 15, 2020

Choose a reason for hiding this comment

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

Got it. We can consider adding some clarifications.

Copy link
Contributor Author

@mikecdavis mikecdavis Sep 15, 2020

Choose a reason for hiding this comment

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

I added doc strings for all the exception classes in this package.

}
Loading