Skip to content
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
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
10 changes: 8 additions & 2 deletions build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -62,11 +62,17 @@ java {
withSourcesJar()
}

tasks.register('testJar', Jar) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

archiveClassifier.set('tests')
from sourceSets.test.output
}

publishing {
publications {
mavenJava(MavenPublication) {
artifactId = 'sdk-common-jvm'
from components.java
artifact testJar // Include the test-jar in the published artifacts
versionMapping {
usage('java-api') {
fromResolutionOf('runtimeClasspath')
Expand Down Expand Up @@ -114,7 +120,7 @@ publishing {

// 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') {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

h4x0r

doLast {
if (!project.hasProperty('release') && !project.hasProperty('snapshot')) {
throw new GradleException("You must specify either -Prelease or -Psnapshot")
Expand All @@ -135,7 +141,7 @@ tasks.named('publish').configure {
}

// Conditionally enable or disable publishing tasks
tasks.withType(PublishToMavenRepository) {
tasks.withType(PublishToMavenRepository).configureEach {
onlyIf {
project.ext.has('shouldPublish') && project.ext.shouldPublish
}
Expand Down
135 changes: 22 additions & 113 deletions src/main/java/cloud/eppo/BaseEppoClient.java
Original file line number Diff line number Diff line change
Expand Up @@ -22,37 +22,46 @@ public class BaseEppoClient {
private final ObjectMapper mapper =
new ObjectMapper()
.registerModule(EppoModule.eppoModule()); // TODO: is this the best place for this?
private static final String DEFAULT_HOST = "https://fscdn.eppo.cloud";
private static final boolean DEFAULT_IS_GRACEFUL_MODE = true;

protected static final String DEFAULT_HOST = "https://fscdn.eppo.cloud";
protected final ConfigurationRequestor requestor;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

protected so upstream SDKs can trigger a (re)load


private final ConfigurationStore configurationStore;
private final ConfigurationRequestor requestor;
private final AssignmentLogger assignmentLogger;
private final BanditLogger banditLogger;
private final String sdkName;
private final String sdkVersion;
private final boolean isConfigObfuscated;
private boolean isGracefulMode;

private static BaseEppoClient instance;

// Fields useful for testing in situations where we want to mock the http client or configuration
// store (accessed via reflection)
/** @noinspection FieldMayBeFinal */
private static EppoHttpClient httpClientOverride = null;

private BaseEppoClient(
public BaseEppoClient(
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to making this protected

String apiKey,
String sdkName,
String sdkVersion,
String host,
ConfigurationStore configurationStore,
AssignmentLogger assignmentLogger,
BanditLogger banditLogger,
boolean isGracefulMode) {
boolean isGracefulMode,
boolean expectObfuscatedConfig) {

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;
}
Comment on lines +52 to +61
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


EppoHttpClient httpClient = buildHttpClient(host, apiKey, sdkName, sdkVersion);
this.configurationStore = configurationStore;
this.configurationStore = new ConfigurationStore();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

requestor = new ConfigurationRequestor(configurationStore, httpClient);
this.assignmentLogger = assignmentLogger;
this.banditLogger = banditLogger;
Expand All @@ -61,8 +70,10 @@ private BaseEppoClient(
this.sdkName = sdkName;
this.sdkVersion = sdkVersion;
// For now, the configuration is only obfuscated for Android clients
this.isConfigObfuscated = sdkName.toLowerCase().contains("android");
this.isConfigObfuscated = expectObfuscatedConfig;
Copy link
Collaborator

Choose a reason for hiding this comment

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

πŸ‘


// TODO: caching initialization (such as setting an API-key-specific prefix
// will probably involve passing in configurationStore to the constructor
}

private EppoHttpClient buildHttpClient(
Expand All @@ -78,51 +89,7 @@ private EppoHttpClient buildHttpClient(
return httpClient;
}

public static BaseEppoClient init(
String apiKey,
String sdkName,
String sdkVersion,
String host,
AssignmentLogger assignmentLogger,
BanditLogger banditLogger,
boolean isGracefulMode) {

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 (instance != null) {
// TODO: also check we're not running a test
log.warn("Reinitializing an Eppo Client instance that was already initialized");
}
instance =
new BaseEppoClient(
apiKey,
sdkName,
sdkVersion,
host,
new ConfigurationStore(),
assignmentLogger,
banditLogger,
isGracefulMode);
instance.refreshConfiguration();

return instance;
}

/**
* Ability to ad-hoc kick off a configuration load. Will load from a filesystem cached file as
* well as fire off an HTTPS request for an updated configuration. If the cache load finishes
* first, those assignments will be used until the fetch completes.
*
* <p>Deprecated, as we plan to make a more targeted and configurable way to do so in the future.
*/
@Deprecated
public void refreshConfiguration() {
protected void loadConfiguration() {
requestor.load();
}

Expand Down Expand Up @@ -479,65 +446,7 @@ private <T> T throwIfNotGraceful(Exception e, T defaultValue) {
throw new RuntimeException(e);
}

public static BaseEppoClient getInstance() {
if (BaseEppoClient.instance == null) {
throw new IllegalStateException("Eppo SDK has not been initialized");
}

return BaseEppoClient.instance;
}

public void setIsGracefulFailureMode(boolean isGracefulFailureMode) {
this.isGracefulMode = isGracefulFailureMode;
}

public static class Builder {
private String apiKey;
private String sdkName;
private String sdkVersion;
private String host = DEFAULT_HOST;
private AssignmentLogger assignmentLogger;
private BanditLogger banditLogger;
private boolean isGracefulMode = DEFAULT_IS_GRACEFUL_MODE;

public Builder apiKey(String apiKey) {
this.apiKey = apiKey;
return this;
}

public Builder sdkName(String sdkName) {
this.sdkName = sdkName;
return this;
}

public Builder sdkVersion(String sdkVersion) {
this.sdkVersion = sdkVersion;
return this;
}

public Builder host(String host) {
this.host = host;
return this;
}

public Builder assignmentLogger(AssignmentLogger assignmentLogger) {
this.assignmentLogger = assignmentLogger;
return this;
}

public Builder banditLogger(BanditLogger banditLogger) {
this.banditLogger = banditLogger;
return this;
}

public Builder isGracefulMode(boolean isGracefulMode) {
this.isGracefulMode = isGracefulMode;
return this;
}

public BaseEppoClient buildAndInit() {
return BaseEppoClient.init(
apiKey, sdkName, sdkVersion, host, assignmentLogger, banditLogger, isGracefulMode);
}
}
}
27 changes: 9 additions & 18 deletions src/main/java/cloud/eppo/EppoHttpClient.java
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ public Response get(String path) {
}
}

public void get(String path, RequestCallback callback) {
public void get(String path, EppoHttpClientRequestCallback callback) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was hard to use this upstream due to the visibility of RequestCallback

HttpUrl httpUrl =
HttpUrl.parse(baseUrl + path)
.newBuilder()
Expand All @@ -83,13 +83,11 @@ public void onResponse(Call call, Response response) {
callback.onFailure("Failed to read response from URL " + httpUrl);
}
} else {
switch (response.code()) {
case HttpURLConnection.HTTP_FORBIDDEN:
callback.onFailure("Invalid API key");
break;
default:
log.debug("Fetch failed with status code: " + response.code());
callback.onFailure("Bad response from URL " + httpUrl);
if (response.code() == HttpURLConnection.HTTP_FORBIDDEN) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to IntelliJ (and I agree!) A switch with a single case should be an if πŸ€“

Copy link
Collaborator

Choose a reason for hiding this comment

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

😎

callback.onFailure("Invalid API key");
} else {
log.debug("Fetch failed with status code: {}", response.code());
callback.onFailure("Bad response from URL " + httpUrl);
}
}
response.close();
Expand All @@ -98,19 +96,12 @@ public void onResponse(Call call, Response response) {
@Override
public void onFailure(Call call, IOException e) {
log.error(
"Http request failure: "
+ e.getMessage()
+ " "
+ Arrays.toString(e.getStackTrace()),
"Http request failure: {} {}",
e.getMessage(),
Arrays.toString(e.getStackTrace()),
e);
callback.onFailure("Unable to fetch from URL " + httpUrl);
}
});
}
}

interface RequestCallback {
void onSuccess(String responseBody);

void onFailure(String errorMessage);
}
7 changes: 7 additions & 0 deletions src/main/java/cloud/eppo/EppoHttpClientRequestCallback.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
package cloud.eppo;

public interface EppoHttpClientRequestCallback {
void onSuccess(String responseBody);

void onFailure(String errorMessage);
}
6 changes: 3 additions & 3 deletions src/main/java/cloud/eppo/ufc/dto/DiscriminableAttributes.java
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,9 @@

public interface DiscriminableAttributes {

public Attributes getNumericAttributes();
Attributes getNumericAttributes();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No need for public in an interface as everything is assumed public


public Attributes getCategoricalAttributes();
Attributes getCategoricalAttributes();

public Attributes getAllAttributes();
Attributes getAllAttributes();
}
Loading