Add MatchRegistry for custom match implementations. #390

mikecdavis · 2020-08-07T19:04:43Z

Summary

Replace MatchType in favor of MatchRegistry
Add conditionValue to Match#match interface.
Remove the abstract AttributeMatch class.
Remove ExactNumberMatch class.
Add NumberComparator for standardizing Number comparisons
Refactor Semantic Versioning to use static convenience method

Incorporating a public MatchRegistry will allow consumers to provide their own implementations of the Match interface.

coveralls · 2020-08-07T19:24:19Z

Pull Request Test Coverage Report for Build 1544

83 of 87 (95.4%) changed or added relevant lines in 17 files are covered.
2 unchanged lines in 1 file lost coverage.
Overall coverage increased (+0.1%) to 89.703%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
core-api/src/main/java/com/optimizely/ab/config/audience/match/MatchRegistry.java	21	22	95.45%
core-api/src/main/java/com/optimizely/ab/config/audience/match/NumberComparator.java	4	5	80.0%
core-api/src/main/java/com/optimizely/ab/config/audience/UserAttribute.java	13	15	86.67%

Files with Coverage Reduction	New Missed Lines	%
core-api/src/main/java/com/optimizely/ab/config/audience/match/SubstringMatch.java	2	75.0%

Totals
Change from base Build 1539:	0.1%
Covered Lines:	4051
Relevant Lines:	4516

💛 - Coveralls

mikecdavis · 2020-08-07T19:52:12Z

Currently failing a single scenario in the FSC:

msohailhussain · 2020-09-15T18:49:20Z

core-api/src/main/java/com/optimizely/ab/config/audience/match/GTMatch.java

    }
-}
+}


msohailhussain

lgtm. few nits

msohailhussain · 2020-09-15T18:49:55Z

core-api/src/main/java/com/optimizely/ab/config/audience/match/MatchRegistry.java

+    private static final Map<String, Match> registry = new ConcurrentHashMap<>();
+    public static final String EXISTS = "exists";
+    public static final String EXACT = "exact";
+    public static final String GREATER_THAN = "gt";


nit: alphabetize.

msohailhussain · 2020-09-15T18:52:04Z

core-api/src/main/java/com/optimizely/ab/config/audience/match/NumberComparator.java

+            throw new UnknownValueTypeException();
+        }
+
+        if (!isValidNumber(o2)) {


Can we merge into the above if condition?

msohailhussain · 2020-09-15T18:52:41Z

core-api/src/main/java/com/optimizely/ab/config/audience/match/NumberComparator.java

+        return compareUnsafe(o1, o2);
+    }
+
+    public static int compareUnsafe(Object o1, Object o2) {


doc string will be helpful here.

msohailhussain · 2020-09-15T18:55:09Z

core-api/src/main/java/com/optimizely/ab/config/audience/match/SubstringMatch.java

@@ -18,11 +18,9 @@

 import javax.annotation.Nullable;

-class SubstringMatch extends AttributeMatch<String> {
-    String value;
+class SubstringMatch implements Match {


update Copyright year.

jaeopt

It looks great. One change suggested.

jaeopt · 2020-09-15T20:33:09Z

core-api/src/main/java/com/optimizely/ab/config/audience/match/Match.java

@@ -20,5 +20,5 @@

 public interface Match {
    @Nullable
-    Boolean eval(Object attributeValue);
+    Boolean eval(Object conditionValue, Object attributeValue) throws UnexpectedValueTypeException, UnknownValueTypeException;


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

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.

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".

Got it. We can consider adding some clarifications.

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

mikecdavis added the chore label Aug 7, 2020

mikecdavis requested a review from a team as a code owner August 7, 2020 19:04

optimizely-codereview-bot assigned mikecdavis Aug 7, 2020

mikecdavis force-pushed the mikecdavis/matcher-registry branch 2 times, most recently from 1743c70 to f30e088 Compare September 9, 2020 21:41

mikecdavis removed the chore label Sep 10, 2020

mikecdavis requested review from thomaszurkan-optimizely and msohailhussain September 10, 2020 00:52

Mike Davis added 11 commits September 15, 2020 10:49

Add registry.

7aeb262

Fix remaining tests.

5a76aae

Update doc string.

1ad8d43

Refactor semver.

fc73e8e

Add UnknownValueTypeExceptioni.

26557fd

Remove null check.

df8ea55

Add and refactor tests.

0d3eea6

Move SemanticVersionTest.java.

d51cc8b

Add missing unit tests.

49fabda

Add unit test for matcher registry.

74c13e9

Fix linter.

fd41198

mikecdavis force-pushed the mikecdavis/matcher-registry branch from ba30742 to fd41198 Compare September 15, 2020 17:49

Restore protected constructors.

f3ec462

msohailhussain reviewed Sep 15, 2020

View reviewed changes

core-api/src/main/java/com/optimizely/ab/config/audience/match/GTMatch.java Outdated

}

}

}

Copy link

Contributor

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

Remove constructors add doc strings.

95fc33c

msohailhussain approved these changes Sep 15, 2020

View reviewed changes

Address PR comments.

b656b0c

jaeopt requested changes Sep 15, 2020

View reviewed changes

jaeopt approved these changes Sep 15, 2020

View reviewed changes

Add doc strings for the Match exceptions.

2bf7f0a

mikecdavis merged commit c6da789 into master Sep 15, 2020

mikecdavis deleted the mikecdavis/matcher-registry branch September 15, 2020 22:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add MatchRegistry for custom match implementations. #390

Add MatchRegistry for custom match implementations. #390

Uh oh!

mikecdavis commented Aug 7, 2020 •

edited

Loading

Uh oh!

coveralls commented Aug 7, 2020 •

edited

Loading

Uh oh!

mikecdavis commented Aug 7, 2020

Uh oh!

msohailhussain Sep 15, 2020

Uh oh!

msohailhussain left a comment

Uh oh!

msohailhussain Sep 15, 2020

Uh oh!

msohailhussain Sep 15, 2020

Uh oh!

msohailhussain Sep 15, 2020

Uh oh!

msohailhussain Sep 15, 2020

Uh oh!

jaeopt left a comment

Uh oh!

jaeopt Sep 15, 2020

Uh oh!

mikecdavis Sep 15, 2020

Uh oh!

mikecdavis Sep 15, 2020

Uh oh!

jaeopt Sep 15, 2020

Uh oh!

mikecdavis Sep 15, 2020

Uh oh!

Uh oh!

Add MatchRegistry for custom match implementations. #390

Add MatchRegistry for custom match implementations. #390

Uh oh!

Conversation

mikecdavis commented Aug 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Uh oh!

coveralls commented Aug 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 1544

💛 - Coveralls

Uh oh!

mikecdavis commented Aug 7, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

msohailhussain left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaeopt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mikecdavis commented Aug 7, 2020 •

edited

Loading

coveralls commented Aug 7, 2020 •

edited

Loading