feat: Add ODP GraphQL Api Interface #481

zashraf1985 · 2022-07-29T21:16:02Z

Summary

Added an ODPApiManager interface and its Default implementation that implements the graphQL call to fetch qualified segments.
The mechanism to get default parser is tightly coupled with Project Config module. I created a separate Factory class to get the default parser. This can be used by any future implementations that need a default parser.
Added parsing for all four supported parsers to parse valid and error responses.

Test plan

Added new unit tests to test all the new code.
Existing unit tests pass
Existing Full stack compatibility suite tests pass

Issues

OASIS-8384

2. Added error response parsing and it's unit tests

Mat001 · 2022-08-01T16:26:40Z

core-httpclient-impl/src/main/java/com/optimizely/ab/odp/DefaultODPApiManager.java

+     }
+    */
+    @Override
+    public String fetchQualifiedSegments(String apiKey, String apiEndpoint, String userKey, String userValue, List<String> segmentsToCheck) {


Name "fetchQualifiedSegments" is already used in upcoming "ODPSegmentManager" (per Jae's naming). Risk of name conflict. Unless you use different names, but that might be a deviation from naming consistency across SDKs?

Mat001 · 2022-08-01T16:27:34Z

core-api/src/main/java/com/optimizely/ab/odp/ODPApiManager.java

+import java.util.List;
+
+public interface ODPApiManager {
+    String fetchQualifiedSegments(String apiKey, String apiEndpoint, String userKey, String userValue, List<String> segmentsToCheck);


Name already "reserved" for the method in ODPSegmentManager? This one should be fetchSegments, no? (consistency across SDKs)

Not 100% sure, maybe I'm wrong... you have ODPAPIManager interface that defines fetchQualifiedSegments method, but there is also method fetchSegments in Swift PR that is located in the GraphQL API Manager. Not sure how you are overriding, but it seems smth not quite right...unless it's my weak java knowledge

Design of java is a little different than other sdks. For all the external calls, we need to create an interface and pass it in to the core SDK from the outside. That is why i created ODPAPIManager which is an interface that will contain all the external calls such as graphQL and ODPEvents. Then i am providing a DefaultODPApiManager as a default implementation. user will use this by default but can pass in their own implementation. To me fethQualifiedSegments makes more sense because thats exactly what this call does. It fetches the qualified segments not all or just any segments.

Mat001 · 2022-08-01T16:32:39Z

core-httpclient-impl/src/test/java/com/optimizely/ab/odp/DefaultODPApiManagerTest.java

+        logbackVerifier.expectMessage(Level.ERROR, "Unexpected response from ODP server, Response code: 500, null");
+        assertEquals(null, responseString);
+    }
+}


What about the test for network issues, such as internet is down, where status code is not returned. Request is not reaching the server and bounces off?

jaeopt

A refactoring for sharing multiple JSON parsers suggested if it's not too much.
I need to see the next stage to see how you want to implement all. It looks good as is!

jaeopt · 2022-08-01T16:33:34Z

core-api/src/main/java/com/optimizely/ab/internal/JsonParserProvider.java

+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public enum JsonParserProvider {


Looks good! Just wondering if there is a smart way to reuse the same code for DefaultConfigParser. It is quite complicated currently supporting multiple parsers and this repeated code will make it worse :)

I wanted to reuse code from there but it was tightly coupled. I wanted to refactor DefaultConfigParser to use this one but i thought i will add a comment and do that later as a separate ticket. Right now it will become too much work. what do you think?

The idea is to make DefaultConfigParser and whatever else needs a parser use this code eventually. i am just too afraid to touch DafaultConfigParser in this PR

I leave it to you.

Cool! I will add a comment in the DefaultConfigParser mentioning the same. I will also add a ticket to refactor this and put it in the tech debt backlog.

jaeopt · 2022-08-01T16:56:08Z

core-api/src/main/java/com/optimizely/ab/odp/parser/impl/GsonParser.java

+                    logMessage.append(errors.get(i).getAsJsonObject().get("message").getAsString());
+                }
+                logger.error(logMessage.toString());
+                return null;


Collecting error messages looks good. For other SDKs (swift and go), extracting "InvalidIdentifierException" from error "classification" and passing back to clients is important. Not sure how we can pass back this info to callers in Java. We can log in at minimum, but this may be redundant to your error log message already collecting all error messages.

jaeopt · 2022-08-01T18:08:56Z

core-httpclient-impl/src/main/java/com/optimizely/ab/odp/DefaultODPApiManager.java

+            return null;
+        }
+
+        if (response.getStatusLine().getStatusCode() != HttpStatus.SC_OK) {


What about more generous error checking like ">= 400"?

jaeopt · 2022-08-01T18:23:00Z

core-httpclient-impl/src/main/java/com/optimizely/ab/odp/DefaultODPApiManager.java

+    */
+    @Override
+    public String fetchQualifiedSegments(String apiKey, String apiEndpoint, String userKey, String userValue, List<String> segmentsToCheck) {
+        HttpPost request = new HttpPost(apiEndpoint);


Not sure about the plan to feed endpoint, but needs "/v3/graphql" appended for graphql to the shred apiEndpoint.

I see this as the lowest level method that will make the actual call that is why it's getting the full endPoint. Users might provide their own implementation so it made more sense for it to expect the full endPoint. My plan is to build this endpoint using the host and path in the ODPSegmentManager and pass it to this one. What do you think?

That's what I thought. If we plan to support custom endpoints, this makes sense. Let's move on and review again at the top level.
One thing we also should consider is that if clients want to implement their own segment services (replacing all our default ODPManager), they can determine segments using their own solution and directly feed the "qualifedSegments" into OptimizelyUserContext, instead of injecting own ODPManager into our SDKs.

jaeopt · 2022-08-01T21:47:42Z

core-api/src/main/java/com/optimizely/ab/odp/ODPApiManager.java

+import java.util.List;
+
+public interface ODPApiManager {
+    String fetchQualifiedSegments(String apiKey, String apiEndpoint, String userKey, String userValue, List<String> segmentsToCheck);


It looks like you plan to open all ODP APIs here to public for customization support. It looks good for now and we can continue discussion the final apis.

Yes, this will contain all the methods that make an external call. ODPEvents calls will also go here

jaeopt

LGTM

Mat001

lgtm

zashraf1985 added 5 commits July 27, 2022 17:27

Development complete, Unit tests remaining.

6034617

Added parameterized tests for parsers

899fa88

1. Added unit tests for default parsers

b14562e

2. Added error response parsing and it's unit tests

Added tests for graphQL API call.

a7c66fc

Added copyrights headers and missing line breaks

889957c

zashraf1985 requested a review from a team as a code owner July 29, 2022 21:16

optimizely-codereview-bot assigned zashraf1985 Jul 29, 2022

zashraf1985 removed their assignment Jul 29, 2022

Mat001 requested changes Aug 1, 2022

View reviewed changes

optimizely-codereview-bot assigned zashraf1985 Aug 1, 2022

jaeopt requested changes Aug 1, 2022

View reviewed changes

incorporated review feedback

f793afc

zashraf1985 removed their assignment Aug 2, 2022

optimizely-codereview-bot requested review from jaeopt and Mat001 August 2, 2022 20:52

jaeopt approved these changes Aug 2, 2022

View reviewed changes

Mat001 approved these changes Aug 2, 2022

View reviewed changes

zashraf1985 merged commit 618377d into master Aug 2, 2022

zashraf1985 deleted the zeeshan/ats-graphql-and-parsers branch August 2, 2022 21:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add ODP GraphQL Api Interface #481

feat: Add ODP GraphQL Api Interface #481

zashraf1985 commented Jul 29, 2022

Mat001 Aug 1, 2022

Mat001 Aug 1, 2022

Mat001 Aug 1, 2022

zashraf1985 Aug 1, 2022

Mat001 Aug 1, 2022

jaeopt left a comment

jaeopt Aug 1, 2022

zashraf1985 Aug 2, 2022 •

edited

Loading

zashraf1985 Aug 2, 2022

jaeopt Aug 2, 2022

zashraf1985 Aug 2, 2022

jaeopt Aug 1, 2022

jaeopt Aug 1, 2022

jaeopt Aug 1, 2022

zashraf1985 Aug 2, 2022

jaeopt Aug 2, 2022

jaeopt Aug 1, 2022

zashraf1985 Aug 2, 2022

jaeopt left a comment

Mat001 left a comment

feat: Add ODP GraphQL Api Interface #481

feat: Add ODP GraphQL Api Interface #481

Conversation

zashraf1985 commented Jul 29, 2022

Summary

Test plan

Issues

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaeopt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zashraf1985 Aug 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaeopt left a comment

Choose a reason for hiding this comment

Mat001 left a comment

Choose a reason for hiding this comment

zashraf1985 Aug 2, 2022 •

edited

Loading