FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Fix value cache batch loader environment by andimarek · Pull Request #278 · graphql-java/java-dataloader · GitHub

Fix value cache batch loader environment - #278

Draft
andimarek wants to merge 4 commits into
masterfrom
fix-pr-277-value-cache-environment
Draft

Fix value cache batch loader environment#278
andimarek wants to merge 4 commits into
masterfrom
fix-pr-277-value-cache-environment

Conversation

andimarek commented May 20, 2026
edited
Loading

Copy link
Copy Markdown
Member

This is a demonstration patch stacked on top of #277. It addresses the BatchLoaderEnvironment semantic regression described in the requested-changes review.

Problem

#277 builds a BatchLoaderEnvironment before value-cache filtering. If a dispatch has a partial ValueCache hit, the actual batch loader is invoked only with the missed keys, but the environment can still describe the original full key/context list.

That breaks the existing invariant that environment.getKeyContextsList() is aligned with the keys argument received by a BatchLoaderWithContext.

Fix

When ValueCache filtering produces missedKeys, build a fresh BatchLoaderEnvironment from missedKeys and missedKeyContexts before invoking the concrete batch loader.

The actual fix is intentionally small:

  • src/main/java/org/dataloader/DataLoaderHelper.java:483 builds missedEnvironment from missedKeys and missedKeyContexts.
  • src/main/java/org/dataloader/DataLoaderHelper.java:484 passes missedEnvironment into the concrete invokeLoader(...) call for the cache misses.

GitHub line link:
https://github.com/graphql-java/java-dataloader/blob/fix-pr-277-value-cache-environment/src/main/java/org/dataloader/DataLoaderHelper.java#L483-L484

Test

Adds a regression test where:

  • A is returned from ValueCache
  • B misses and is sent to the batch loader
  • the batch loader asserts it receives only B
  • environment.getKeyContextsList().get(0) matches B's context, not A's

The key test lines are:

  • src/test/java/org/dataloader/DataLoaderBatchLoaderEnvironmentTest.java:221-249

GitHub line link:
https://github.com/graphql-java/java-dataloader/blob/fix-pr-277-value-cache-environment/src/test/java/org/dataloader/DataLoaderBatchLoaderEnvironmentTest.java#L221-L249

Verification

  • ./gradlew test --tests org.dataloader.DataLoaderBatchLoaderEnvironmentTest
  • ./gradlew test
  • git diff --check origin/pr/277

Copy link
Copy Markdown

Hello, this pull request has been inactive for 60 days, so we're marking it as stale. If you would like to continue working on this pull request, please make an update within the next 30 days, or we'll close the pull request.

github-actions Bot added the Stale label Jul 20, 2026

Copy link
Copy Markdown

Hello, as this pull request has been inactive for 90 days, we're closing this pull request. We always welcome contributions, and if you would like to continue, please open a new pull request.

github-actions Bot closed this Aug 20, 2026
andimarek reopened this Aug 20, 2026
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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants


Back | FazBrowse Home | New Git URL