Updates for Configuration Generification#257
Conversation
hbordersTwitch
left a comment
hbordersTwitch
left a comment
There was a problem hiding this comment.
Some guidance for reviewers
| MockitoAnnotations.openMocks(this); | ||
| } | ||
|
|
||
| private static class TestBuilder extends AndroidBaseClient.Builder< |
There was a problem hiding this comment.
We must make an explicit class so that we can pass a class literal that captures the generic types we pass up to AndroidBaseClient.Builder. Then, all of our builder methods can downcast to TestBuilder properly.
| protected CachingConfigurationStore( | ||
| @NotNull ConfigurationCodec<Configuration> codec, @NotNull ByteStore byteStore) { | ||
| @NotNull ConfigurationCodec<ConfigurationType> codec, @NotNull ByteStore byteStore) { | ||
| this.configuration = codec.emptyConfiguration(); |
There was a problem hiding this comment.
This allows us to use any SerializableEppoConfiguration, not just Configuration
| * Generic equivalent to {@link Configuration#emptyConfig()} | ||
| * @return an empty Configuration. | ||
| */ | ||
| @NotNull ConfigurationType emptyConfiguration(); |
There was a problem hiding this comment.
For use in CachingConfigurationStore.init
| this.configClass = configClass; | ||
| } | ||
|
|
||
| public static class Default implements ConfigurationCodec<Configuration> { |
There was a problem hiding this comment.
Since Default depends on Jackson, consumers would only ever use Configuration here, so there's no point in adding generics. Other ConfigurationCodec implementations will not use Jackson.
| private long pollingJitterMs; | ||
|
|
||
| @Nullable private static AndroidBaseClient<?> instance; | ||
| @Nullable private static AndroidBaseClient<?, ?, ?> instance; |
There was a problem hiding this comment.
This isn't really safe. Consumers probably won't have two different eppo instances with different generics in the same classpath, but if they do, this will cause ClassCastExceptions
| private final Application application; | ||
| private final ConfigurationParser<JsonFlagType> configurationParser; | ||
| private final EppoConfigurationClient configurationClient; | ||
| protected final Class<SelfType> selfClass; |
There was a problem hiding this comment.
Made all of these protected so that a subclass can access them. This might not be necessary though.
| // Optional parameters with defaults | ||
| @Nullable private String apiBaseUrl; | ||
| @Nullable private AssignmentLogger assignmentLogger; | ||
| @Nullable private CachingConfigurationStore configStore; |
There was a problem hiding this comment.
Made configStore required because we can't create a FileBackedConfigStore as a convenience anymore since we don't want to depend on Configuration
| @NotNull ConfigurationParser<ConfigurationType, ConfigurationBuilderType, JsonFlagType> configurationParser, | ||
| @NotNull CachingConfigurationStore<ConfigurationType> configStore, | ||
| @NotNull EppoConfigurationClient configurationClient) { | ||
| if (selfClass == null) { |
There was a problem hiding this comment.
EppoClient.Builder checks for nulls in its build methods, so I thought it made sense to check for them also. It's better to check for final methods at the point of initialization rather than in the build methods tho. I didn't change the EppoClent.Builder behavior.
| String sdkName = obfuscateConfig ? "android" : "android-debug"; | ||
| String sdkVersion = BuildConfig.EPPO_VERSION; | ||
|
|
||
| if (configStore == null) { |
There was a problem hiding this comment.
We can't created a FileBackedConfigStore in AndroidBaseClient anymore because we don't have a ConfigurationCodec, and we can't create a ConfigurationCode because it needs to be generic over SerializableEppoConfiguration
| private AndroidBaseClient<JsonElement> client; | ||
| private AndroidBaseClient<Configuration, Configuration.Builder, JsonElement> client; | ||
|
|
||
| private class GsonAndroidBaseClientBuilder extends AndroidBaseClient.Builder< |
There was a problem hiding this comment.
We must make an explicit class so that we can pass a class literal that captures the generic types we pass up to AndroidBaseClient.Builder. Then, all of our builder methods can downcast to GsonAndroidBaseClientBuilder properly.
1 participant
Eppo Internal:
🎟️ Fixes issue
📜 Design Doc: link if applicable
Motivation and Context
Needed to update classes dependent on https://github.com/Eppo-exp/sdk-common-jdk/ classes.
Needed to ensure that
AndroidBaseClienthad noConfigurationdependencies.See Eppo-exp/sdk-common-jdk#241
Description
AndroidBaseClientgeneric overSerializableEppoConfigurationandSerializableEppoConfiguration.AbstractBuildertypes for compatibility withBaseEppoClientchanges in Made Configuration generic to enable injectable JSON parsing sdk-common-jdk#241AndroidBaseClient.Buildergeneric overSerializableEppoConfigurationandSerializableEppoConfiguration.AbstractBuilderso that subclasses can reuse it in their builders.AndroidBaseClient.Builder.configStore()because we can't create aFileBackedConfigStorefor the consumer anymore sinceAndroidBaseClient.Builderis now generic overSerializableEppoConfiguration.AndroidBaseClient.Builder.initCachingConfigurationStoreandFiledBackedConfigStoregeneric overSerializableEppoConfigurationso that they can store anySerializableEppoConfigurationand not justConfiguration.ConfigurationCodec.emptyConfiguration()so thatCachingConfigurationStoreto remove aConfigurationdependency fromCachingConfigurationStoreConfigurationCodec.DefaultbecauseConfigurationCodec.Defaultdepends on Jackson for parsing, so onlyConfigurationmakes sense to use with it.How has this been documented?
Added javadoc
How has this been tested?
Ran unit and Android tests