Improve integration with `spring-data-jpa`

[david-kubecka]

Update: Originally there was a different description but through a thorough discussion the goal can now be summarized as:

• facilitate Spring Data repository naming conventions while allowing to pass EntityViewSetting as an argument to repository methods

To understand the motivation please read the discussion. The gist is here: https://github.com/Blazebit/blaze-persistence/issues/1559#issuecomment-1280984619

[beikov]

Thanks for the interest. We have an open issue for this already, which is https://github.com/Blazebit/blaze-persistence/issues/1511 You can watch that issue for updates.

[david-kubecka]

Before filing this issue, I looked at the existing ones including #1511 and now I'm unsure whether my original issue description was clear enough so I will try again :-)

In this issue, I ask for a new feature where you could combine autogenerated DB methods by naming convention (Spring Data feature) and the ability to craft optimal SQL based on GraphQL DataFetchingEnvironment (Blaze feature). #1511, on the other hand, seems to be just about creating an example project, i.e. not a new feature.

Or did I misunderstand the purpose of #1511?

[beikov]

The part .. and maybe also an integration if needed is about integrating with the Spring GraphQL module. The Spring Data integration of Blaze-Persistence already supports almost everything that you seem to be looking for. You just have to wire up the GraphQL endpoint to the repository method.

Do I understand it right, that what you want is to get rid of the code that wires up the GraphQL endpoint and the repository method?

[david-kubecka]

I see now where the misunderstanding might come from :-) I'm not asking for the Blaze integration with spring-data-graphql but for the integration of the existing blaze-persistence-integration-graphql with spring-data-jpa, namely with JpaRepository and its naming-convention based methods (see my example). Getting rid of the extra wiring code you are talking about will be a consequence of this new integration.

To be more specific, instead of the current getAllUsers one should be simply able to use just the predefined findAll method. That could be defined either as

List<T> findAll(DataFetchingEnvironment dfe)

or

List<T> findAll(EntityViewSetting evs)

The latter option assumes that the setting is constructed from provided DataFetchingEnvironment in the controller. In a later version, I can imagine that even this can be automated and DataFetchingEnvironment-based view setting can be created and passed automatically.

On top of the last paragraph, apart from the predefined JpaRepository interface methods, one should able to also define his own methods, e.g. findByLowerCaseName mentioned in the original issue description, which should also be GraphQL-context-aware.

I hope that this makes more sense now.

(Note that I know that there already is blaze-persistence-integration-spring-data-X but that doesn't seem to be GraphQL-aware. AFAIK)

[beikov]

Ok, that's a different thing then. I guess that this is something that will be Spring GraphQL only though, so this kind of depends on an integration with that first, right?

[david-kubecka]

AFAIK spring-graphql provides mostly the convenience controller annotations for automatic wiring of DataFetchers and query Arguments. As you said (and I agree with that), this can be utilized as is even today with acceptable overhead (as can be seen in the example repo).

What is not easily possible is to use those spring-data-jpa methods like getAllUsers or findAll. To enable that, better integration with spring-graphql wouldn't help IMO. Instead, you should improve the integration with spring-data-jpa, e.g. similarly to what was already done with specifications where you can either pass findAll (and others) the standard Specification or the BlazeSpecification. What I'm asking for here is basically to allow us to pass findAll also some GraphQL context, either in the "raw" format of graphl-java DataFetchingEnvironment or wrapped into Blaze EntityViewSetting. Sure, one can also create the methods manually with CriteriaBuilder as seen in getAllUsers. It's just lot of quite repetitive work.

To sum up, while better integration with spring-graphql certainly be convenient, the more important missing part is to provide the convenience of Blaze-powered spring-data-jpa methods.

[beikov]

Thanks for the clarification. I guess that should be doable :)

[david-kubecka]

Thanks for accepting this issue :-)

Could you please briefly describe your battle plan for how to implement that? I've briefly looked at how spring-data-jpa processes the named queries. It uses CriteriaBuilder internally but there seems to be no way to hook onto the process, e.g. by injecting EntityViewSetting. So I'm curious about what you had in mind.

[beikov]

We did add support for "injection" of custom parameter types like EntityViewSettingProcessor into repository query methods in the past, so I think this is a matter of adding support for yet another parameter type in a similar fashion. That's one possible battle plan :)

I'd also look into how Spring Data implements this itself though, to possibly leverage some SPI that I don't know of yet. Can you share some details about how this GraphQLExecutor stuff and the passing of DataFetchingEnvironment is supposed to work in other contexts? I couldn't find any reference to passing a DataFetchingEnvironment to a repository or what this GraphQLExecutor interface is supposed to do. Also, from which project is GraphQLExecutor?

[david-kubecka]

GraphQLExecutor is the suggested new interface, its name inspired by JpaSpecificationExecutor which "enhances" the find methods with a Specification parameter. Similarly, the proposed GraphQLExecutor could enhance the standard find methods with a DataFetchingEnvironment or EntityViewSetting parameter (the easy part), but also do the same for the derived query methods (IMO the hard part).

Hope it's now more clear.

[beikov]

By suggested you mean that you propose Blaze-Persistence should support this? Because I thought you meant that Spring already has something like that, but couldn't find this in the Spring repository: https://github.com/spring-projects/spring-graphql

Adding support for passing the DataFetchingEnvironment is not that hard in general, but unless Spring also supports that, I'm not sure if it's a good idea to add this. I actually thought that this is mostly about exposing a repository method directly as GraphQL query 😆

[david-kubecka]

Yes, I propose that BP supports it. Sorry for being unclear :-)

Well, spring-graphql concentrates on the automatic wiring of controllers and input types to queries/mutations. It doesn't assume anything about the actual persistence layer (there might not even be an underlying DB at all!), so that's why in principle it can't interact with that. OTOH, BP with its EntityViews is IMO in an excellent position to utilize DataFetchingEnvironment by passing it down to the persistence layer.

this is mostly about exposing a repository method directly as GraphQL query

Yeah, that's basically it. Note that this is mainly spring-data-jpa specific and by itself doesn't require any extra integration with spring-graphql. At least in the first step when the app code would manually construct EntityViewSetting from DataFetchingEnvironment. As a second nice-to-have step, you could automate even that by further integration with spring-graphql:

@QueryMapping
fun getUsers(
       @Argument filterInput: FilterInput?,
       setting: EntityViewSetting,
): List<User>

Note: FilterInput is a custom class from which BlazeSpecification will be constructed.

The repository method should then look like

<T> findAll(setting: EntityViewSetting<T>, specification: BlazeSpecification): List<T>

[beikov]

Ok, so here are is concrete proposal:

The Spring Data integration should support a DataFetchingEnvironment as special parameter type for repository methods. When the return type of the method is a subclass of Page, it will use GraphQLEntityViewSupport#createPaginatedSetting(graphql.schema.DataFetchingEnvironment), otherwise it uses GraphQLEntityViewSupport#createSetting(graphql.schema.DataFetchingEnvironment) to construct the EntityViewSetting.

Is that what you hand in mind? Or am I missing something?

[david-kubecka]

Yes, that was one of the options I had in mind. The other one was to pass EntityViewSetting directly. The first option has both pros and cons:

• Pros: It nicely encapsulates EntityViewSetting construction from the DataFetchingEnvironment.
• Cons: It binds the presentation and persistence layers, i.e. the latter one knows about graphQL which it shouldn't.

Thinking more about it I tend to give more weight to the con. Such kind of interlayer binding is usually a sign of design smell. What if I want to expose my entities (via views) in both graphQL and REST CRUD endpoints? Then I would have to have two separate repos for each use case which surely isn't optimal.

So I prefer the second option when I pay a small price (constructing specific EntityViewSetting manually) for greater design flexibility. In fact, the price should be really negligible because paging in controllers is a repeating concern and as such should be extracted somewhere anyway. Moreover, I'm sure that auto-construction of the setting from the DataFetchingEnvironment in the controller can be implemented later.

Another feature I would really love and probably didn't mention before (and which would be possible only with the second option) is having the repository find methods generic in the view type, i.e.:

public interface CatViewRepository extends EntityViewRepository<Cat, Long> {

     // for cases when you are ok with the autogenerated EntityViewSetting
    <V, Q> List<V> findAll(BlazeSpecification specification, Pageable pageable);

    // here you can pass your own setting, e.g. created from DataFetchingEnvironment
    // perhaps here should/could be Pageable as well?
    <V, Q> List<V> findAll(EntityViewSetting<V, Q> setting, BlazeSpecification specification);
}

I don't know whether that's even feasible with the current design (note that there is no entity view type in EntityViewRepository parametrization) but if yes then that will IMO be the ultimately correct separation of concerns because it would enable the presentation layer to specify the required view without bothering with technical details while achieving optimal DB performance. The usage could then be:

@Controller
public class MyCatController {

    @Autowired
    private CatViewRepository catViewRepository;

    public Iterable<SimpleCatView> getCatDataForDisplay(final int minAge) {
        return catViewRepository.findAll<SimpleCatView>(someSpec);
    }

    public Iterable<FullCatView> getCatDataForExport(final int minAge) {
        FullCatView fullCatView = EntityViewSetting.create(FullCatView.class).applySomeConfig()
        return catViewRepository.findAll(fullCatView, someSpec);
    }
}

Also, I realize now that what I actually want is perhaps not graphQL-specific at all 🤣 because this generic design is fully independent of the API system (graphQL, REST, SOAP, etc.)! Sorry for taking me so long to realize that. I'm still in the process of learning and digesting all the design possibilities of the BP framework and it takes some time.

To sum up, while your (actually originally also mine) suggested solution would satisfy my use case I don't think it would be a good design choice in the long run. I presented an alternative that is more general and that promotes clean and reusable architecture. I'm also sure that there might be yet other solutions satisfying the same design requirements and which didn't come to my mind yet. Of course, ultimately it is your choice of direction where you want BP to evolve.

[beikov]

So I prefer the second option when I pay a small price (constructing specific EntityViewSetting manually) for greater design flexibility. In fact, the price should be really negligible because paging in controllers is a repeating concern and as such should be extracted somewhere anyway. Moreover, I'm sure that auto-construction of the setting from the DataFetchingEnvironment in the controller can be implemented later.

Good to hear that your thinking move in the same direction as mine. I also think that such an integration would limit the flexibility. So let's not do the support for DataFetchingEnvironment in Spring Data repositories for now, and instead maybe allow EntityViewSetting or something like that in the GraphQL endpoint.

Another feature I would really love and probably didn't mention before (and which would be possible only with the second option) is having the repository find methods generic in the view type, i.e.:

That's already possible by passing a Class. Also see https://docs.spring.io/spring-data/jpa/docs/current/reference/html/#projection.dynamic

Given that we currently only support passing a Class and an EntityViewSettingProcessor separately, but the general abstraction is the EntityViewSetting object itself, I think that we might want to change the scope of this issue to the following:

Allow Spring Data repository methods to accept EntityViewSetting as parameter type.

What do you think?

[david-kubecka]

That's already possible by passing a Class. Also see https://docs.spring.io/spring-data/jpa/docs/current/reference/html/#projection.dynamic

Oh, I totally missed that 😱 Does it work OOTB with entity views created from DataFetchingEnvironment? I.e. are the queries tailor-made to the input selection set?

Allow Spring Data repository methods to accept EntityViewSetting as parameter type.

Agree! Later I will change the issue subject and clean up the description because currently it's might be misleading. Still, there's one bit I'm not quite sure of - paging. Docs suggest that you can use either a separate Pageable (possibly KeysetPageable) parameter or have paging info already embedded into EntityViewSetting. This would also deserve some unification thoughts. Moreover, it's probably related to an issue you created some time ago (can't find it ATM) which is about simplification of the current verbose typing of EntityViewSetting which is related to paging. (Hope I'm not mixing this with something else.)

[beikov]

Oh, I totally missed that 😱 Does it work OOTB with entity views created from DataFetchingEnvironment? I.e. are the queries tailor-made to the input selection set?

Unfortunately, doing this requires a bit of work (calling some methods explicitly), but I personally doubt that it is a good idea to have different entity view types for a GraphQL endpoint. Please share how you would like to use this so I understand what is missing.

Docs suggest that you can use either a separate Pageable (possibly KeysetPageable) parameter or have paging info already embedded into EntityViewSetting. This would also deserve some unification thoughts.

What kind of unification are you thinking about? The support for Pageable in the integration is there because that's what Spring Data applications usually use. Internally, it is translated to a EntityViewSetting. We definitely need to document that mixing Pageable with EntityViewSetting will not work.

Moreover, it's probably related to an issue you created some time ago (can't find it ATM) which is about simplification of the current verbose typing of EntityViewSetting which is related to paging. (Hope I'm not mixing this with something else.)

I guess you are referring to https://github.com/Blazebit/blaze-persistence/issues/371 which will help to get rid of the type parameter that is needed to distinguish between the different criteria builder return types. That is unrelated.

[david-kubecka]

Unfortunately, doing this requires a bit of work (calling some methods explicitly), but I personally doubt that it is a good idea to have different entity view types for a GraphQL endpoint. Please share how you would like to use this so I understand what is missing.

I would like to have the proposed Allow Spring Data repository methods to accept EntityViewSetting as parameter type, but additionally I would also expect that the possible graphQL selection sets in EntityViewSetting (however it's implemented) are respected. Naively, I expect that the proposed integration of Spring Data with EntityViewSetting would work internally in a very similar manner as this example taken from the GraphQL examples. What's wrong with my assumption?

[david-kubecka]

We definitely need to document that mixing Pageable with EntityViewSetting will not work.

This is the kind of clarification I was looking after! When using EntityViewSetting there should be no reason for additionally using also Pageable.

[beikov]

I would like to have the proposed Allow Spring Data repository methods to accept EntityViewSetting as parameter type, but additionally I would also expect that the possible graphQL selection sets in EntityViewSetting (however it's implemented) are respected. Naively, I expect that the proposed integration of Spring Data with EntityViewSetting would work internally in a very similar manner as this example taken from the GraphQL examples.

Support for accepting EntityViewSetting in a Spring Data repository is essentially what the whole discussion here seems to boil down to. The selection sets are applied through the EntityViewSetting#fetch API, so yes, that would work for GraphQL then.

What's wrong with my assumption?

In the original request, you were talking about passing a Class for the entity view type to the Spring Data repository, but making the result type dynamic is not something that sounds very useful to me in the context of GraphQL. I'd like to understand what you are aiming at with this request or if I misunderstood you.

[david-kubecka]

In the original request, you were talking about passing a Class for the entity view type to the Spring Data repository, but making the result type dynamic is not something that sounds very useful to me in the context of GraphQL. I'd like to understand what you are aiming at with this request or if I misunderstood you.

There were a lot of turnarounds from my side, so some things I could have said might contradict each other :-) So to sum up, I'm still looking for

<V, Q> List<V> findAll(EntityViewSetting<V, Q> setting, BlazeSpecification specification);

which you have confirmed will be possible and will work as expected for the GraphQL-powered setting.

Since we agreed that passing GraphQL-specific stuff to repository methods is not a good idea, we ended up with this proposal where the entity view class is passed implicitly via setting. I agree that specifically for the GraphQL use case it doesn't make much sense to create more entity views for a single entity (and I do not plan to do that), but it's nice to be able to pass more EntityViewSetting<V, Q> for more Vs anyway, i.e. for non-GraphQL use cases (I don't have any yet but can imagine that in the future - perhaps some internal tooling).

Anyway, thank you for your diligence. Hope everything is clear now!

[beikov]

Yep, everything should be clear now. Thanks for clarifying.