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

Repository to (optionally) return managed JPA entities (JPA integration) #264

Closed
1 of 4 tasks
lprimak opened this issue Sep 17, 2023 · 13 comments
Closed
1 of 4 tasks

Comments

@lprimak
Copy link
Contributor

lprimak commented Sep 17, 2023

Description

As far as I understand it, the repository finder methods return POJOs and not managed entities. In order to integrate with existing JPA codebases, it would be very useful if managed entities were returned.

As a:

  • Application user/user of the configuration itself
  • API user (application developer)
  • SPI user (container or runtime developer)
  • Specification implementer

All that would be needed is a reference to EntityManager. I propose to infer EntityManager from CDI,
similar to what's implemented here: https://docs.flowlogix.com/#section-daohelper

@Inject
// Inject default EntityManager
@EntityManagerSelector
MyRepository<Entity, Key> repository;
@Inject
@EntityManagerSelector(NonDefaultEM.class)
MyRepository<Entity, Key> repository;

where NonDefaultEM is a custom @Qualifier annotation that points to a producer method that injects the appropriate EM.
Default would be no-qualifier injection, which would work in most cases.

@otaviojava
Copy link
Contributor

otaviojava commented Sep 18, 2023

Hello @lprimak, how are you?

I updated the spec to return the EntityManager using conventional methods.

https://github.com/jakartaee/data/blob/main/spec/src/main/asciidoc/repository.asciidoc#resource-accessor-methods

Also, we are discussing about the entity lifecycle on this issue:

#224

Do you think it would help?

@lprimak
Copy link
Contributor Author

lprimak commented Sep 18, 2023

Hi, Otavio!
First of all, thanks for your great work on this spec. It looks very promising!

  • Is my assumption that the finder methods don't return managed entities correct?

If my assumption is correct, I would want the ability to pick which EntityManager manages the returned entities. Since an application may have multiple databases and entity managers, it's not enough for the implementation to pick one. I want to be able to pick one in application code, thus @EntityManagerSelector annotation.

This would also permit the return of managed entities from finder methods, using the correct entity manager.

Hopefully this makes things more clear.

Thank you!

@njr-11
Copy link
Contributor

njr-11 commented Sep 18, 2023

  • Is my assumption that the finder methods don't return managed entities correct?

The spec still needs to be updated to explicitly say so, but the intent is to return entities that are not managed, consistent with a stateless entity manager.

If my assumption is correct, I would want the ability to pick which EntityManager manages the returned entities. Since an application may have multiple databases and entity managers, it's not enough for the implementation to pick one. I want to be able to pick one in application code, thus @EntityManagerSelector annotation.

@EntityManagerSelector is one idea on how that could be done, but there could be other options as well. I think we should defer discussion over managed entities to the subsequent version of Jakarta Data. There is already a lot going into version 1, and we need to focus on ensuring we get a core set of function specified correctly.

@lprimak
Copy link
Contributor Author

lprimak commented Sep 18, 2023

I think we should defer discussion over managed entities to the subsequent version of Jakarta Data.

Understood, with the implication that it would limit Data's usefulness when adding to current JPA-based codebases.

I think in that case, spec needs to be updated to say so explicitly, with a caveat that the feature may be added in the future to integrate more tightly with JPA

Does spec mention how a persistence unit is picked currently, it there are multiple ones? I might have missed it or it's not there.
Does it use persistence.xml file or not?

@njr-11
Copy link
Contributor

njr-11 commented Sep 18, 2023

Does spec mention how a persistence unit is picked currently, it there are multiple ones? I might have missed it or it's not there.
Does it use persistence.xml file or not?

The spec has dataStore on the @Repository annotation for selecting which to use. This is provided as a String for general compatibility with a variety of providers, some JPA-based, some JDBC-based, some NoSQL-based, or other. The intention is to eventually be able to use it with Jakarta Config for more detailed configuration. But Jakarta Config won't be in Jakarta EE 11. In the mean time, you just point it at the name of a resource that your provider requires. That could be a persistence unit name for JPA, a DataSourceDefintion name or JNDI name, or some other identifier. Jakarta Data specification itself does not mandate the use of any XML files, including those from JPA. That part is up to the provider, whose responsibility it is for documenting any underlying resources that it requires and their configuration.

@lprimak
Copy link
Contributor Author

lprimak commented Sep 18, 2023

Ah, that's what I was missing, thanks Nathan!

@lprimak lprimak closed this as not planned Won't fix, can't repro, duplicate, stale Oct 16, 2023
@lprimak lprimak reopened this Nov 7, 2023
@njr-11 njr-11 added this to the Jakarta Data Future milestone Nov 7, 2023
@lprimak lprimak changed the title Repository to return managed JPA entities (JPA integration) Repository to (optionally) return managed JPA entities (JPA integration) Nov 7, 2023
@gavinking
Copy link
Contributor

We can solve this issue by specifying that a resource accessor method may be annotated with a CDI qualifier annotation, for example:

@Repository
interface JpaRepo {

    @DocumentDatabase
    EntityManager entityManager()

     ...

}

WDYT?

@gavinking
Copy link
Contributor

gavinking commented Feb 22, 2024

The spec has dataStore on the @Repository annotation for selecting which to use. This is provided as a String for general compatibility with a variety of providers, some JPA-based, some JDBC-based, some NoSQL-based, or other.

Sorry, @njr-11, I had missed this comment.

That indeed is reasonable as a way to disambiguate datastores and persistence units by name.

On the other hand, it doesn't let me use CDI qualifiers, as in my previous comment, or as in the issue description.

@gavinking
Copy link
Contributor

gavinking commented Feb 22, 2024

Alternatively, I guess you could used CDI method injection, for example:

@Repository
interface JpaRepo {

    @Inject void init(@DocumentDatabase EntityManager entityManager);

     ...

}

But I actually hate method injection and I guess I think this is significantly worse. (If, admittedly, more standard.)

@lprimak
Copy link
Contributor Author

lprimak commented Feb 22, 2024

I personally like the CDI qualifier solution, but either one will work for me :)
Thank you guys!

@gavinking
Copy link
Contributor

I have opened issues #476 and #470, and I would like to close this issue. Any objections?

@lprimak lprimak closed this as completed Feb 22, 2024
@njr-11
Copy link
Contributor

njr-11 commented Feb 22, 2024

I have opened issues #476 and #470, and I would like to close this issue. Any objections?

That sounds good. I'll spend some time looking into/thinking about whether it could make sense to have both ways of configuring which data source/persistence unit reference to use

@gavinking
Copy link
Contributor

I'll spend some time looking into/thinking about whether it could make sense to have both ways of configuring which data source/persistence unit reference to use

Perfect!

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