Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

851: Refactor data loaders and disable caching #853

Merged
merged 3 commits into from
Mar 14, 2023

Conversation

michael-markl
Copy link
Member

@michael-markl michael-markl commented Mar 8, 2023

Closes #851. Refactoring of the data loaders:

  • Disable caching on all data loaders
  • Make registering and naming of data loaders less boilerplaty and less error-prone
  • Avoid name conflicts of data loaders
  • Make them more type safe

Also, I rewrote the data loader for physicalStoreByStoreIdLoader which makes the searchAcceptingStores GraphQL query a lot faster: Instead of ~20 single SQL queries (one for each store) we now make a single SQL query.

fun physicalStore(dataFetchingEnvironment: DataFetchingEnvironment): CompletableFuture<PhysicalStore?> =
dataFetchingEnvironment.getDataLoader<Int, PhysicalStore?>(PHYSICAL_STORE_BY_STORE_ID_LOADER_NAME).load(id)
fun physicalStore(environment: DataFetchingEnvironment): CompletableFuture<PhysicalStore?> =
physicalStoreByStoreIdLoader.fromEnvironment(environment).load(id)
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems you forgot sth. here.

Suggested change
physicalStoreByStoreIdLoader.fromEnvironment(environment).load(id)
physicalStoreByStoreIdLoader.fromEnvironment(environment).load(id).thenApply { it!! }

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually not: We designed our API to support AcceptingStores that do not have a PhyiscalStore, (so this is a 0..1 connection). Hence, the physicalStore function here should return a nullable value.

Copy link
Member Author

@michael-markl michael-markl Mar 14, 2023

Choose a reason for hiding this comment

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

In contrast to that, the above occurences of the data loader should always return a non-nullable field, as we require a category and a contact object on each accepting store.

(So previously the typing of the data loader was false in the above functions contact and category. That was also why I refactored it so that we don't have to specify the generics of the data loader here, but instead infer it directly from the dataloader we use)

@michael-markl michael-markl merged commit f2be46e into main Mar 14, 2023
@michael-markl michael-markl deleted the 851-disable-caching-on-dataloaders branch March 14, 2023 10:04
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.

Disable Caching on DataLoaders
2 participants