Add strict mode for DataLoaderRegistry#252
Uh oh!
There was an error while loading. Please reload this page.
Conversation
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 left a comment
bbakerman left a comment There was a problem hiding this comment.
Move the strict check in nameAndInstrumentDL as a common place and all the registering methods will then be covered
| if (strictMode){ | ||
| assertKeyStrictly(name); | ||
| } | ||
| dataLoaders.put(name, nameAndInstrumentDL(name, instrumentation, dataLoader)); |
There was a problem hiding this comment.
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<?, ?>>) There was a problem hiding this comment.
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.
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.
2 participants
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 theDataLoaderRegistryso 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
falseto avoid breaking changes, and deliberately mimics the referenced PR to maintain consistency.