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

Can't use Spring Security annotation when injecting DataLoader and returning a CompletableFuture #343

Closed
koenpunt opened this issue Mar 29, 2022 · 9 comments

Comments

@koenpunt
Copy link
Contributor

koenpunt commented Mar 29, 2022

I'm having trouble combining DataLoaders with Spring Security;

My scenario;

typealias MerchantDataLoader = DataLoader<UUID, Merchant?>

// MerchantDataLoader
registry.forTypePair(UUID::class.java, Merchant::class.java).registerMappedBatchLoader { merchantIds, _ ->
    Mono.just(merchantService.getMerchants(merchantIds).associateBy { it.id })
}

// ...

@SchemaMapping
@PreAuthorize("@permissionEvaluator.hasPermission(authentication, #merchant.id, 'Merchant', 'read')")
fun dashboardForDate(
    merchant: Merchant,
    @Argument date: LocalDate?,
    merchantLoader: MerchantDataLoader
): CompletableFuture<Dashboard> = merchantLoader.load(merchant.id).thenApply {
    if (it == null) throw EntityNotFoundException()
    Dashboard(date, it)
}

For GraphQL Java being able to dispatch the dataloader the method has to return the CompletableFuture, but for the @PreAuthorize annotation it "must return an instance of org.reactivestreams.Publisher (i.e. Mono / Flux) or the function must be a Kotlin coroutine function".

I've also tried making it a suspend fun, while still returning the CompletableFuture, but then I get an error like does not match the type of the source Object 'class java.util.concurrent.CompletableFuture', which makes sense because my controller is not expecting the CompletableFuture.

Is my assumption correct, or is there a way around this that I don't know about?

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Mar 29, 2022
@koenpunt
Copy link
Contributor Author

Interestingly enough converting to a mono seems to work;

@SchemaMapping
@PreAuthorize("@permissionEvaluator.hasPermission(authentication, #merchant.id, 'Merchant', 'read')")
fun dashboardForDate(
    merchant: Merchant,
    @Argument date: LocalDate?,
    merchantLoader: MerchantDataLoader
): Mono<Dashboard> = merchantLoader.load(merchant.id).thenApply {
    if (it == null) throw EntityNotFoundException()
    Dashboard(date, it)
}.toMono()

Although I'm not sure if this is coincidental, or that this is supposed to work.

@rstoyanchev
Copy link
Contributor

The method needs to return a reactive type for reactive context to propagate. So this is expected, but you're bringing up a good point that our examples with injecting a DataLoader and using it as is should at least be improved. Maybe we could even consider injecting an alternative ReactiveDataLoader type that returns Mono instead of CompletableFuture.

@rstoyanchev rstoyanchev added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Apr 19, 2022
@rstoyanchev rstoyanchev added this to the 1.0 Backlog milestone Apr 19, 2022
@rstoyanchev rstoyanchev changed the title Can't use DataLoader in method with Spring Security annotation Can't use Spring Security annotation when injecting DataLoader and returning a ComletableFuture Apr 19, 2022
@koenpunt
Copy link
Contributor Author

koenpunt commented Apr 19, 2022

Maybe we could even consider injecting an alternative ReactiveDataLoader type that returns Mono instead of CompletableFuture.

So returning a mono is supposed to work for dataloaders? In comparison; awaiting the CompletableFuture in a suspend fun causes the application to lock-up.

And a ReactiveDataLoader sounds like a good idea!

@koenpunt koenpunt changed the title Can't use Spring Security annotation when injecting DataLoader and returning a ComletableFuture Can't use Spring Security annotation when injecting DataLoader and returning a CompletableFuture Apr 21, 2022
@rstoyanchev
Copy link
Contributor

So returning a mono is supposed to work for dataloaders?

Yes that should work. Mono and Flux are supported as return values for any DataFetcher with context propagation, through ContextDataFetcherDecorator.

@koenpunt
Copy link
Contributor Author

Alright, good to know it's spring graphql specific.
I suppose this could be extended to support coroutines too?

@rstoyanchev
Copy link
Contributor

Probably so, but need to investigate.

@bclozel bclozel modified the milestones: 1.2 Backlog, 1.x Backlog Jan 12, 2023
@rstoyanchev rstoyanchev modified the milestones: 1.x Backlog, 1.2 Backlog May 15, 2023
@rstoyanchev rstoyanchev modified the milestones: 1.2 Backlog, 1.3 Backlog Oct 13, 2023
@rstoyanchev
Copy link
Contributor

@koenpunt since coroutines are now supported and #653 was processed, is anything left to do here?

@koenpunt
Copy link
Contributor Author

@rstoyanchev I'm not sure about whether the combination with Spring Security annotations now works, since we stopped using that because it didn't work well together with Spring R2DBC (overfetching data because it doesn't have a cache like JPA does).

So also the need for it disappeared for us.

@rstoyanchev
Copy link
Contributor

Closing for now.

@rstoyanchev rstoyanchev closed this as not planned Won't fix, can't repro, duplicate, stale May 23, 2024
@rstoyanchev rstoyanchev removed this from the 1.3 Backlog milestone May 23, 2024
@rstoyanchev rstoyanchev removed the type: enhancement A general enhancement label May 23, 2024
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

No branches or pull requests

4 participants