Remove singleton pattern #28

aarsilv · 2024-08-13T21:47:36Z

Eppo Internal:
🎟️ Ticket: FF-3033 - Update common SDK repository for upstream use by Java SDK
👯‍♀️ Related PR: java-server-sdk #70

This update makes this common shared SDK easier to integrate with upstream SDKs. It accomplishes this with a few key changes:

Removing the singleton pattern and builder (Upstream SDKs can still use those patterns)
Make EppoHttpClient's request callback a public interface, EppoHttpClientRequestCallback
Reorganize and exposing testing functionality so they can be used by upstream SDKs as well

I also implemented some additional random cleanup as recommended by the IntelliJ IDE Inspections

aarsilv · 2024-08-13T21:49:08Z

build.gradle

  withSourcesJar()
 }

+tasks.register('testJar', Jar) {


Also providing a testJar artifact so upstream SDKs can use our handy test utilities. Sharing is caring!

aarsilv · 2024-08-13T21:49:52Z

build.gradle

 // Custom task to ensure we can conditionally publish either a release or snapshot artifact
 // based on a command line switch. See github workflow files for more details on usage.
-task checkVersion {
+tasks.register('checkVersion') {


IntelliJ said tasks.register() and later on configureEach below is more 1337

aarsilv · 2024-08-13T21:51:03Z

src/main/java/cloud/eppo/BaseEppoClient.java

-  private static final boolean DEFAULT_IS_GRACEFUL_MODE = true;
+
+  protected static final String DEFAULT_HOST = "https://fscdn.eppo.cloud";
+  protected final ConfigurationRequestor requestor;


protected so upstream SDKs can trigger a (re)load

aarsilv · 2024-08-13T21:51:27Z

src/main/java/cloud/eppo/BaseEppoClient.java

+    if (apiKey == null) {
+      throw new IllegalArgumentException("Unable to initialize Eppo SDK due to missing API key");
+    }
+    if (sdkName == null || sdkVersion == null) {
+      throw new IllegalArgumentException(
+          "Unable to initialize Eppo SDK due to missing SDK name or version");
+    }
+    if (host == null) {
+      host = DEFAULT_HOST;
+    }


Moved validation to the constructor so each SDK's builder doesn't have to worry about it

aarsilv · 2024-08-13T21:52:09Z

src/main/java/cloud/eppo/BaseEppoClient.java


    EppoHttpClient httpClient = buildHttpClient(host, apiKey, sdkName, sdkVersion);
-    this.configurationStore = configurationStore;
+    this.configurationStore = new ConfigurationStore();


we were always just passing in new ConfigurationStore() so in-lining this for now until we handle caching at which point I reckon this may find its way back to the constructor, but didn't want to put the cart before the horse.

aarsilv · 2024-08-13T21:56:17Z

src/test/java/cloud/eppo/BaseEppoClientTest.java

+  private final ObjectMapper mapper =
+      new ObjectMapper().registerModule(AssignmentTestCase.assignmentTestCaseModule());

+  private BaseEppoClient eppoClient;


This is not static because for these tests we often initialize a new client, so we use the two methods below to get a new client for each test as needed

aarsilv · 2024-08-13T21:56:48Z

src/test/java/cloud/eppo/BaseEppoClientTest.java


-  private void initClient(
-      String host, boolean isGracefulMode, boolean isConfigObfuscated, String apiKey) {
+  private void initClient(boolean isGracefulMode, boolean isConfigObfuscated) {


no need to pass in host and API key as they are always the same

aarsilv · 2024-08-13T21:57:23Z

src/test/java/cloud/eppo/BaseEppoClientTest.java

    verify(mockAssignmentLogger, times(1)).logAssignment(assignmentLogCaptor.capture());
  }

-  private void mockHttpResponse(String responseBody) {


Moved to a newly-created TestUtils class so it can be used upstream

aarsilv · 2024-08-13T21:58:28Z

src/test/java/cloud/eppo/helpers/AssignmentTestCase.java


-  public void setFileName(String fileName) {
-    this.fileName = fileName;
+  private static final ObjectMapper mapper =


Below is where I relocated all the useful AssignmentTestCase related utility methods. They are public so they can be used by upstream SDks

aarsilv · 2024-08-13T21:58:54Z

src/test/java/cloud/eppo/helpers/BanditTestCase.java


-  public void setFileName(String fileName) {
-    this.fileName = fileName;
+  public static Stream<Arguments> getBanditTestData() {


And here is why I relocated all the BanditTestCase-related utility methods, also public.

typotter

Looks great. appreciate all the comments to navigate the changes!

typotter · 2024-08-14T18:16:03Z

src/main/java/cloud/eppo/BaseEppoClient.java

  private static EppoHttpClient httpClientOverride = null;

-  private BaseEppoClient(
+  public BaseEppoClient(


Should this be protected so it's only callable by subclasses?

Good idea! I had it public as it's used by automated tests, but forgot it will still have package access. Will change!

+1 to making this protected

typotter · 2024-08-14T18:18:16Z

src/main/java/cloud/eppo/BaseEppoClient.java

    this.sdkVersion = sdkVersion;
    // For now, the configuration is only obfuscated for Android clients
-    this.isConfigObfuscated = sdkName.toLowerCase().contains("android");
+    this.isConfigObfuscated = expectObfuscatedConfig;


typotter · 2024-08-14T18:24:08Z

src/main/java/cloud/eppo/EppoHttpClient.java

-                    default:
-                      log.debug("Fetch failed with status code: " + response.code());
-                      callback.onFailure("Bad response from URL " + httpUrl);
+                  if (response.code() == HttpURLConnection.HTTP_FORBIDDEN) {


felipecsl

LGTM!

felipecsl · 2024-08-14T22:40:23Z

src/main/java/cloud/eppo/BaseEppoClient.java

  private static EppoHttpClient httpClientOverride = null;

-  private BaseEppoClient(
+  public BaseEppoClient(


+1 to making this protected

felipecsl · 2024-08-14T22:43:05Z

src/test/java/cloud/eppo/helpers/TestUtils.java

+import java.lang.reflect.Field;
+import okhttp3.*;
+
+public class TestUtils {


super minor nit: good practice to make "utils"-like classes final to tell users they're not supposed to subclass it

felipecsl · 2024-08-14T22:44:13Z

src/test/java/cloud/eppo/helpers/TestUtils.java

+        .get(anyString(), any(EppoHttpClientRequestCallback.class));
+
+    setBaseClientHttpClientOverrideField(mockHttpClient);
+  }


I'd honestly prefer if we used OkHttp's excellent MockWebServer (https://github.com/square/okhttp/tree/master/mockwebserver) but this is low priority

* Eppo Client with shared UFC tests passing (#23) * tests passing for rule evaluator, flag evaluator, and eppo value * work in progress * shared UFC tests passing * don't check in test data * changes from self-review of PR * apply spotless linter * working on tests * better test logging * use make test for tests * Add bandit support (#25) * bandit test harness ready * add bandit result * set up for dropping in bandit evaluation * bandit deserialization wired up * loading bandit parameters * bandit stuff happening * shared bandit tests passing * bandit logger classes * bandit log test passing * more tests for logger * bandit tests for graceful mode * apply spotless formatting autofix * changes from self-review of PR so far * more changes from self-review of PR * more changes from self-review * make test less fragile * bump version; don't sign local maven * bandit logging errors should be non-fatal * use normalized probability floor * update result before even attempting to log bandit * spotless 🙄 * do the rename (#26) * Remove singleton pattern (#28) * work in progress * remove singleton for base client * linter * expose bandit test harnesses * expose test uilities * changes from self-review of PR * make base client constructor protected

aarsilv added 6 commits August 12, 2024 22:38

work in progress

e2f9074

remove singleton for base client

e7880d5

linter

b964d83

expose bandit test harnesses

58e28d6

expose test uilities

5e856be

changes from self-review of PR

b10ffb0

aarsilv commented Aug 13, 2024

View reviewed changes

aarsilv mentioned this pull request Aug 13, 2024

Update to support UFC configuration files via using the shared common SDK Eppo-exp/java-server-sdk#70

Merged

aarsilv marked this pull request as ready for review August 13, 2024 22:16

aarsilv requested review from felipecsl, rasendubi and typotter August 13, 2024 22:16

aarsilv assigned felipecsl and typotter Aug 13, 2024

typotter approved these changes Aug 14, 2024

View reviewed changes

typotter assigned aarsilv and unassigned typotter Aug 14, 2024

felipecsl approved these changes Aug 14, 2024

View reviewed changes

make base client constructor protected

1519ba6

aarsilv merged commit ee5abd1 into aaron/ff-2710/ufc Aug 15, 2024

aarsilv deleted the aaron/ff-3033/update-for-java-sdk branch August 15, 2024 03:47

aarsilv mentioned this pull request Aug 15, 2024

Bring UFC-based EppoClient into Common #29

Merged

Remove singleton pattern #28

Remove singleton pattern #28

Uh oh!

Conversation

aarsilv commented Aug 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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!

typotter 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!

felipecsl 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!

Uh oh!

aarsilv commented Aug 13, 2024 •

edited

Loading