Repository navigation
fix: Reject override snapshots and files that contain a deleted flag or segment - #259
Open
kinyoklion wants to merge 1 commit into
Open
kinyoklion wants to merge 1 commit into
kinyoklion wants to merge 1 commit into
Conversation
…or segment The override layer never holds a deleted item (sdk-specs#288, Requirement 1.1.1.1). A snapshot from any source that contains one is rejected as a whole: the layer is unchanged, no flag change listener is notified, and the rejection is logged at Error level with the kind and the key (1.2.8.1). The file override source fails the load of a file that has a flag or segment entry marked as deleted, as for a file that cannot be parsed: the overrides in effect stay, the failure names the file and the key, and the retry applies (1.3.6.2). An entry with "deleted": false is an ordinary definition. The file data source keeps accepting deleted entries.
Member
Author
|
bugbot review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit b3e3725. Configure here.
This branch has not been deployed
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.
Requirements
Related issues
Implements the "reject deleted entries" rule of the flag overrides spec, launchdarkly/sdk-specs#288 (Requirements 1.1.1.1, 1.2.8.1 and 1.3.6.2).
This PR is stacked on #225 (
rlamb/overrides-java-contract-tests) so that the PRs below it do not change. Retarget it when the stack merges.Describe the solution you've provided
Before: a flag or segment entry with
"deleted": truebecame a deleted placeholder. That was true both in an override file and in a snapshot from a custom source. The override layer stored it and the overlay served it, so the key looked absent: the LaunchDarkly definition was hidden from evaluation, from prerequisites and segment reads, and from the all-flags state, and the evaluation was not marked as override-affected.After:
OverrideSink). The override layer never holds a deleted item. If a snapshot contains one, the SDK rejects the whole snapshot. The layer stays unchanged, no flag change listener is notified, and the SDK logs an Error naming the kind and the key, for exampleRejected an override snapshot because flag "f" is marked as deleted; the overrides in effect are unchanged. A deleted item is either the placeholder thatDataKind.deserializeproduces or a model object marked as deleted. The check runs inOverrideLayer.setAllbefore the contents are swapped, andOverrideSinkImpllogs the rejection. TheOverrideSinksignature is unchanged and nothing is thrown to the source.OverrideFileLoaderfails the load when a file has a flag or segment entry marked as deleted, the same way it fails for a file that cannot be parsed. The overrides in effect stay, and the existing retry applies. The failure is logged like other load failures and names the file and the key:Unable to load flags: file /path/overrides.json: flag "f" is marked as deleted; remove the entry to remove its override. The check also fires when the duplicate keys handling would drop the entry, because the file still needs correcting. The source detects this while loading, before it calls the sink."deleted": falseis an ordinary definition and still loads.FileOverrides.source()gains one sentence saying that a file with an entry marked as deleted fails to load and that an override is removed by removing its entry or deleting the file.OverrideSink.setOverridesgains one sentence saying that a snapshot with a deleted flag or segment is rejected. The experimental notice is unchanged on both.The ordinary file data source is unchanged on purpose.
FileDataSourceBaseandFileDataSourceParsingare not modified. Rejecting deleted entries there would be a breaking change, which is planned for a later major version. The check lives only in the override path (OverrideFileLoaderand the override layer). A new test,DataLoaderTest.entriesMarkedAsDeletedStillLoadAsDeletedItems, pins the data source's current behavior: it still loads such entries as deleted items.Tests
OverrideLayerTest:replacementThatContainsADeletedItemIsRejectedAndTheLayerIsUnchangedreplacesdeletedPlaceholderIsStoredAsIs, which pinned the old behavior. It covers the deserialized placeholder and a model object marked as deleted, for both flags and segments.definitionWithDeletedFalseIsStoredis new.OverrideSinkImplTest:snapshotWithADeletedFlagIsRejected,snapshotWithADeletedSegmentIsRejectedandsnapshotWithADeletedItemIsRejectedWhenNothingIsListening. Each checks that the layer is unchanged, that no notification is sent, and that the Error is logged.OverrideFileLoaderTest:flagMarkedAsDeletedFailsTheLoadandsegmentMarkedAsDeletedFailsTheLoadcover JSON and YAML, with the exact message naming the file and the key. AlsoentryMarkedAsDeletedFailsTheLoadEvenWhenItIsADroppedDuplicateandentriesWithDeletedFalseAreOrdinaryDefinitions.FileOverrideSourceTest:flagMarkedAsDeletedFailsTheReloadAndKeepsTheOverridesandsegmentMarkedAsDeletedFailsTheReloadAndKeepsTheOverrides. In each, an in-place edit adds the deleted entry and changes another override. Nothing is supplied, the logged Error names the file and the key, and removing the entry recovers.entriesWithDeletedFalseLoadis also new.DataLoaderTest.entriesMarkedAsDeletedStillLoadAsDeletedItems: the ordinary file data source behavior is pinned as unchanged.How I ran them, on JDK 17 (arm64):
b5deba0) with./gradlew publishToMavenLocal -Pversion=2.6.0 -PskipSigning=trueinlib/shared/common. Internal came from the feat: Count override-affected evaluations separately and send no individual events for them #219 head (0e7744f) with-Pversion=1.13.0inlib/shared/internal.lib/sdk/server, ran the override and file data source classes with./gradlew --max-workers=2 --no-daemon -Dorg.gradle.jvmargs=-Xmx2g test --tests '*Override*' --tests '*.integrations.File*' --tests '*.integrations.DataLoaderTest' --tests '*.integrations.FlagFileParser*' --tests '*.integrations.ClientWithFileDataSourceTest'. Result: 343 tests in 29 classes, 0 failures.testtask (same Gradle options). Result: 2275 tests in 129 classes, 0 failures. The 72 skipped tests are the existing skips inPersistentDataStoreWrapperTest.checkstyleMain checkstyleTest javadoc: passed, with no new Javadoc warnings.deleted: falsetests and the data source pin still pass. With the sink's catch removed, the 3 sink tests fail.Describe alternatives you've considered
Iterabletwice. Checking inOverrideLayer.setAllkeeps it to one pass and makes the layer enforce its own invariant.Additional context
CI stays red until the shared libraries are released: the stack pins
launchdarkly-java-sdk-common2.6.0 andlaunchdarkly-java-sdk-internal1.13.0, which are not published yet.Opened from termag
Note
Overview
Implements the overrides spec rule that deleted flags or segments must not enter the override layer. A snapshot or file that includes
"deleted": true(or a deleted placeholder) is rejected in full instead of hiding LaunchDarkly data for that key.OverrideLayer.setAllvalidates every entry before swapping contents and throwsDeletedItemExceptionwhen any item is deleted.OverrideSinkImplcatches that, logs an error, leaves current overrides unchanged, and skips flag-change notifications—sources are not thrown throughOverrideSink.OverrideFileLoadertreats deleted entries as a load failure (like a parse error), including when duplicate-key handling would drop the entry.FileOverrides/OverrideSinkdocs now say overrides are removed by deleting the entry or file, not via"deleted": true. The ordinary file data source is unchanged (still loads deleted items).Tests cover layer, sink, loader, and file override reload behavior;
deleted: falseremains valid.Reviewed by Cursor Bugbot for commit b3e3725. Bugbot is set up for automated code reviews on this repo. Configure here.