feat: add android-sdk-framework module#251
Conversation
Add new android-sdk-framework module that provides base client functionality and configuration storage abstractions for Android SDK implementations. Key components: - AndroidBaseClient: Base class for Android SDK clients - CachingConfigurationStore: Abstract store for cached configurations - FileBackedConfigStore: File-based configuration persistence - ConfigurationCodec: Pluggable serialization/deserialization - ByteStore: Low-level byte storage abstraction This module uses the v4 Java SDK framework and is added as a standalone module without integration into the existing eppo module yet.
The v4 framework snapshots are deployed to the new Maven Central snapshot repository at https://central.sonatype.com/repository/maven-snapshots, not the old OSSRH at https://oss.sonatype.org/content/repositories/snapshots/
- resumePolling(): store pollingIntervalMs/pollingJitterMs on instance so pause/resume works after initial polling is started via Builder - safeCacheKey(): guard against keys shorter than 8 characters with Math.min to avoid StringIndexOutOfBoundsException - BaseCacheFile.setContents(): use try-with-resources to close writer even when write() throws - FileBackedByteStore: use a dedicated single-thread executor for I/O instead of ForkJoinPool.commonPool() to avoid pool saturation on low-core-count devices - ConfigCacheFile: document why deprecated package-private constructors exist (v3→v4 migration support) - build.gradle: clarify intent of EPPO_VERSION build config field
- safeCacheKey(): add null/empty guard returning "" to match Copilot suggestion; also prevents NPE on null key during initialization - GsonConfigurationCodec: delete from framework module; uses reflection on private fields of Configuration which R8 will rename in release builds (no proguard keep rules), causing NoSuchFieldException at runtime — flagged by both Copilot and code reviewer
|
@copilot review |
There was a problem hiding this comment.
Pull request overview
Adds a new android-sdk-framework Android library module that provides a shared base client and configuration persistence abstractions for Android SDK implementations, while keeping the existing :eppo module unchanged.
Changes:
- Introduces
AndroidBaseClient(builder-based singleton) extending the v4 Java SDK framework client. - Adds storage abstractions (
CachingConfigurationStore,ByteStore,ConfigurationCodec) and a file-backed persistence implementation. - Adds Robolectric unit tests, an instrumentation test for polling pause/resume, and a large JSON fixture used by tests.
Reviewed changes
Copilot reviewed 19 out of 20 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
settings.gradle |
Adds the new module include and adjusts the snapshots Maven repo URL. |
android-sdk-framework/build.gradle |
Declares the new Android library module, dependencies, publishing/signing, and Spotless config. |
android-sdk-framework/src/main/AndroidManifest.xml |
Minimal manifest for the library module. |
android-sdk-framework/src/main/java/cloud/eppo/android/framework/AndroidBaseClient.java |
Base Android client with builder/init flow and polling pause/resume. |
android-sdk-framework/src/main/java/cloud/eppo/android/framework/exceptions/NotInitializedException.java |
Exception thrown when accessing the singleton before initialization. |
android-sdk-framework/src/main/java/cloud/eppo/android/framework/exceptions/EppoInitializationException.java |
Initialization error wrapper exception. |
android-sdk-framework/src/main/java/cloud/eppo/android/framework/util/Utils.java |
Shared helpers for log tags and cache key derivation. |
android-sdk-framework/src/main/java/cloud/eppo/android/framework/storage/ByteStore.java |
Async byte I/O abstraction. |
android-sdk-framework/src/main/java/cloud/eppo/android/framework/storage/BaseCacheFile.java |
File wrapper for cache read/write helpers. |
android-sdk-framework/src/main/java/cloud/eppo/android/framework/storage/ConfigCacheFile.java |
Cache filename/extension mapping based on content type. |
android-sdk-framework/src/main/java/cloud/eppo/android/framework/storage/ConfigurationCodec.java |
Pluggable config serialization API with Java-serialization default. |
android-sdk-framework/src/main/java/cloud/eppo/android/framework/storage/CachingConfigurationStore.java |
In-memory config cache with async persistence support. |
android-sdk-framework/src/main/java/cloud/eppo/android/framework/storage/FileBackedByteStore.java |
File-backed ByteStore implementation. |
android-sdk-framework/src/main/java/cloud/eppo/android/framework/storage/FileBackedConfigStore.java |
Wiring of CachingConfigurationStore + file-backed persistence. |
android-sdk-framework/src/test/resources/flags-v1.json |
Large JSON fixture used to create a non-empty configuration in tests. |
android-sdk-framework/src/test/java/cloud/eppo/android/framework/storage/CachingConfigurationStoreTest.java |
Tests for caching, persistence behavior, and concurrency scenarios. |
android-sdk-framework/src/test/java/cloud/eppo/android/framework/storage/ConfigurationCodecTest.java |
Tests for default codec behavior and error cases. |
android-sdk-framework/src/test/java/cloud/eppo/android/framework/storage/FileBackedByteStoreTest.java |
Tests for file-backed byte store read/write behavior. |
android-sdk-framework/src/test/java/cloud/eppo/android/framework/storage/FileBackedConfigStoreTest.java |
Tests for file-backed config store behavior. |
android-sdk-framework/src/androidTest/java/cloud/eppo/android/framework/EppoClientPollingTest.java |
Instrumentation tests for polling pause/resume call safety. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @Nullable private static AndroidBaseClient<?> instance; | ||
|
|
||
| /** | ||
| * Private constructor. Use Builder to construct instances. | ||
| * | ||
| * @param apiKey API key for Eppo | ||
| * @param sdkName SDK name identifier | ||
| * @param sdkVersion SDK version string | ||
| * @param apiBaseUrl Base URL for API calls | ||
| * @param assignmentLogger Logger for assignments | ||
| * @param configurationStore Store for configuration persistence | ||
| * @param isGracefulMode Whether to operate in graceful mode | ||
| * @param expectObfuscatedConfig Whether configuration is obfuscated | ||
| * @param initialConfiguration Initial configuration future | ||
| * @param assignmentCache Cache for assignments | ||
| * @param configurationParser Parser for configuration JSON | ||
| * @param configurationClient HTTP client for configuration fetching | ||
| */ | ||
| protected AndroidBaseClient( | ||
| String apiKey, | ||
| String sdkName, | ||
| String sdkVersion, | ||
| @Nullable String apiBaseUrl, | ||
| @Nullable AssignmentLogger assignmentLogger, | ||
| CachingConfigurationStore configurationStore, | ||
| boolean isGracefulMode, | ||
| boolean expectObfuscatedConfig, | ||
| @Nullable CompletableFuture<Configuration> initialConfiguration, | ||
| @Nullable IAssignmentCache assignmentCache, | ||
| ConfigurationParser<JsonFlagType> configurationParser, | ||
| EppoConfigurationClient configurationClient) { | ||
| super( | ||
| apiKey, | ||
| sdkName, | ||
| sdkVersion, | ||
| apiBaseUrl, | ||
| assignmentLogger, | ||
| null, // banditLogger is not supported in Android | ||
| configurationStore, | ||
| isGracefulMode, | ||
| expectObfuscatedConfig, | ||
| false, // no bandits. | ||
| initialConfiguration, | ||
| assignmentCache, | ||
| null, | ||
| configurationParser, | ||
| configurationClient); | ||
| } | ||
|
|
||
| /** | ||
| * Gets the singleton instance of EppoClient. | ||
| * | ||
| * @return The singleton instance | ||
| * @throws NotInitializedException if the client has not been initialized | ||
| * @param <T> The JSON type parameter | ||
| */ | ||
| @SuppressWarnings("unchecked") | ||
| public static <T> AndroidBaseClient<T> getInstance() throws NotInitializedException { | ||
| if (instance == null) { | ||
| throw new NotInitializedException(); | ||
| } | ||
| return (AndroidBaseClient<T>) instance; | ||
| } |
There was a problem hiding this comment.
instance is a mutable static used for a singleton but isn't volatile and access isn't synchronized. If initialization and getInstance() can occur across threads, this risks visibility issues (reading a partially constructed instance) and racy reinitialization. Consider making instance volatile and/or synchronizing the initialization path (or using an AtomicReference).
|
|
||
| // Dedicated single-thread executor avoids saturating ForkJoinPool.commonPool() with blocking I/O | ||
| // on low-core-count Android devices. | ||
| private static final Executor IO_EXECUTOR = Executors.newSingleThreadExecutor(); |
There was a problem hiding this comment.
IO_EXECUTOR is a static newSingleThreadExecutor() that is never shut down and uses a non-daemon thread by default. This can leak a thread for the lifetime of the process and can also prevent JVM test runners from exiting cleanly. Consider using a ThreadFactory that creates daemon/named threads and/or an ExecutorService that can be shut down (or delegate to an app-provided executor).
| private static final Executor IO_EXECUTOR = Executors.newSingleThreadExecutor(); | |
| private static final Executor IO_EXECUTOR = | |
| Executors.newSingleThreadExecutor(r -> { | |
| Thread t = new Thread(r); | |
| t.setName("FileBackedByteStore-IO"); | |
| t.setDaemon(true); | |
| return t; | |
| }); |
| * | ||
| * @param configClass the class of the configuration type | ||
| */ | ||
| public Default(@NotNull Class<T> configClass) { |
There was a problem hiding this comment.
Default accepts configClass as @NotNull but doesn't check it. If callers accidentally pass null, fromBytes() will throw a confusing NPE when calling configClass.isInstance(...). Consider validating configClass in the constructor and throwing IllegalArgumentException if it's null.
| public Default(@NotNull Class<T> configClass) { | |
| public Default(@NotNull Class<T> configClass) { | |
| if (configClass == null) { | |
| throw new IllegalArgumentException("configClass must not be null"); | |
| } |
| package cloud.eppo.android.framework.util; | ||
|
|
||
| public class Utils { | ||
| public static String logTag(Class loggingClass) { |
There was a problem hiding this comment.
logTag uses a raw Class type and doesn't guard against null, which can lead to a runtime NPE (loggingClass.getSimpleName()). Consider changing the signature to Class<?> (or @NotNull Class<?>) and validating the argument.
| public static String logTag(Class loggingClass) { | |
| public static String logTag(Class<?> loggingClass) { | |
| if (loggingClass == null) { | |
| // Fallback tag when no class is provided | |
| return "EppoSDK"; | |
| } |
| mavenLocal() | ||
| maven { | ||
| url "https://central.sonatype.com/repository/maven-snapshots/" | ||
| url "https://central.sonatype.com/repository/maven-snapshots" |
There was a problem hiding this comment.
Maven repository base URLs typically need a trailing slash for correct URI resolution of artifact paths. Without it, some resolvers can treat the last path segment as a file and resolve dependencies incorrectly. Consider restoring the trailing / on this snapshots repo URL.
| url "https://central.sonatype.com/repository/maven-snapshots" | |
| url "https://central.sonatype.com/repository/maven-snapshots/" |
| public AndroidBaseClient<JsonFlagType> buildAndInit() { | ||
| try { | ||
| return buildAndInitAsync().get(); | ||
| } catch (ExecutionException | InterruptedException | CompletionException e) { | ||
| // If the exception was an `EppoInitializationException`, we know for sure that | ||
| // `buildAndInitAsync` logged it (and wrapped it with a RuntimeException) which was then | ||
| // wrapped by `CompletableFuture` with a `CompletionException`. | ||
| if (e instanceof CompletionException) { | ||
| Throwable cause = e.getCause(); | ||
| if (cause instanceof RuntimeException | ||
| && cause.getCause() instanceof EppoInitializationException) { | ||
| @SuppressWarnings("unchecked") | ||
| AndroidBaseClient<JsonFlagType> typedInstance = | ||
| (AndroidBaseClient<JsonFlagType>) instance; | ||
| return typedInstance; | ||
| } | ||
| } | ||
| Log.e(TAG, "Exception caught during initialization: " + e.getMessage(), e); | ||
| if (!isGracefulMode) { | ||
| throw new RuntimeException(e); | ||
| } | ||
| } |
There was a problem hiding this comment.
buildAndInit() catches InterruptedException but does not restore the thread interrupt status (e.g., via Thread.currentThread().interrupt()). Swallowing interrupts can break caller cancellation/timeout logic. Consider re-interrupting the thread when InterruptedException is caught (and decide whether to propagate or return gracefully).
| AndroidBaseClient<JsonNode> androidBaseClient = buildOfflineClientWithoutPolling(); | ||
| assertNotNull("Client should be initialized", androidBaseClient); | ||
|
|
||
| // Try to resume polling (should log warning and not crash per EppoClient.java:436-441) |
There was a problem hiding this comment.
These comments reference EppoClient.java:436-441, but this module uses AndroidBaseClient and line numbers will drift over time. Consider referencing the specific method (AndroidBaseClient.resumePolling) or the expected behavior instead of a hard-coded file/line reference.
| // Try to resume polling (should log warning and not crash per EppoClient.java:436-441) | |
| // Try to resume polling; AndroidBaseClient.resumePolling should log a warning and not crash if polling was never started |
| } else if (offlineMode || failCount.incrementAndGet() == 2) { | ||
| ret.completeExceptionally( | ||
| new EppoInitializationException( | ||
| "Unable to initialize client; Configuration could not be loaded", ex)); | ||
| } else { | ||
| Log.i(TAG, "Initial config was not used."); | ||
| failCount.incrementAndGet(); |
There was a problem hiding this comment.
The failCount increment logic here double-increments on the first "initial config not used" path: failCount.incrementAndGet() == 2 is evaluated (incrementing), then the else block increments again. This can cause initialization to fail/short-circuit earlier than intended. Refactor to increment exactly once per failure (e.g., store the incremented value in a local variable and reuse it in the condition/logging).
| } else if (offlineMode || failCount.incrementAndGet() == 2) { | |
| ret.completeExceptionally( | |
| new EppoInitializationException( | |
| "Unable to initialize client; Configuration could not be loaded", ex)); | |
| } else { | |
| Log.i(TAG, "Initial config was not used."); | |
| failCount.incrementAndGet(); | |
| } else { | |
| int failures = failCount.incrementAndGet(); | |
| if (offlineMode || failures == 2) { | |
| ret.completeExceptionally( | |
| new EppoInitializationException( | |
| "Unable to initialize client; Configuration could not be loaded", | |
| ex)); | |
| } else { | |
| Log.i(TAG, "Initial config was not used."); | |
| } |
| private volatile Configuration configuration = Configuration.emptyConfig(); | ||
|
|
||
| protected CachingConfigurationStore( | ||
| @NotNull ConfigurationCodec<Configuration> codec, @NotNull ByteStore byteStore) { |
There was a problem hiding this comment.
The constructor accepts codec/byteStore as @NotNull but doesn't validate them. Passing null will lead to later NPEs that are harder to diagnose. Consider adding explicit null checks and throwing IllegalArgumentException with a clear message.
| @NotNull ConfigurationCodec<Configuration> codec, @NotNull ByteStore byteStore) { | |
| @NotNull ConfigurationCodec<Configuration> codec, @NotNull ByteStore byteStore) { | |
| if (codec == null) { | |
| throw new IllegalArgumentException("codec must not be null"); | |
| } | |
| if (byteStore == null) { | |
| throw new IllegalArgumentException("byteStore must not be null"); | |
| } |
| ConfigCacheFile( | ||
| @NotNull Application application, | ||
| @NotNull String configType, | ||
| @NotNull String suffix, | ||
| @NotNull String contentType) { | ||
| this(application, configType + "-" + suffix, contentType); | ||
| } | ||
|
|
||
| /** | ||
| * Creates a cache file with the given full file name (no prefix). Used when the caller supplies | ||
| * the complete filename (e.g. baseName + "." + extension). | ||
| * | ||
| * @deprecated Use {@link #ConfigCacheFile(Application, String, String)} instead. These | ||
| * package-private constructors exist only to support migration of the eppo module from v3 to | ||
| * v4; they will be removed once that migration is complete. | ||
| */ | ||
| ConfigCacheFile(@NotNull Application application, @NotNull String fullFileName) { | ||
| super(application, fullFileName); |
There was a problem hiding this comment.
These constructors are documented with @deprecated in Javadoc but aren't annotated with @Deprecated. Without the annotation, callers won't get compile-time deprecation warnings. Consider adding @Deprecated to each deprecated constructor (and optionally @SuppressWarnings("deprecation") in call sites if needed).
aarsilv
left a comment
aarsilv
left a comment
There was a problem hiding this comment.
Nice work! You've really put together a flexible, extensible, and well-tested Android SDK with many lessons we should take with us to Datadog. (On that note, please write up lessons from all this in a confluence page). I like the clean code and good test coverage including concurrency tests.
In addition to my own review, I added co-pilot and also had Claude take a pass. I'm approving as-is, although there are some things that I and the tools found that I'd love for you to engage on:
- EppoClientPollingTest should check for actual poll calls (or lack of, when paused) using spys or some other mechanism
- Copilot-identified leaky newSingleThreadExecutor
- Claude-identified saveConfiguration disk/memory race condition
- Name of AndroidBaseClient
- Consider a default no-op instance
- Claude-identified possibility of
buildAndInit()hanging forever - Copilot-identified need of atomic singleton and/or thread safe access of instance in AndroidBaseClient (volatile/synchronized)
- gson shouldn't be a dependency anymore
- slf4j-android maybe should be runtime only
| * | ||
| * @return Initialized EppoClient | ||
| */ | ||
| private AndroidBaseClient<JsonNode> buildOfflineClientWithoutPolling() |
There was a problem hiding this comment.
pretty much same as previous method but with a different boolean passed for pollingEnabled() and the call to pollingIntervalMs() Could be consolidated to a single method (with 0 pollingMs meaning no polling) or the explicitly named methods with a shared base method.
| } | ||
|
|
||
| @Test | ||
| public void testPauseAndResumePolling() throws ExecutionException, InterruptedException { |
There was a problem hiding this comment.
This test doesn't seem to be doing much. Consider a shorter polling interval and then pausing longer than the interval to make sure no polls happens and then check polling happened again after unpausing.
| } | ||
|
|
||
| @Test | ||
| public void testPauseResumeSequenceDoesNotCrash() |
| } | ||
|
|
||
| @Test | ||
| public void testMultiplePauseResumeCycles() throws ExecutionException, InterruptedException { |
| ConfigCacheFile cacheFile = | ||
| new ConfigCacheFile(application, cacheFileSuffix, codec.getContentType()); | ||
| return new FileBackedByteStore(cacheFile); |
There was a problem hiding this comment.
Pretty small--any reason not to inline it int he constructor?
| (success, ex) -> { | ||
| if (ex == null && Boolean.TRUE.equals(success)) { | ||
| ret.complete(newInstance); | ||
| } else if (offlineMode || failCount.incrementAndGet() == 2) { |
There was a problem hiding this comment.
Had Claude give this a look and it is worried that buildAndInitAsync() can hang forever.
buildAndInitAsync() can hang forever. If initialConfigFuture is non-null and
resolves with success = false/null, the else branch increments
failCount twice (to 2, then 3). If loadConfigurationAsync then also fails,
incrementAndGet() returns 4 — the == 2 check never matches, and ret is never
completed. The caller blocks forever on .get().
Also on that note, consider providing an optional timeout (with a default to something generous like 30 seconds?)
There was a problem hiding this comment.
It also has a possible other recommendation regarding use of anyOf()
ret.complete() can be called from two different callbacks on success. Both
loadConfigurationAsync and initialConfigFuture handlers call
ret.complete(newInstance) on success. CompletableFuture.complete() is
idempotent so this isn't a bug, but it suggests the dual-callback design is
more complex than it needs to be. A clearer approach would be
CompletableFuture.anyOf() for success with an explicit failure combiner.
| delete(); | ||
| try (BufferedWriter writer = getWriter()) { | ||
| writer.write(contents); |
There was a problem hiding this comment.
Technically, a crash between these two will result in the loss of file. So you could do write and then rename. But if this is is only used for tests not worth worrying about this edge case.
| private long pollingIntervalMs; | ||
| private long pollingJitterMs; | ||
|
|
||
| @Nullable private static AndroidBaseClient<?> instance; |
There was a problem hiding this comment.
By not having a default no-op instance, buildAndInit() could return null if graceful mode is at play and an error happens before instance is set.
There was a problem hiding this comment.
Also, this is to be extended by EppoClient which has its own singleton so I don't think we want this one here nor does it make sense to have multiple singleton instances throughout the inheritance tree.
| api 'cloud.eppo:eppo-sdk-framework:0.1.0-SNAPSHOT' | ||
|
|
||
| api 'com.google.code.gson:gson:2.10.1' | ||
| api 'org.slf4j:slf4j-android:1.7.36' |
There was a problem hiding this comment.
Don't see this in the code, should this be runtimeOnly?
| .thenRun( | ||
| () -> { | ||
| this.configuration = config; | ||
| }); |
There was a problem hiding this comment.
Had Claude dig into this, and it thinks there a race condition.
The write goes to the single-thread IO_EXECUTOR, but thenRun executes on
whatever thread completes the future — typically the IO thread, but not
guaranteed. Here's the race:
Thread 1: saveConfiguration(A)
1. codec.toBytes(A)
2. byteStore.write(A) → queued on IO_EXECUTOR
Thread 2: saveConfiguration(B)
3. codec.toBytes(B)
4. byteStore.write(B) → queued on IO_EXECUTOR
IO_EXECUTOR runs them in order:
5. writes A to disk
6. writes B to disk ← disk now has B
thenRun callbacks fire:
7. this.configuration = B ← from step 4's callback
8. this.configuration = A ← from step 2's callback (ran slightly later)
Result: disk=B, memory=A
Step 8 can run after step 7 because thenRun doesn't run on the IO_EXECUTOR —
it runs on whatever thread calls complete(), or the calling thread if the
future is already done. The two thenRun callbacks are not ordered by the
single-thread executor.
The simplest fix is to run the in-memory update inside the IO_EXECUTOR too:
return byteStore
.write(bytes)
.thenRunAsync(() -> { this.configuration = config; }, IO_EXECUTOR);
This guarantees the disk write and memory update for A both happen before B's,
since they're all serialized on the same single thread. Though that would
require exposing the executor or moving the logic into FileBackedByteStore.
|
I asked Claude to check past PRs for past issues and make sure we're not reintroducing them, it came up: Details: |
- Fix failCount double-increment hang: remove redundant increment from the else-branch in buildAndInitAsync; only the else-if path increments - Fix concurrent-save revert race in CachingConfigurationStore: replace volatile field with AtomicReference + compareAndSet so a concurrent successful save is never overwritten by a failing save's revert - Preserve HTTP exception as cause in EppoInitializationException when both the network fetch and initial config load fail; previously the cause was silently null - Add @after teardown to EppoClientPollingTest to stop polling timers between tests; replace verify(never()) with verify(atMost(1)) to tolerate one in-flight invocation after cancel(false) - Fix testConcurrentWrites assertion to assertArrayEquals(data2) since IO_EXECUTOR serializes writes in submission order - Add daemon thread factory to IO_EXECUTOR so JVM/Robolectric can exit - Add volatile to static singleton instance field - Split InterruptedException catch to restore interrupt flag - Various comments, null guards, and minor scope/naming fixes
… docs - Add ObjectInputFilter allowlist to ConfigurationCodec.Default.fromBytes() to prevent gadget-chain attacks; restricts deserialization to cloud.eppo.**, java.util collections (with dollar-named inner classes listed first per first-match-wins semantics), and java.lang types - Remove dead CompletionException catch branch from AndroidBaseClient.buildAndInit(); CompletableFuture.get() only throws ExecutionException, never CompletionException - Remove unused import java.util.concurrent.CompletionException - Add @throws RuntimeException to buildAndInit() Javadoc - Document intentional early singleton assignment in buildAndInitAsync() for graceful mode; getInstance() is callable immediately after the future is returned - Improve saveConfiguration() Javadoc to document exceptional future completion - Move sign publishing.publications inside GPG credential guard to prevent Gradle model configuration errors when credentials are absent - Fix POM developer email: replace URL with valid email address sdk@geteppo.com - Add PublishToMavenLocal checkVersion dependency guard
java.io.ObjectInputFilter is a Java 9+ API absent from Android's android.jar at any API level, causing a compile error. The deserialization risk is low: the cache file lives in the app's private internal storage and the bytes are produced by the SDK's own ObjectOutputStream write path. The isInstance() type check remains to prevent returning the wrong type to callers; updated comment clarifies it does not prevent gadget-chain execution (which occurs inside readObject() before the check runs).
…efile publish target - CLAUDE.md: documents module layering, Android API constraints (ObjectInputFilter and other Java 9+ APIs absent from android.jar), key abstractions, build/test commands, and Sonatype Central Portal publish workflow - .gitignore: uncomment *.jks/*.keystore, add *.pem, *.p12, .env, credentials* - Makefile: update credential check from OSSRH_* to mavenCentralUsername/ mavenCentralPassword to match vanniktech Central Portal setup; use ./gradlew :eppo:publish -Prelease (checkVersion already wired as dependency)
- Rename AndroidBaseClient → BaseAndroidClient to match BaseEppoClient naming pattern - Fix async cache load race: seed in-memory cache from disk after loadFromStorage() so getConfiguration() returns the cached value while the network fetch is in-flight; previously it returned emptyConfig() until saveConfiguration() was called by the HTTP fetch - Fix seedCache() to use a static EMPTY_SENTINEL so compareAndSet uses reference equality against the same instance stored in the AtomicReference at construction time - Add .exceptionally() handler to storage load chain so IO failures are logged rather than propagated as an opaque null cause in offline mode - Fix slf4j-android to runtimeOnly scope; add slf4j-api as implementation dep - Add checkNoSnapshotApiDeps Gradle task to prevent SNAPSHOT api-scoped dependencies from leaking to consumers; only enforced on -Prelease builds
BaseAndroidClient (cloud.eppo.android.framework) and CachingConfigurationStore (cloud.eppo.android.framework.storage) are in different packages. Package-private visibility blocked cross-package access at compile time.
Add CachingConfigurationStore.loadAndSeedFromStorage() which combines loadFromStorage() + seedCache() in one call. Revert seedCache() from public back to package-private; BaseAndroidClient.Builder no longer needs to reach across package boundaries to seed the in-memory cache.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 20 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| dependencies { | ||
| api 'cloud.eppo:eppo-sdk-framework:0.1.0-SNAPSHOT' | ||
|
|
||
| implementation 'org.slf4j:slf4j-android:1.7.36' | ||
| testImplementation 'com.google.code.gson:gson:2.10.1' | ||
| compileOnly 'org.jetbrains:annotations:24.0.0' | ||
|
|
||
| testImplementation 'cloud.eppo:sdk-common-jvm:4.0.0-SNAPSHOT' |
| "key": "medum-size", | ||
| "rules": [ | ||
| { | ||
| "conditions": [ | ||
| { |
| public CompletableFuture<AndroidBaseClient<JsonFlagType>> buildAndInitAsync() { | ||
| // Singleton handling | ||
| if (instance != null && !forceReinitialize) { | ||
| Log.w(TAG, "Eppo Client instance already initialized"); | ||
| @SuppressWarnings("unchecked") | ||
| AndroidBaseClient<JsonFlagType> typedInstance = (AndroidBaseClient<JsonFlagType>) instance; | ||
| return CompletableFuture.completedFuture(typedInstance); | ||
| } else if (instance != null) { | ||
| // Stop polling if reinitializing | ||
| instance.stopPolling(); | ||
| Log.i(TAG, "forceReinitialize triggered - reinitializing Eppo Client"); | ||
| } |
| protected BaseCacheFile(Application application, String fileName) { | ||
| File filesDir = application.getFilesDir(); | ||
| cacheFile = new File(filesDir, fileName); | ||
| } |
| // Remove non-alphanumeric characters so it plays nice with filesystem paths. | ||
| // Note: if the first 8 characters are all non-alphanumeric the result is an empty string, |
| @Before | ||
| public void setUp() { | ||
| application = RuntimeEnvironment.getApplication(); | ||
| codec = new ConfigurationCodec.Default<>(Configuration.class); | ||
| cacheFileSuffix = "test-" + System.currentTimeMillis(); | ||
| } |
- Add null/empty validation for Builder required params - Synchronize singleton check-through-assign in buildAndInitAsync() - Fix safeCacheKey comment to document \W keeping underscores - Remove unused gson testImplementation dependency - Add @after cleanup to FileBackedConfigStoreTest (fix wrong .bin extension)
Singleton management (instance field, getInstance(), synchronized init guard) removed from BaseAndroidClient. Concrete subclasses (EppoClient in :eppo) declare their own singleton. Framework-only consumers hold the reference from buildAndInitAsync() directly. This prevents the architectural conflict where both BaseAndroidClient and EppoClient would have independent static instance fields, causing confusion about which getInstance() to call.
loadAndSeedFromStorage() ran its continuation on IO_EXECUTOR (inherited from byteStore.read()). ConfigurationRequestor.setInitialConfiguration() chained .thenApply on the same thread, which called saveConfiguration() → byteStore.write() → supplyAsync(IO_EXECUTOR). Since IO_EXECUTOR is a single-thread executor, the write was queued behind the .thenApply that was waiting for it — classic deadlock. Fix: use thenApplyAsync (common pool) in loadAndSeedFromStorage() to break out of IO_EXECUTOR before downstream continuations run.
typotter
force-pushed
the
typo/v4/add-framework-module
branch
from
0144c26 to
8d16eeb
Compare
Validate application and fileName params match the guard-clause pattern used in other storage classes (CachingConfigurationStore, FileBackedByteStore).
When the initial config future resolves (cache miss or bad cache data) before the HTTP fetch completes, the else branch was calling failCount.incrementAndGet() a second time. Combined with the increment already performed in the else-if condition check, failCount reached 2 before HTTP failure could trigger it. The subsequent HTTP failure then incremented to 3, skipping the == 2 check and leaving ret never completed — a hang. Fix: drop the redundant increment from the else block. The HTTP failure handler is solely responsible for completing ret exceptionally when both paths fail. Reproduces as a timeout in testCachedBadResponseRequiresFetch on API 34 where file I/O resolves faster than the HTTP round-trip.
- Change `ex == null && success` to `ex == null && Boolean.TRUE.equals(success)` to guard against NPE if the future completes normally with a null Boolean value; matches the pattern already used in BaseAndroidClient - Improve comment on the else branch to clarify that failCount is already incremented to 1 by the else-if condition evaluation (side effect of `failCount.incrementAndGet() == 2` being false), so no additional increment is needed in the else block
…k-framework (#255) - volatile on pollingIntervalMs/pollingJitterMs in BaseAndroidClient (builder thread writes, polling thread reads — no synchronization guard existed) - volatile on EppoClient.instance singleton field - FileBackedByteStore: IO_EXECUTOR demoted from static final to instance-level ExecutorService; implements Closeable with shutdown() so tests can reclaim the IO thread - CachingConfigurationStore: remove optimistic update pattern in saveConfiguration(); disk write now happens first and in-memory cache is only updated on success, eliminating the window where getConfiguration() could return an unpersisted value
|
🤖 Comment from AI: |
| * | ||
| * @param <JsonFlagType> The JSON type used for JSON flag values (e.g., JsonNode, JsonElement) | ||
| */ | ||
| public class AndroidBaseClient<JsonFlagType> extends BaseEppoClient<JsonFlagType> { |
There was a problem hiding this comment.
Still stand by a rename, such as AndroidEppoClient
| if (project.hasProperty('release') && project.version.endsWith('SNAPSHOT')) { | ||
| throw new GradleException("You cannot specify -Prelease with a SNAPSHOT version") | ||
| } | ||
| if (project.hasProperty('snapshot') && !project.version.endsWith('SNAPSHOT')) { |
There was a problem hiding this comment.
🤖 Comment from AI: This is the line that makes the snapshot publish path impossible for the new module. Since version is fixed at 0.1.0, ./gradlew :android-sdk-framework:publish -Psnapshot always throws here (You cannot specify -Psnapshot with a non-SNAPSHOT version). Release publish is blocked too while the api dependency is still 0.1.0-SNAPSHOT. Could we either make the module version 0.1.0-SNAPSHOT until upstream is released, or derive the version from the snapshot/release flag so the snapshot flow can actually publish?
| * to release the background IO thread. If not closed explicitly, the daemon thread will be | ||
| * reclaimed by the JVM/Android runtime on process exit. | ||
| */ | ||
| public final class FileBackedByteStore implements ByteStore, java.io.Closeable { |
There was a problem hiding this comment.
🤖 Comment from Claude: close() was added so the single-thread ioExecutor can be reclaimed, but nothing in the production path ever calls it — neither CachingConfigurationStore nor FileBackedConfigStore is Closeable, and BaseAndroidClient holds the store without closing it. For a long-lived singleton store this is fine (the thread is a daemon). But Builder.forceReinitialize(true) constructs a brand-new FileBackedConfigStore → new FileBackedByteStore → new executor thread each time, with no path to shut down the previous one. Repeated reinitialization leaks one daemon thread per cycle. Consider plumbing close() through CachingConfigurationStore/FileBackedConfigStore and closing the prior store on reinitialize.
| // Note: if the first 8 characters are all non-word the result is an empty string, | ||
| // which produces the filename "eppo-sdk-flags-.bin". Eppo-issued API keys always start | ||
| // with alphanumeric characters, so this edge case is not expected in production. | ||
| return key.substring(0, Math.min(8, key.length())).replaceAll("\\W", ""); |
There was a problem hiding this comment.
🤖 Comment from Claude: Updated — lower priority after checking the backend. I traced SDK-key generation in the Eppo monorepo (api-key.service.ts). Keys are {randomBytes}.{encodedPayload}: the high-entropy random portion (≥8 chars, MIN_RANDOM_BYTES = 8) comes before the period, and the encoded domain/routing info comes after. The backend itself keys off token.substr(0, 8) as a uniqueness prefix, so the first 8 chars are effectively unique per key — collision risk for real Eppo keys is negligible.
Two small leftover notes, take them or leave them:
\\Wstrips base64url-(but keeps_), so a-among those first 8 chars slightly shortens the effective key. Negligible in practice.- If a non-Eppo / custom key (no random prefix) is ever passed, the 8-char-prefix assumption breaks. A one-line comment noting the dependency on Eppo's key format would be enough.
There was a problem hiding this comment.
This comment can be safely ignored, no practical implications
| * @return a future that completes with the loaded configuration, or null if storage is empty or | ||
| * missing | ||
| */ | ||
| @NotNull public CompletableFuture<Configuration> loadAndSeedFromStorage() { |
There was a problem hiding this comment.
🤖 Comment from Claude: Minor efficiency observation: loadAndSeedFromStorage() both seeds the in-memory cache and returns the loaded config, which Builder.buildAndInitAsync() then passes as initialConfiguration. Per the deadlock-fix commit (8d16eeb), the framework's ConfigurationRequestor.setInitialConfiguration() chains into saveConfiguration() → byteStore.write(). So on every cold start with a valid cache, the SDK reads the cached bytes and then immediately writes the identical bytes back to disk. Functionally harmless, but it's an avoidable disk write on each launch. Seeding the in-memory cache (which you already do) should be sufficient to make getConfiguration() return immediately without re-persisting.
| rootProject.name = "Eppo SDK" | ||
| include ':example' | ||
| include ':eppo' | ||
| include ':android-sdk-framework' No newline at end of file |
There was a problem hiding this comment.
🤖 Comment from Claude: Nit: no trailing newline at end of file. The module's spotless misc block enforces endWithNewline() on *.gradle, but it's scoped to the android-sdk-framework dir so this root file isn't caught by CI — still worth adding the newline for consistency.
…lback buildAndInit()'s defensive ExecutionException catch was returning null in graceful mode. In practice this path is unreachable (buildAndInitAsync()'s .exceptionally() handler already returns the instance), but as a safety net track the constructed instance on the Builder and return it here instead.
eppo-sdk-framework and sdk-common-jvm are still SNAPSHOT-only on Maven Central (sdk-common-jdk#238 just merged but release not yet published). Set android-sdk-framework version to 0.1.0-SNAPSHOT so -Psnapshot publish works. Update CLAUDE.md version table to match.
- Rename class and file to BaseAndroidEppoClient - Update EppoClientPollingTest references - Update CLAUDE.md module table
…tor on reinitialize - Make FileBackedConfigStore implement java.io.Closeable using a private two-arg constructor to capture the FileBackedByteStore reference before passing it to super() - Add ownedConfigStore field to Builder; close it when forceReinitialize is true so the background IO executor is not leaked across reinits
- Fix safeCacheKey comment: clarify \W preserves underscores and note the 8-char prefix assumption depends on Eppo's key format - Replace System.currentTimeMillis() suffix with UUID in FileBackedConfigStoreTest to avoid timestamp collision on fast machines - Fix "medum-size" typo to "medium-size" in flags-v1.json fixture
Merge testMultiplePauseResumeCycles and testPauseResumeSequenceDoesNotCrash into a single test that verifies polling actually stops and resumes via mock verification. Upgrade testPauseAfterInit to verify behavior instead of just checking no-crash. Remove unused Log/TAG artifacts.
|
@coderabbitai review |
✅ Action performedReview finished.
|
📝 WalkthroughWalkthroughIntroduces a new Changesandroid-sdk-framework module, EppoClient fix, and project config
Sequence Diagram(s)sequenceDiagram
participant App
participant Builder as BaseAndroidEppoClient.Builder
participant FileBackedConfigStore
participant BaseAndroidEppoClient
participant EppoConfigurationClient
App->>Builder: buildAndInitAsync()
Builder->>FileBackedConfigStore: create (if none provided)
Builder->>FileBackedConfigStore: loadAndSeedFromStorage()
FileBackedConfigStore-->>Builder: cached config (or null)
Builder->>BaseAndroidEppoClient: new instance
alt not offlineMode
Builder->>EppoConfigurationClient: fetchConfiguration()
EppoConfigurationClient-->>Builder: config or exception
Builder->>FileBackedConfigStore: saveConfiguration(config)
end
alt pollingEnabled
Builder->>BaseAndroidEppoClient: startPolling(interval, jitter)
end
Builder-->>App: CompletableFuture~BaseAndroidEppoClient~
App->>BaseAndroidEppoClient: pausePolling()
BaseAndroidEppoClient->>BaseAndroidEppoClient: stopPolling()
App->>BaseAndroidEppoClient: resumePolling()
BaseAndroidEppoClient->>BaseAndroidEppoClient: startPolling(storedInterval, storedJitter)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
CLAUDE.md (2)
57-60: 💤 Low valueOptional: Add language specifier to fenced code block.
Markdownlint recommends specifying a language for fenced code blocks. Consider adding
propertiesafter the opening backticks.📝 Proposed fix
-``` +```properties mavenCentralUsername=<your-central-portal-token-username> mavenCentralPassword=<your-central-portal-token-password>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CLAUDE.md` around lines 57 - 60, The fenced code block containing the mavenCentralUsername and mavenCentralPassword configuration is missing a language specifier, which violates markdown linting standards. Add the language identifier "properties" immediately after the opening triple backticks (```) to properly specify the code block type. This improves readability and follows markdown best practices for documentation.Source: Linters/SAST tools
7-12: 💤 Low valueOptional: Add language specifier to fenced code block.
Markdownlint recommends specifying a language for fenced code blocks. Consider adding
textafter the opening backticks for the ASCII dependency diagram.📝 Proposed fix
-``` +```text eppo-sdk-framework (external, platform-neutral flag evaluation) └── sdk-common-jvm (external, OkHttp + Jackson + JVM defaults)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CLAUDE.md` around lines 7 - 12, The ASCII dependency diagram fenced code block is missing a language specifier which violates Markdownlint recommendations. Add the language specifier `text` after the opening backticks for the code block containing the eppo-sdk-framework dependency tree diagram by changing the opening ``` to ```text to properly identify the code block as plain text content.Source: Linters/SAST tools
android-sdk-framework/src/test/java/cloud/eppo/android/framework/storage/FileBackedByteStoreTest.java (1)
26-38: CallbyteStore.close()in teardown to release the executor thread.
FileBackedByteStoreimplementsjava.io.Closeableand owns a dedicatedExecutorServicefor background IO operations. Theclose()method must be called in@After(line 37) to properly shut down this executor. Although the thread is a daemon and won't block process exit, explicit cleanup ensures proper resource management and prevents executor threads from accumulating in test environments.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@android-sdk-framework/src/test/java/cloud/eppo/android/framework/storage/FileBackedByteStoreTest.java` around lines 26 - 38, In the tearDown method marked with `@After`, the byteStore object is not being properly closed to release its dedicated ExecutorService. Add a call to byteStore.close() in the tearDown method to ensure the executor thread is properly shut down. This should be called before or after the cacheFile.delete() call to guarantee proper resource cleanup in the test teardown.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@android-sdk-framework/src/main/java/cloud/eppo/android/framework/storage/BaseCacheFile.java`:
- Around line 20-29: In the BaseCacheFile constructor, validate the fileName
parameter to prevent directory escape attacks before constructing the cache file
path. Check that fileName does not contain path traversal sequences such as
"..", absolute paths (starting with "/"), or other directory separator
characters that could allow the resulting cacheFile path to escape the intended
filesDir location. Add validation logic after the null checks but before
creating the File object with new File(filesDir, fileName) to ensure only safe,
relative filenames are accepted.
---
Nitpick comments:
In
`@android-sdk-framework/src/test/java/cloud/eppo/android/framework/storage/FileBackedByteStoreTest.java`:
- Around line 26-38: In the tearDown method marked with `@After`, the byteStore
object is not being properly closed to release its dedicated ExecutorService.
Add a call to byteStore.close() in the tearDown method to ensure the executor
thread is properly shut down. This should be called before or after the
cacheFile.delete() call to guarantee proper resource cleanup in the test
teardown.
In `@CLAUDE.md`:
- Around line 57-60: The fenced code block containing the mavenCentralUsername
and mavenCentralPassword configuration is missing a language specifier, which
violates markdown linting standards. Add the language identifier "properties"
immediately after the opening triple backticks (```) to properly specify the
code block type. This improves readability and follows markdown best practices
for documentation.
- Around line 7-12: The ASCII dependency diagram fenced code block is missing a
language specifier which violates Markdownlint recommendations. Add the language
specifier `text` after the opening backticks for the code block containing the
eppo-sdk-framework dependency tree diagram by changing the opening ``` to
```text to properly identify the code block as plain text content.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d048402a-ddaa-4bc2-aa0c-f53028075816
📒 Files selected for processing (24)
.gitignoreCLAUDE.mdMakefileandroid-sdk-framework/build.gradleandroid-sdk-framework/src/androidTest/java/cloud/eppo/android/framework/EppoClientPollingTest.javaandroid-sdk-framework/src/main/AndroidManifest.xmlandroid-sdk-framework/src/main/java/cloud/eppo/android/framework/BaseAndroidEppoClient.javaandroid-sdk-framework/src/main/java/cloud/eppo/android/framework/exceptions/EppoInitializationException.javaandroid-sdk-framework/src/main/java/cloud/eppo/android/framework/exceptions/NotInitializedException.javaandroid-sdk-framework/src/main/java/cloud/eppo/android/framework/storage/BaseCacheFile.javaandroid-sdk-framework/src/main/java/cloud/eppo/android/framework/storage/ByteStore.javaandroid-sdk-framework/src/main/java/cloud/eppo/android/framework/storage/CachingConfigurationStore.javaandroid-sdk-framework/src/main/java/cloud/eppo/android/framework/storage/ConfigCacheFile.javaandroid-sdk-framework/src/main/java/cloud/eppo/android/framework/storage/ConfigurationCodec.javaandroid-sdk-framework/src/main/java/cloud/eppo/android/framework/storage/FileBackedByteStore.javaandroid-sdk-framework/src/main/java/cloud/eppo/android/framework/storage/FileBackedConfigStore.javaandroid-sdk-framework/src/main/java/cloud/eppo/android/framework/util/Utils.javaandroid-sdk-framework/src/test/java/cloud/eppo/android/framework/storage/CachingConfigurationStoreTest.javaandroid-sdk-framework/src/test/java/cloud/eppo/android/framework/storage/ConfigurationCodecTest.javaandroid-sdk-framework/src/test/java/cloud/eppo/android/framework/storage/FileBackedByteStoreTest.javaandroid-sdk-framework/src/test/java/cloud/eppo/android/framework/storage/FileBackedConfigStoreTest.javaandroid-sdk-framework/src/test/resources/flags-v1.jsoneppo/src/main/java/cloud/eppo/android/EppoClient.javasettings.gradle
| protected BaseCacheFile(Application application, String fileName) { | ||
| if (application == null) { | ||
| throw new IllegalArgumentException("application must not be null"); | ||
| } | ||
| if (fileName == null) { | ||
| throw new IllegalArgumentException("fileName must not be null"); | ||
| } | ||
| File filesDir = application.getFilesDir(); | ||
| cacheFile = new File(filesDir, fileName); | ||
| } |
There was a problem hiding this comment.
Harden cache path construction against directory escape.
Line 27-Line 28 accepts raw fileName and directly resolves it under filesDir. Absolute names or traversal-like input can point outside the intended cache location.
Suggested fix
protected BaseCacheFile(Application application, String fileName) {
@@
- File filesDir = application.getFilesDir();
- cacheFile = new File(filesDir, fileName);
+ File filesDir = application.getFilesDir();
+ if (filesDir == null) {
+ throw new IllegalStateException("application filesDir is unavailable");
+ }
+ try {
+ File canonicalDir = filesDir.getCanonicalFile();
+ File candidate = new File(canonicalDir, fileName).getCanonicalFile();
+ String dirPath = canonicalDir.getPath() + File.separator;
+ if (!candidate.getPath().startsWith(dirPath)) {
+ throw new IllegalArgumentException("fileName resolves outside app files directory");
+ }
+ cacheFile = candidate;
+ } catch (IOException e) {
+ throw new IllegalArgumentException("invalid cache file path", e);
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| protected BaseCacheFile(Application application, String fileName) { | |
| if (application == null) { | |
| throw new IllegalArgumentException("application must not be null"); | |
| } | |
| if (fileName == null) { | |
| throw new IllegalArgumentException("fileName must not be null"); | |
| } | |
| File filesDir = application.getFilesDir(); | |
| cacheFile = new File(filesDir, fileName); | |
| } | |
| protected BaseCacheFile(Application application, String fileName) { | |
| if (application == null) { | |
| throw new IllegalArgumentException("application must not be null"); | |
| } | |
| if (fileName == null) { | |
| throw new IllegalArgumentException("fileName must not be null"); | |
| } | |
| File filesDir = application.getFilesDir(); | |
| if (filesDir == null) { | |
| throw new IllegalStateException("application filesDir is unavailable"); | |
| } | |
| try { | |
| File canonicalDir = filesDir.getCanonicalFile(); | |
| File candidate = new File(canonicalDir, fileName).getCanonicalFile(); | |
| String dirPath = canonicalDir.getPath() + File.separator; | |
| if (!candidate.getPath().startsWith(dirPath)) { | |
| throw new IllegalArgumentException("fileName resolves outside app files directory"); | |
| } | |
| cacheFile = candidate; | |
| } catch (IOException e) { | |
| throw new IllegalArgumentException("invalid cache file path", e); | |
| } | |
| } |
🧰 Tools
🪛 ast-grep (0.43.0)
[warning] 27-27: Prevent path traversal
Context: new File(filesDir, fileName)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal'). Security best practice.
(path-traversal-java)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@android-sdk-framework/src/main/java/cloud/eppo/android/framework/storage/BaseCacheFile.java`
around lines 20 - 29, In the BaseCacheFile constructor, validate the fileName
parameter to prevent directory escape attacks before constructing the cache file
path. Check that fileName does not contain path traversal sequences such as
"..", absolute paths (starting with "/"), or other directory separator
characters that could allow the resulting cacheFile path to escape the intended
filesDir location. Add validation logic after the null checks but before
creating the File object with new File(filesDir, fileName) to ensure only safe,
relative filenames are accepted.
Source: Linters/SAST tools
Summary
Add new android-sdk-framework module that provides base client functionality and configuration storage abstractions for Android SDK implementations.
Key Components
Dependencies
This module depends on:
Testing
Notes
Summary by CodeRabbit
New Features
Documentation
Tests