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

Improve lookupFragmentInterface to supports a better hierarchy in repository composition [DATACMNS-1537] #1965

Closed
spring-projects-issues opened this issue May 21, 2019 · 15 comments
Assignees
Labels
status: declined

Comments

@spring-projects-issues
Copy link

@spring-projects-issues spring-projects-issues commented May 21, 2019

arielcarrera opened DATACMNS-1537 and commented

Improve method lookupFragmentInterface in CdiRepositoryBean to supports a better hierarchy in repository composition

Check my test project:

https://github.com/arielcarrera/cdi-spring-data-jpa-test

Usage of CustomJpaRepository/Impl in a base interface.

https://github.com/arielcarrera/cdi-spring-data-jpa-test/blob/master/src/main/java/com/github/arielcarrera/cdi/repositories/helpers/CustomJpaRepository.java

 

 


Issue Links:

  • DATACMNS-1539 Repository fragments do not work when indirectly extended
    ("is duplicated by")
@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented May 21, 2019

Jens Schauder commented

arielcarrera please clarify what you want improved, what exactly isn't working?

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented May 21, 2019

arielcarrera commented

Hello Jens Schauder, if you have a project using the CDI Extension, when you make a compose with your repository fragments in a multilevel hierarchy and if you want to add some base fragment with custom implementation... it is not working.
with some minor changes improving the repository fragments lookup to extend support for hierarchies, it can be more flexible.
You can check my example project, I did a set of fragment repositories in hierarchy and, for example, added a Custom implementation in a top level interface that injects and implements a helper method (to provide entity manager so it can be used in fragments with default methods)... the current cdi fragments lookup is a little more simple.
Regards

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented May 21, 2019

Mark Paluch commented

Sorry, I do not understand what you're trying to achieve. Please outline your use-case with a simple code sample without us requiring to understand your repository code

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented May 21, 2019

arielcarrera commented

Hi Mark, I will make a sample project without my fix. Give me a while

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented May 21, 2019

Mark Paluch commented

No need for a sample project in the first place. Having interface declaration stubs and class stubs here should be sufficient

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented May 21, 2019

arielcarrera commented

Hi Mark, I do a new sample project with a minimal repository hierarchy.

When you execute my junit Test (RepositoryTest.java) you get something like this:

org.jboss.weld.exceptions.DeploymentException: Failed to create query for method public abstract javax.persistence.EntityManager com.github.arielcarrera.cdi.repositories.fragments.DFragment.customMethodEntityManager()! No property customMethodEntityManager found for type TestEntity!org.jboss.weld.exceptions.DeploymentException: Failed to create query for method public abstract javax.persistence.EntityManager com.github.arielcarrera.cdi.repositories.fragments.DFragment.customMethodEntityManager()! No property customMethodEntityManager found for type TestEntity!
....
Caused by: java.lang.IllegalArgumentException: Failed to create query for method public abstract javax.persistence.EntityManager com.github.arielcarrera.cdi.repositories.fragments.DFragment.customMethodEntityManager()! No property customMethodEntityManager found for type TestEntity! at org.springframework.data.jpa.repository.query.PartTreeJpaQuery.<init>(PartTreeJpaQuery.java:84) at org.springframework.data.jpa.repository.query.JpaQueryLookupStrategy$CreateQueryLookupStrategy.resolveQuery(JpaQueryLookupStrategy.java:106)
....
org.springframework.data.mapping.PropertyReferenceException: No property customMethodEntityManager found for type TestEntity! at org.springframework.data.mapping.PropertyPath.<init>(PropertyPath.java:94) 

So, in my new sample project you have:

  • 3 minimal repository fragments (with one method)
  • 1 minimal repository fragment with 1 custom method Implementation (with Impl class)

https://github.com/arielcarrera/spring-data-jpa-test-DATACMNS-1537/tree/master/src/main/java/com/github/arielcarrera/cdi/repositories/fragments

 

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented May 21, 2019

arielcarrera commented

I do not know if the minimum example that I have uploaded is sufficient to understand the flexibility that could be achieved by improving the search for fragments in a hierarchical way. Tell me if you do not understand something or I'm wrong, or if you want a PR to test a possible improvement. Best regards

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented May 21, 2019

Mark Paluch commented

Thanks a lot. So what you basically want to achieve is using fragments in ``@NoRepositoryBean``s.

This is currently not supported, neither in when using Spring's dependency management nor using CDI. We currently only inspect top-level repository interfaces only for fragment declarations and do not consider ones that are @NoRepositoryBean annotated nor are implemented by a @NoRepositoryBean.

Changing this approach results in a more extensive scanning. The actual question is: Where do we stop? Each fragment causes a scan for a custom implementation. This is already cached but nevertheless we potentially would scan for implementations of CrudRepository and the like.

We also need to find a proper rule for base packages. Right now, fragment implementation are expected to be found within the either configured base packages (for Spring) and the fragment interface package for CDI. Typically, you'd expect the implementation somewhere close to the fragment interface and so we would scan for components outside the repository interface

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented May 21, 2019

arielcarrera commented

I will try to check my code and improve my lookup method... I hope to send to you a draft or a pull request in a few days

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented May 22, 2019

Mark Paluch commented

This is not about code changes in the first place, it is about defining how fragment lookup is supposed to work and whether we want to incorporate this change at all

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented May 22, 2019

arielcarrera commented

You're right Mark, discuss if you're interested in the change, in my personal opinion I think it brings several advantages compared to the current implementation.
First of all, the change would be backward compatible.

Secondly, It would add more tools to customize the repositories allowing to make more complex compositions that are usually needed for build base/core classes of projects.

It would allow for example to create a class/fragment "Helper" with methods like getEntityManager, getUserTransaction, getCriteriaBuilder, etc ... and inherit it from any interface in the hierarchy (without extend a class).
Better still, if one could access the repository information (by injecting it) and be able to expose methods such as getEntityClass, getPK, getPKName, getPKType, etc. This would further facilitate the possibility of creating small custom implementations.
A little example of these advantages can be seen in my code, in the repository implementation that allows a "Soft Delete" in any entity that implements "LogicalDeletion" interface (there is a similar open issue about soft deletes DATAJPA-307).
As you can see in my example, it would allow to have a repository with physical deletion and another one with logical deletion in a very simple way.

Of course the same can be achieved without the need to support this but I think this can be more flexible.

 

Regarding the disadvantage of requiring a more extensive scan:

"Where do we stop?": Different strategies could be proposed for example by means of a cache to avoid cyclic references, a parameter of maximum depth, an annotation to enable or disable scanning, etc.

"Each fragment causes a scan for a custom implementation. This is already cached but nevertheless we would like to scan for implementations of CrudRepository and the like": If it is saved in cache and it is done only once, the search for the implementation does not seem like a great penalty, however you could evaluate any strategy to skip or enable this type of search by annotation, or initialize the cache with a "No implementation" value for some interfaces, etc.

Please let me know if I can be of any help

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Aug 9, 2019

arielcarrera commented

Hi Mark Paluch,  do you have any news about this? Can I help you with something?

@spring-projects-issues spring-projects-issues added status: waiting-for-feedback in: repository type: enhancement labels Dec 30, 2020
@mp911de mp911de added status: feedback-provided and removed status: waiting-for-feedback labels Dec 30, 2020
@mp911de
Copy link
Member

@mp911de mp911de commented Jan 27, 2021

Duplicates #2142.

@arielcarrera after reiterating a few times, we came to the decision to not implement fragment support. Resolving hierarchies along with method precedence introduces another level of complexity and we want to keep things simple.

@mp911de mp911de closed this as completed Jan 27, 2021
@mp911de mp911de added status: declined and removed in: repository status: feedback-provided type: enhancement labels Jan 27, 2021
@arielcarrera
Copy link

@arielcarrera arielcarrera commented Jan 27, 2021

Duplicates #2142.

@arielcarrera after reiterating a few times, we came to the decision to not implement fragment support. Resolving hierarchies along with method precedence introduces another level of complexity and we want to keep things simple.

Ok Thanks Mark, Is there no chance to provide a default implementation and open the possibility to provide a custom implementation / discovery leaving this complexity in the hands of the user?

@mp911de
Copy link
Member

@mp911de mp911de commented Jan 27, 2021

You can assemble a repository via …RepositoryFactory by providing the actual fragment composition yourself. The limiting factor in Spring Data is that we don't descend into implemented interfaces when scanning for repository fragment interfaces.

Something along the lines of:

JpaRepositoryFactory factory = new JpaRepositoryFactory(entityManager);

MyRepository repo = factory.getRepository(MyRepository.class, RepositoryComposition.RepositoryFragments.of(RepositoryFragment.implemented(…), RepositoryFragment.implemented(…)));

The order of RepositoryFragments decides over method precedence.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: declined
Projects
None yet
Development

No branches or pull requests

3 participants