spring-projects / spring-graphql

Spring Integration for GraphQL
https://spring.io/projects/spring-graphql
Apache License 2.0
1.53k stars 303 forks source link

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

Closed koenpunt closed 5 months ago

koenpunt commented 2 years ago

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?

koenpunt commented 2 years ago

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 commented 2 years ago

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.

koenpunt commented 2 years ago

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!

rstoyanchev commented 2 years ago

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 commented 2 years ago

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

rstoyanchev commented 2 years ago

Probably so, but need to investigate.

rstoyanchev commented 6 months ago

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

koenpunt commented 6 months ago

@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 commented 5 months ago

Closing for now.