feat: Add maxKeyValueCacheBytes and maxKeyValueCacheFiles to configuration to manage cache size#1227
Conversation
|
I will reformat the title to use the proper commit message syntax. |
parse-github-assistant
bot
changed the title
|
🚀 Thanks for opening this pull request! We appreciate your effort in improving the project. Please let us know once your pull request is ready for review. Note Please respond to review comments from AI agents just like you would to comments from a human reviewer. Let the reviewer resolve their own comments, unless they have reviewed and accepted your commit, or agreed with your explanation for why the feedback was incorrect. Caution Pull requests must be written using an AI agent with human supervision. Pull requests written entirely by a human will likely be rejected, because of lower code quality, higher review effort and the higher risk of introducing bugs. Please note that AI review comments on this pull request alone do not satisfy this requirement. |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds configurable key-value cache limits to Parse.Configuration and Builder, wires those values into ParseKeyValueCache during initialization, and adds/updates tests and test scaffolding; also includes Gradle wrapper script and properties updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer (Builder)
participant Config as Parse.Configuration
participant Init as Parse.initialize
participant Cache as ParseKeyValueCache
Dev->>Config: build() with maxKeyValueCacheBytes/Files
Config->>Init: provide configuration
Init->>Cache: set maxKeyValueCacheBytes/Files (when localDatastore disabled)
Init->>Cache: initialize()
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@parse/src/main/java/com/parse/Parse.java`:
- Around line 725-740: Both Builder.maxKeyValueCacheBytes and
Builder.maxKeyValueCacheFiles must reject negative inputs; add validation in
these setter methods (maxKeyValueCacheBytes(int) and maxKeyValueCacheFiles(int))
that throws an IllegalArgumentException (or similar) when the argument is < 0
with a clear message, and only assign the field and return this when the value
is valid.
In `@parse/src/main/java/com/parse/ParseKeyValueCache.java`:
- Around line 25-28: The constants DEFAULT_MAX_KEY_VALUE_CACHE_BYTES and
DEFAULT_MAX_KEY_VALUE_CACHE_FILES are declared public but are unreachable
because their enclosing class ParseKeyValueCache is package-private; fix by
changing the ParseKeyValueCache class declaration to public so external SDK
consumers (and Parse.Configuration.Builder Javadoc references) can access
DEFAULT_MAX_KEY_VALUE_CACHE_BYTES and DEFAULT_MAX_KEY_VALUE_CACHE_FILES,
ensuring no other API or visibility expectations are violated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 09ce1077-fd24-4cf6-aee8-e239010d2197
📒 Files selected for processing (3)
parse/src/main/java/com/parse/Parse.javaparse/src/main/java/com/parse/ParseKeyValueCache.javaparse/src/test/java/com/parse/ParseKeyValueCacheTest.java
There was a problem hiding this comment.
🧹 Nitpick comments (2)
parse/src/main/java/com/parse/Parse.java (1)
729-749: Clarify applicability of these settings in Javadoc.Consider noting in the method docs that these limits are applied on the non-local-datastore initialization path, to avoid ambiguity for integrators.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@parse/src/main/java/com/parse/Parse.java` around lines 729 - 749, Update the Javadoc for Builder.maxKeyValueCacheBytes and Builder.maxKeyValueCacheFiles to explicitly state these limits are only applied when the Parse SDK initializes the disk cache via the non-local-datastore initialization path (i.e., when the SDK manages the on-disk key/value cache), and do not affect disk caching when using a local datastore or custom cache implementation; mention the default constants (Builder#DEFAULT_MAX_KEY_VALUE_CACHE_BYTES and Builder#DEFAULT_MAX_KEY_VALUE_CACHE_FILES) and that these settings take effect during SDK initialization so integrators know when they will be honored.parse/src/test/java/com/parse/ParseKeyValueCacheTest.java (1)
126-149: Prefer non-nullContextin builder tests.Consider using a mocked
Contextinstead ofnullso tests align with the@NonNullAPI contract and remain resilient if constructor null-checking is tightened later.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@parse/src/test/java/com/parse/ParseKeyValueCacheTest.java` around lines 126 - 149, Replace the null Context used in the Parse.Configuration.Builder calls in the tests with a non-null mocked Context to honor the `@NonNull` API contract; update the tests testConfigurationBuilderSetsCustomCacheValues, testConfigurationBuilderRejectsNegativeCacheBytes, and testConfigurationBuilderRejectsNegativeCacheFiles to instantiate a mock or dummy android.content.Context and pass it to new Parse.Configuration.Builder(...) so the builder calls (e.g., .applicationId(...), .maxKeyValueCacheBytes(...), .maxKeyValueCacheFiles(...), .build()) operate with a real Context object rather than null.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@parse/src/main/java/com/parse/Parse.java`:
- Around line 729-749: Update the Javadoc for Builder.maxKeyValueCacheBytes and
Builder.maxKeyValueCacheFiles to explicitly state these limits are only applied
when the Parse SDK initializes the disk cache via the non-local-datastore
initialization path (i.e., when the SDK manages the on-disk key/value cache),
and do not affect disk caching when using a local datastore or custom cache
implementation; mention the default constants
(Builder#DEFAULT_MAX_KEY_VALUE_CACHE_BYTES and
Builder#DEFAULT_MAX_KEY_VALUE_CACHE_FILES) and that these settings take effect
during SDK initialization so integrators know when they will be honored.
In `@parse/src/test/java/com/parse/ParseKeyValueCacheTest.java`:
- Around line 126-149: Replace the null Context used in the
Parse.Configuration.Builder calls in the tests with a non-null mocked Context to
honor the `@NonNull` API contract; update the tests
testConfigurationBuilderSetsCustomCacheValues,
testConfigurationBuilderRejectsNegativeCacheBytes, and
testConfigurationBuilderRejectsNegativeCacheFiles to instantiate a mock or dummy
android.content.Context and pass it to new Parse.Configuration.Builder(...) so
the builder calls (e.g., .applicationId(...), .maxKeyValueCacheBytes(...),
.maxKeyValueCacheFiles(...), .build()) operate with a real Context object rather
than null.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ca2f12bb-e9e3-489b-96a1-0ac520843b01
📒 Files selected for processing (2)
parse/src/main/java/com/parse/Parse.javaparse/src/test/java/com/parse/ParseKeyValueCacheTest.java
|
The CI failures are pre-existing and unrelated to this PR. They occur on the master branch as well (ParseCorePluginsTest, ParseCountingUriHttpBodyTest, ParseDecoderTest, ParseFileControllerTest, ParseFileTest, ParseUriHttpBodyTest). All 5 new tests added in this PR pass successfully. |
|
Without a functioning CI we cannot make any merges realistically speaking. We'd be flying with eyes closed. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
parse/src/test/java/com/parse/ParseKeyValueCacheTest.java (2)
141-155: Consider adding edge-case tests for zero values.The tests verify that negative values are rejected, which is good. However, the behavior for zero values (
maxKeyValueCacheBytes(0)andmaxKeyValueCacheFiles(0)) is not tested. Depending on the intended design:
- If zero is invalid (e.g., could cause issues with cache logic), add tests asserting rejection.
- If zero is valid (e.g., effectively disables caching), add tests confirming acceptance.
This clarifies the API contract for edge cases.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@parse/src/test/java/com/parse/ParseKeyValueCacheTest.java` around lines 141 - 155, Add explicit tests for zero values to clarify the API contract: in ParseKeyValueCacheTest create two new `@Test` methods that invoke new Parse.Configuration.Builder(null).applicationId("test").maxKeyValueCacheBytes(0).build() and .maxKeyValueCacheFiles(0).build() and assert the expected behavior (either expect IllegalArgumentException if zero should be rejected, or no exception and successful build if zero should be accepted); reference the existing Parse.Configuration.Builder and the methods maxKeyValueCacheBytes and maxKeyValueCacheFiles to place the new tests alongside the negative-value tests so zero-case behavior is covered.
125-139: Consider adding an integration test forParse.initialize().This test verifies that
Parse.Configurationstores the custom values, but according to the PR description,Parse.initialize()should apply these values toParseKeyValueCache. An integration test verifying that the configured values propagate toParseKeyValueCache.maxKeyValueCacheBytesandParseKeyValueCache.maxKeyValueCacheFilesduring initialization would strengthen coverage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@parse/src/test/java/com/parse/ParseKeyValueCacheTest.java` around lines 125 - 139, Add an integration test that calls Parse.initialize(...) with a Parse.Configuration built via Parse.Configuration.Builder setting maxKeyValueCacheBytes and maxKeyValueCacheFiles, then verify those configured values are applied to the ParseKeyValueCache (e.g., check ParseKeyValueCache.maxKeyValueCacheBytes and ParseKeyValueCache.maxKeyValueCacheFiles or the cache instance's corresponding getters) after initialization; ensure the test uses unique values, resets/cleans Parse state before/after (so Parse.initialize can run deterministically), and references Parse.initialize, Parse.Configuration.Builder, and ParseKeyValueCache to locate the relevant code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@parse/src/test/java/com/parse/ParseKeyValueCacheTest.java`:
- Around line 141-155: Add explicit tests for zero values to clarify the API
contract: in ParseKeyValueCacheTest create two new `@Test` methods that invoke new
Parse.Configuration.Builder(null).applicationId("test").maxKeyValueCacheBytes(0).build()
and .maxKeyValueCacheFiles(0).build() and assert the expected behavior (either
expect IllegalArgumentException if zero should be rejected, or no exception and
successful build if zero should be accepted); reference the existing
Parse.Configuration.Builder and the methods maxKeyValueCacheBytes and
maxKeyValueCacheFiles to place the new tests alongside the negative-value tests
so zero-case behavior is covered.
- Around line 125-139: Add an integration test that calls Parse.initialize(...)
with a Parse.Configuration built via Parse.Configuration.Builder setting
maxKeyValueCacheBytes and maxKeyValueCacheFiles, then verify those configured
values are applied to the ParseKeyValueCache (e.g., check
ParseKeyValueCache.maxKeyValueCacheBytes and
ParseKeyValueCache.maxKeyValueCacheFiles or the cache instance's corresponding
getters) after initialization; ensure the test uses unique values, resets/cleans
Parse state before/after (so Parse.initialize can run deterministically), and
references Parse.initialize, Parse.Configuration.Builder, and ParseKeyValueCache
to locate the relevant code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9bb75d03-ae0a-4d87-a651-85acdbadac0b
📒 Files selected for processing (2)
parse/src/main/java/com/parse/Parse.javaparse/src/test/java/com/parse/ParseKeyValueCacheTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- parse/src/main/java/com/parse/Parse.java
|
I suggest to fix the CI in a separate PR first, and not fix it into this PR. |
|
I'll give it a try with a separate PR. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
parse/src/test/java/com/parse/ResetPluginsParseTest.java (1)
19-24: Ordering is correct and follows internal pattern.The addition of
Parse.setLocalDatastore(null)beforeParseCorePlugins.getInstance().reset()matches the pattern used inParse.disableLocalDatastore()(see context snippet 3), ensuring theisLocalDatastoreEnabledflag is false before controllers are cleared.Consider adding the same call to
setUp()for defensive completeness—if a previous test fails or is interrupted before itstearDown()completes, the subsequent test'ssetUp()would still start with stale datastore state.♻️ Optional: Add defensive cleanup to setUp()
`@Before` public void setUp() throws Exception { + Parse.setLocalDatastore(null); ParseCorePlugins.getInstance().reset(); ParsePlugins.reset(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@parse/src/test/java/com/parse/ResetPluginsParseTest.java` around lines 19 - 24, Add defensive cleanup to setUp(): call Parse.setLocalDatastore(null) at the start of the setUp() method so tests always begin with local datastore disabled; this mirrors the tearDown() ordering that calls Parse.setLocalDatastore(null) before ParseCorePlugins.getInstance().reset() and ParsePlugins.reset(), and prevents stale datastore state if a prior test failed to complete its tearDown().parse/src/test/java/com/parse/ParseFileControllerTest.java (1)
231-249: Drop the temp-file assertion that is no longer on the URI path.After Line 233 switches the source to
mock(Uri.class), the file created on Lines 231-232 is no longer used bysaveAsync(...).assertTrue(file.exists())at Line 248 now passes regardless of the behavior under test, so either restore a realUri.fromFile(file)or remove the temp file and assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@parse/src/test/java/com/parse/ParseCountingUriHttpBodyTest.java`:
- Around line 66-69: The test registers a single reusable InputStream via
Shadows.shadowOf(...).registerInputStream(uri, ...) but
ParseCountingUriHttpBody.writeTo() opens the URI twice (once for content length,
once to copy), so the second open gets EOF; replace the registerInputStream(...)
call with registerInputStreamSupplier(...) and provide a supplier that returns a
fresh new ByteArrayInputStream(getData().getBytes()) for the given uri so each
openInputStream() in ParseCountingUriHttpBody.writeTo() receives a new stream;
update the test where ShadowContentResolver and uri are used to call
registerInputStreamSupplier instead.
---
Nitpick comments:
In `@parse/src/test/java/com/parse/ResetPluginsParseTest.java`:
- Around line 19-24: Add defensive cleanup to setUp(): call
Parse.setLocalDatastore(null) at the start of the setUp() method so tests always
begin with local datastore disabled; this mirrors the tearDown() ordering that
calls Parse.setLocalDatastore(null) before
ParseCorePlugins.getInstance().reset() and ParsePlugins.reset(), and prevents
stale datastore state if a prior test failed to complete its tearDown().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 32c264f7-dc4d-4011-987d-0fc17ef446a3
⛔ Files ignored due to path filters (1)
gradle/wrapper/gradle-wrapper.jaris excluded by!**/*.jar
📒 Files selected for processing (8)
gradle/wrapper/gradle-wrapper.propertiesgradlewgradlew.batparse/src/test/java/com/parse/ParseCountingUriHttpBodyTest.javaparse/src/test/java/com/parse/ParseFileControllerTest.javaparse/src/test/java/com/parse/ParseFileTest.javaparse/src/test/java/com/parse/ParseUriHttpBodyTest.javaparse/src/test/java/com/parse/ResetPluginsParseTest.java
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1227 +/- ##
=============================================
+ Coverage 0.00% 67.46% +67.46%
- Complexity 0 2304 +2304
=============================================
Files 124 124
Lines 10076 10090 +14
Branches 1359 1361 +2
=============================================
+ Hits 0 6807 +6807
+ Misses 10076 2748 -7328
- Partials 0 535 +535 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…iguration.Builder
evtimmy
force-pushed
the
feature/configurable-cache-size
branch
from
05c6991 to
206922c
Compare
|
@mtrezza rebased the PR and now CI is passing. Do we need anything else? Otherwise ready to merge when you get the chance, thanks! |
mtrezza
changed the title
maxKeyValueCacheBytes and maxKeyValueCacheFiles to Configuration
mtrezza
changed the title
maxKeyValueCacheBytes and maxKeyValueCacheFiles to ConfigurationmaxKeyValueCacheBytes and maxKeyValueCacheFiles to configuration to manage cache size
|
@evtimmy Nice work! |
# [4.4.0](4.3.0...4.4.0) (2026-03-11) ### Features * Add `maxKeyValueCacheBytes` and `maxKeyValueCacheFiles` to configuration to manage cache size ([#1227](#1227)) ([c19f02f](c19f02f))
|
🎉 This change has been released in version 4.4.0 |
4 participants
New Pull Request Checklist
Issue Description
Closes: #1098
Approach
Exposed
DEFAULT_MAX_KEY_VALUE_CACHE_BYTESandDEFAULT_MAX_KEY_VALUE_CACHE_FILESaspublic constants in
ParseKeyValueCache, and addedmaxKeyValueCacheBytes()andmaxKeyValueCacheFiles()builder methods toParse.Configuration.Builder.The configured values are applied in
Parse.initialize()before the cache is initialized.If the cache size is reduced in an app update, existing cache files are trimmed lazily on
the next write via the existing LRU eviction logic.
TODOs before merging
Summary by CodeRabbit
New Features
Behavior
Tests