Add strict mode for DataLoaderRegistry - #252
Conversation
Sorry, something went wrong.
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
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
Sorry, something went wrong.
| 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<?, ?>>)
Sorry, something went wrong.
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.
Sorry, something went wrong.
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
left a comment
There was a problem hiding this comment.
Thanks
Sorry, something went wrong.
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.