Skip to content

fix: Reject override snapshots and files that contain a deleted flag or segment - #259

Open
kinyoklion wants to merge 1 commit into
rlamb/overrides-java-contract-testsfrom
rlamb/overrides-java-reject-deleted
Open

kinyoklion wants to merge 1 commit into
rlamb/overrides-java-contract-testsfrom
rlamb/overrides-java-reject-deleted

Conversation

@kinyoklion

@kinyoklion kinyoklion commented Nov 10, 2026 •

Copy link
Copy Markdown
Member

Requirements

  • I have added test coverage for new or changed functionality
  • I have followed the repository's pull request submission guidelines
  • I have validated my changes against all supported platform versions

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": true became 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:

  • Sink (every source, including a custom one through 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 example Rejected an override snapshot because flag "f" is marked as deleted; the overrides in effect are unchanged. A deleted item is either the placeholder that DataKind.deserialize produces or a model object marked as deleted. The check runs in OverrideLayer.setAll before the contents are swapped, and OverrideSinkImpl logs the rejection. The OverrideSink signature is unchanged and nothing is thrown to the source.
  • File override source. OverrideFileLoader fails 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": false is an ordinary definition and still loads.
  • Public docs. 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.setOverrides gains 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. FileDataSourceBase and FileDataSourceParsing are 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 (OverrideFileLoader and 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: replacementThatContainsADeletedItemIsRejectedAndTheLayerIsUnchanged replaces deletedPlaceholderIsStoredAsIs, which pinned the old behavior. It covers the deserialized placeholder and a model object marked as deleted, for both flags and segments. definitionWithDeletedFalseIsStored is new.
  • OverrideSinkImplTest: snapshotWithADeletedFlagIsRejected, snapshotWithADeletedSegmentIsRejected and snapshotWithADeletedItemIsRejectedWhenNothingIsListening. Each checks that the layer is unchanged, that no notification is sent, and that the Error is logged.
  • OverrideFileLoaderTest: flagMarkedAsDeletedFailsTheLoad and segmentMarkedAsDeletedFailsTheLoad cover JSON and YAML, with the exact message naming the file and the key. Also entryMarkedAsDeletedFailsTheLoadEvenWhenItIsADroppedDuplicate and entriesWithDeletedFalseAreOrdinaryDefinitions.
  • FileOverrideSourceTest: flagMarkedAsDeletedFailsTheReloadAndKeepsTheOverrides and segmentMarkedAsDeletedFailsTheReloadAndKeepsTheOverrides. 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. entriesWithDeletedFalseLoad is also new.
  • DataLoaderTest.entriesMarkedAsDeletedStillLoadAsDeletedItems: the ordinary file data source behavior is pinned as unchanged.

How I ran them, on JDK 17 (arm64):

  1. Published the unreleased shared libraries locally from the monorepo. Common came from the feat: Add the overrideAffected indicator to EvaluationReason #218 head (b5deba0) with ./gradlew publishToMavenLocal -Pversion=2.6.0 -PskipSigning=true in lib/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.0 in lib/shared/internal.
  2. In 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.
  3. Ran the full server test task (same Gradle options). Result: 2275 tests in 129 classes, 0 failures. The 72 skipped tests are the existing skips in PersistentDataStoreWrapperTest.
  4. Ran checkstyleMain checkstyleTest javadoc: passed, with no new Javadoc warnings.
  5. Checked that the new tests fail without the change. With the layer check and the loader check disabled, all 9 rejection tests fail, and the deleted: false tests and the data source pin still pass. With the sink's catch removed, the 3 sink tests fail.

Describe alternatives you've considered

  • Checking only in the sink, with a separate pass over the snapshot. That pass would iterate the source's Iterable twice. Checking in OverrideLayer.setAll keeps it to one pass and makes the layer enforce its own invariant.
  • Dropping only the deleted entry. The spec requires rejecting the whole snapshot, so the layer is always exactly a snapshot that the source supplied.

Additional context

CI stays red until the shared libraries are released: the stack pins launchdarkly-java-sdk-common 2.6.0 and launchdarkly-java-sdk-internal 1.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.setAll validates every entry before swapping contents and throws DeletedItemException when any item is deleted. OverrideSinkImpl catches that, logs an error, leaves current overrides unchanged, and skips flag-change notifications—sources are not thrown through OverrideSink.

OverrideFileLoader treats deleted entries as a load failure (like a parse error), including when duplicate-key handling would drop the entry. FileOverrides / OverrideSink docs 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: false remains valid.

Reviewed by Cursor Bugbot for commit b3e3725. Bugbot is set up for automated code reviews on this repo. Configure here.

…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.
@kinyoklion

Copy link
Copy Markdown
Member Author

bugbot review

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant