Repository navigation
SBS review fixes: replace stored settings, keep the consent requirement, device ID trigger, store file - #307
Merged
Conversation
The store file was truncated and then written in place on every save, and the SDK saves it at every init and every settings response. A process stopped mid-write left a file that made every later Countly.init throw a JSONException into the application. Saves now write a temporary file next to the store and move it over the store, atomically where the file system allows. A store that is not valid JSON is logged and replaced by empty storage on the next save instead of failing init, and a temporary file left by an interrupted save is deleted when the storage opens, so it is not taken for other stored SDK data.
Each response was merged into the stored settings, so a setting removed in SDK Manager kept its last server value for good, across restarts too: a removed kill switch, filter, journey trigger, limit or rqs stayed in force. Each valid response now replaces the stored settings, and a setting it leaves out goes back to the developer configuration or the SDK default, as the reference implementation does since its settings merge fix. Values the validation drops resolve the same way. A cr false from the server, from storage or from the provided settings no longer lifts the consent requirement the developer set: the resolved requirement is the developer's value or a cr true, so the settings can add the requirement but never remove it. While a device ID change without merge ends the previous device ID's timed events and view, a journey trigger or a full event queue reached on that thread no longer sends the queue under the new device ID; those events go out together under the previous one, and refresh no content zone. Tests whose later responses relied on the merge now carry the settings they need in each response.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #275. Your uncommitted P7 diff (log gathering, connection test tests, changelog) applies cleanly on top of this branch (
git apply --check).What
tracking:false,eb,jte,lklorrqsstayed in force across restarts. Now each valid answer replaces the storedc, and a missing key resolves to the developer value or SDK default.removeListingFilterKeysFromConfigis gone.crnever lifts the developer's requirementrequiresConsentOR acrtrue. A server, stored or providedcr:falseliftedsetRequiresConsent(true): everything tracked without consent andonConsentRemovalwas ignored.deviceIdChangedends timed events after the new ID is set, so ajtetrigger (or a full queue) force-sent the previous user's events under the new ID. Those events now go out together under the previous ID, with no content refresh for the new user.JsonFileStorage.save()truncated and rewrote in place; a process stopped mid-write left a file that made every laterCountly.initthrowJSONExceptioninto the app. Saves now go through a temp file and an atomic move, an invalid store starts empty, and a leftover temp file is deleted at open.Verification
crlifted, removed setting kept, events undernew_device_id,JSONExceptionout of init).:sdk-javasuite, JDK 8, isolateduser.home: 675/675, twice.{}: withsetRequiresConsent(true)and providedcr:false, feat: SBS #275 sent events from two runs without consent and this branch sent none. Atracking:falsethe server dropped in its next answer kept tracking off on feat: SBS #275, not here. The journey trigger events went out under the new device ID on feat: SBS #275 and under the previous one here. A cut-short store file madeCountly.initthrow on feat: SBS #275, not here.Left for you
updateConfigVariablesjavadoc still says "merged settings": your P7 diff edits the adjacent line, so changing it here would conflict.networking(false), so networking turns back on mid-test. They pass because sends to the dummy URL fail and keep the files; only the journey trigger test asserted pending triggers, which is fixed here. GatesTests also overlaps your P7 edits, so it is untouched.cr:truefrom the server still makes the feature facades returnnullon the next start (D2), so host code likeevents().recordEvent(...)throws an NPE. That needs a design decision (non-null no-op facades vs documenting it).ModuleDeviceIdCore. It would also sendviews().startViewend events under the old ID. Larger blast radius, so not here.🤖 Generated with Claude Code