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

Spring @Transactional support #448

Open
koenpunt opened this issue Jul 21, 2022 · 15 comments
Open

Spring @Transactional support #448

koenpunt opened this issue Jul 21, 2022 · 15 comments
Labels
in: data Issues related to working with data status: pending-design-work type: enhancement A general enhancement
Milestone

Comments

@koenpunt
Copy link
Contributor

koenpunt commented Jul 21, 2022

When we first started with spring we used JPA and made heavy use of its relational support. This caused some issues with spring-graphql because sometimes lazy associations were accessed in a field resolver, causing an error that said something like "there isn't an active transaction".
We worked around this by writing a custom instrumentation to start a transaction for every query and mutation.

Now that's we removed all associations (in preparation of migrating to R2DBC), we no longer need this transaction by default. But removing this instrumentation somehow causes a significant increase in queries.

Adding @Transactional to graphql controllers doesn't seem to make a difference. Also adding @Transactional to a parent controller, and @Transactional(propagation = Propagation.MANDATORY) to controller of an underlying type controller, causes the execution to fail with:

org.springframework.transaction.IllegalTransactionStateException: No existing transaction found for transaction marked with propagation 'mandatory'

This seems odd to me, because I would expect a transaction to exist there because of the transaction started in the parent controller. After some discussion with a teammate I've realized this isn't odd, because both controllers are invoked separately by the datafetchers, and thus not sharing the transaction. It still would be very welcome to have a solution for this.

Is there any support for transactions in Spring Graphql? Or is using a custom instrumentation (or ExecutionStrategy) the only way out for this?

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jul 21, 2022
@nort3x
Copy link

nort3x commented Jul 21, 2022

I'm not 100% sure but i think jpa.open-in-view=true can solve this,
I know it's not recommend and I'm waiting for an answer on this as well

@mp911de
Copy link
Member

mp911de commented Aug 5, 2022

Spring GraphQL uses a DataFetcher-oriented execution approach in which nested fields are retrieved by subsequent calls to nested DataFetchers once the parent has been materialized.

Once a DataFetcher call has been completed, the GraphQL engine introspects the returned object and uses a collection of DataFetchers to resolve potentially nested properties.

Because of this arrangement, it is impossible to declare a global transaction via e.g. @Transactional as there is no single entry point, contrary to Spring WebMVC where all activity happens within a controller method.

You see an increased number of queries because that's what happens here. Looking up references by their identifier is recommended to avoid lazy-loading issues. However, that approach comes at the cost of additional queries.

JPA implementations typically optimize to some extent using JOIN queries or batch lookups to avoid database roundtrips. They can leverage caches to avoid database hits if the objects are already in the session/2nd level cache.

You could de-proxy any objects within your DataFetcher or map these onto a DTO (another form of de-proxying). Still, such an approach requires a mapping utility such as MapStruct or manual copying of properties.

Spring GraphQL and the GraphQL engine can leverage reactive programming and asynchronous execution patterns, making it impossible to have ThreadLocal transactions out of the box as we cannot make any assumptions over the actual thread on which a call is being made.

@koenpunt
Copy link
Contributor Author

koenpunt commented Aug 5, 2022

[...] to have ThreadLocal transactions out of the box as we cannot make any assumptions over the actual thread on which a call is being made.

Not that I think that would be a solution on its own, but aren't there database clients for spring that support transactions over reactive streams?

Also; thanks for the extensive response.

@mp911de
Copy link
Member

mp911de commented Aug 5, 2022

There is reactive transaction support, but since there is no @Transactional entry-point for the entire GraphQL request processing, we cannot start/clean up the transaction.

@koenpunt
Copy link
Contributor Author

koenpunt commented Aug 5, 2022

but since there is no @Transactional entry-point for the entire GraphQL request

Isn't the entrypoint where the @QueryMapping or @MutationMapping annotation is?

@mp911de
Copy link
Member

mp911de commented Aug 5, 2022

No, the entry point is the WebGraphQlHandler that holds on to DataFetchers (typically the one that handles the Query element in the request document) and interceptors. The config infrastructure creates DataFetchers for annotated methods.

@rstoyanchev
Copy link
Contributor

rstoyanchev commented Aug 5, 2022

A more concrete sample or snippets of schema types, entity objects, and controller methods would help.

If it is only a @QueryMapping method then yes, that is a single point of entry. However, often there are additional @SchemaMapping methods for nested fields, and no single point of entry, at least not in application code. The last such point is the web interception chain that calls ExecutionGraphQlService and that in turn calls GraphQL Java which invokes DataFetchers, and that's what controller methods are registered as.

@rstoyanchev
Copy link
Contributor

Keep in mind that even then, there is no single Reactor chain since GraphQL's AsyncExecutionStrategy invokes DataFetchers and expects CompletableFuture. We adapt Flux and Mono to that but it means it's not a fully connected Reactor chain through which context can propagate from start to end. Only a number of independent smaller chains, one for each DataFetcher. We do however propagate context from the transport layer to each of those smaller chains, so they all see whatever comes from that level.

@xenoterracide
Copy link

If the data fetchers are tied to the request, and the request received something like multiple mutations or even nested queries wouldn't it be possible to wrap the entire request in a transaction?

@rstoyanchev
Copy link
Contributor

rstoyanchev commented Sep 15, 2022

We've had an internal discussion.

The recent upgrade of our context propagation in #459 provides some new capabilities that make it easier to propagate context across data fetchers, and not only from the transport level to data fetchers. What we need to investigate are some options to hook into the start and end of a transaction.

For example if @Transactional is on a @QueryMapping method, then we need to start a transaction before this is invoked, and then close it after the entire query processing is done, which is obviously not the same as when the @QueryMapping method exits. That would be a place to start.

A single @MutationMapping method is easy to surround with a transaction. I don't know if nested mutations are possible or an option in GraphQL Java? For multiple mutations, there isn't anything to put @Transactional that would surround all of them, and so the question there is how to indicate transaction boundaries, perhaps a schema directive.

For @Transactional on a nested @SchemaMapping, it seems relatively straight forward in terms of participating in an existing transaction (assuming we start propagating transactional context). It is less clear what to do if the intent is to start a transaction, as it's not clear how we would hook into the right place for when it should end, or perhaps we might simply not support that.

@rstoyanchev rstoyanchev added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Sep 15, 2022
@rstoyanchev rstoyanchev added this to the 1.1 Backlog milestone Sep 15, 2022
@koenpunt
Copy link
Contributor Author

For multiple mutations, there isn't anything to put @transactional that would surround all of them, and so the question there is how to indicate transaction boundaries, perhaps a schema directive.

According to the graphql spec mutations are supposed to be processed in isolation, so I don't think there should be something running them in a single transaction.

@maciejwalkowiak
Copy link

One possible way to solve it could be setting custom queryExecutionStrategy on GraphQL object,
(as described here https://sigmoid.at/post/2021/05/03/graphql_java_spring_transactions/), but as far as I can see there's currently no way to configure queryExecutionStrategy at least not with Spring Boot autoconfigurations.

@maxhov
Copy link

maxhov commented Oct 10, 2022

One possible way to solve it could be setting custom queryExecutionStrategy on GraphQL object, (as described here https://sigmoid.at/post/2021/05/03/graphql_java_spring_transactions/), but as far as I can see there's currently no way to configure queryExecutionStrategy at least not with Spring Boot autoconfigurations.

You can actually configure it, by defining a GraphQlSourceBuilderCustomizer bean like this:

  @Bean
  GraphQlSourceBuilderCustomizer configurer(PlatformTransactionManager platformTransactionManager) {
    var executionStrategy = new TransactionalExecutionStrategy(platformTransactionManager, new SimpleDataFetcherExceptionHandler());
    return builder ->
        builder.configureGraphQl(
            graphQl ->
                graphQl
                    .mutationExecutionStrategy(executionStrategy)
                    .queryExecutionStrategy(executionStrategy));
  }

Unfortunately I couldn't find a nice way to get the Spring provided DataFetcherExceptionHandler ExceptionResolversExceptionHandler injected there, because it is manually instantiated in AbstractGraphQlSourceBuilder::build, so for the sake of the example I used SimpleDataFetcherExceptionHandler.

@igdianov
Copy link

igdianov commented Oct 27, 2022

Using JPA for GraphQL Queries is a tricky business to make it work around transactional boundaries with lazy JPA proxy initializations for entity attributes. You can take a look at the GraphQL Query Api for JPA Entity Models project that solves this problem and many other JPA Query problems, i.e. N+1, search criteria, batch loading, large query result streaming, etc..

I successfully integrated it with spring-grapql using code first schema generation approach from JPA entity models :)

https://github.com/introproventures/graphql-jpa-query

@bclozel bclozel modified the milestones: 1.2 Backlog, 1.x Backlog Jan 12, 2023
@rstoyanchev rstoyanchev added the in: data Issues related to working with data label May 25, 2023
@MorningK
Copy link

I also encountered this problem when using Subscription. I set jpa.open-in-view to true, but when I tried to get the FetchType as Lazy field, I got the exception message: failed to lazily initialize a collection of role: could not initialize proxy - no Session. I tried to implement a custom TransactionalExecutionStrategy, but it didn’t work.
For example, in this case, when trying to get the nutrients field of Menu, and the FetchType of nutrients happens to be Lazy, the above error will occur; while in Query, the same structure can work normally.

type Subscription {
  familyMemberMenusSubscribe(
    familyUserInfoId: ID!
    date: Date!
  ): [Menu!]!
}

type Menu {
  id: ID!
  energy: BigDecimal
  mealTime: MealTime
  ediblePeriod: EdiblePeriod!
  dishes: [MenuDish!]!
  nutrients: [MenuNutrient!]!
}

@Entity
@Table(name = "menu", schema = "nourish")
public class Menu {
  @Id
  @GeneratedValue(strategy = GenerationType.IDENTITY)
  private Long id;

  private LocalDate mealDate;

  @Enumerated(EnumType.STRING)
  private MealTime mealTime;

  private Integer diners;

  private OffsetDateTime createdAt;

  @ManyToOne
  @JoinColumn(name = "edible_period_id", nullable = false)
  private EdiblePeriod ediblePeriod;

  @OneToMany(mappedBy = "menu", cascade = CascadeType.PERSIST)
  @OrderBy("id ASC")
  private Set<MenuDish> dishes;

  @OneToMany(mappedBy = "menu", cascade = CascadeType.PERSIST)
  @OrderBy("nutrient.id ASC")
  private Set<MenuNutrient> nutrients;
}

class TransactionalExecutionStrategy extends SubscriptionExecutionStrategy {
    private final PlatformTransactionManager transactionManager;

    public TransactionalExecutionStrategy(PlatformTransactionManager transactionManager) {
      this.transactionManager = transactionManager;
    }

    @Override
    public CompletableFuture<ExecutionResult> execute(
        ExecutionContext executionContext, ExecutionStrategyParameters parameters)
        throws NonNullableFieldWasNullException {
      TransactionStatus transaction =
          transactionManager.getTransaction(TransactionDefinition.withDefaults());
      return super.execute(executionContext, parameters)
          .whenComplete(
              (executionResult, throwable) -> {
                if (throwable != null) {
                  transaction.setRollbackOnly();
                }
                if (transaction.isCompleted()) {
                  return;
                }
                if (transaction.isRollbackOnly()) {
                  transactionManager.rollback(transaction);
                } else {
                  transactionManager.commit(transaction);
                }
              });
    }
  }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: data Issues related to working with data status: pending-design-work type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests