GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Add strict mode for DataLoaderRegistry by AlexandreCarlton · Pull Request #252 · graphql-java/java-dataloader

Conversation

AlexandreCarlton commented Nov 22, 2025

Copy link

Copy Markdown

Collaborator

In a similar vein to graphql-java/graphql-java#3565 which enabled "strict mode" for type wiring (preventing multiple DataFetchers being registered to the same field on a GraphQL type), we add "strict mode" to the DataLoaderRegistry so that we don't accidentally register multiple DataLoaders to the same key (which would result in the last registration winning). This can prevent confusing bugs from emerging.

This defaults to false to avoid breaking changes, and deliberately mimics the referenced PR to maintain consistency.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          Add strict mode for DataLoaderRegistry

af3ebf4

In a similar vein to graphql-java/graphql-java#3565
which enabled "strict mode" for type wiring (preventing multiple
`DataFetcher`s being registered to the same field on a GraphQL type), we
add "strict mode" to the `DataLoaderRegistry` so that we don't
accidentally register multiple DataLoaders to the same key (which would
result in the last registration winning).

This defaults to `false` to avoid breaking changes.

bbakerman requested changes

Nov 24, 2025

View reviewed changes

bbakerman left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Move the strict check in nameAndInstrumentDL as a common place and all the registering methods will then be covered

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

src/main/java/org/dataloader/DataLoaderRegistry.java

		if (strictMode) {
		assertKeyStrictly(name);
		}
		dataLoaders.put(name, nameAndInstrumentDL(name, instrumentation, dataLoader));

bbakerman Nov 24, 2025

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

You need to cover org.dataloader.DataLoaderRegistry#computeIfAbsent as well

I think this code would be better put in the nameAndInstrumentDL method as a common place

The following is all the places it is called

instrumentDLs(Map<String, DataLoader<?, ?>>, DataLoaderInstrumentation)
register(DataLoader<?, ?>)
register(String, DataLoader<?, ?>)
registerAndGet(String, DataLoader<?, ?>)
computeIfAbsent(String, Function<String, DataLoader<?, ?>>)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

AlexandreCarlton Nov 24, 2025

Copy link

Copy Markdown

Collaborator Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Done - as agreed elsewhere, I've omitted computeIfAbsent since this repeated invocations to the same key will not override the DataLoader with strict mode off.

I did have to make nameAndInstrumentDL to get access to strictMode and the DataLoader map, however.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          Collect assertKeyStrictly into one location

1628a27

This is done to ensure coverage of all registering locations.

We purposefully omit `computeIfAbsent` as this does _not_ register a new
DataLoader if there is an existing key.

bbakerman approved these changes

Nov 25, 2025

View reviewed changes

bbakerman left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Thanks

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bbakerman added this to the 7.0.0 milestone

Nov 25, 2025

Reviewers

bbakerman

bbakerman approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

7.0.0

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

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.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.