typesafe way to pass options and an EntityGraph to find()

[gavinking]

Currently, setting the CacheStoreMode or CacheRetrieveMode requires the use of string typing, the jakarta.persistence.cache.retrieveMode or jakarta.persistence.cache.storeMode properties.

This is obviously bad, and therefore:

• We should add setCacheStoreMode() and setCacheRetrieveMode() to EntityManager and to Query.
• It might even be worth introducing a superinterface of CacheStoreMode, CacheRetrieveMode, and LockModeType, calling it FindOption, for example, along with new overloads of find() and refresh() with signature T find(Class<T> entityClass, Object primaryKey, FindOption... options).

[gavinking]

There's a similar, related problem with jakarta.persistence.loadgraph and jakarta.persistence.fetchgraph, though those are a little trickier.

[gavinking]

It might even be worth introducing a superinterface of CacheStoreMode, CacheRetrieveMode, and LockModeType, calling it FindOption, for example, along with new overloads of find() and refresh() with signature T find(Class entityClass, Object primaryKey, FindOption... lockMode).

• It might even be worth introducing a superinterface of CacheStoreMode, CacheRetrieveMode, and LockModeType, calling it FindOption, for example, along with new overloads of find() and refresh() with signature T find(Class<T> entityClass, Object primaryKey, FindOption... lockMode).

Actually, scratch that, a better design would probably be a FindOptions class which lets you specify:

• the cache modes,
• the lock mode,
• the jakarta.persistence.lock.timeout and PessimisticLockScope, and/or
• a load graph or fetch graph,

and then add an overload with the signature:

T find(Class<T> entityClass, Object primaryKey, FindOptions options)
void refresh(Object entity, FindOptions options)

So this FindOptions class would just be a typesafe replacement for the Map argument of find().

[gavinking]

So, after quite a lot of reflection on this, I'm now inclined to go for something much more like how Hibernate does this, but with cleaned-up naming.

I generally prefer newing objects to "fluent" APIs, but I think in this case the fluent approach has advantages.

You see, the tricky part of this is passing the EntityGraph in a typesafe way, without adding verbosity and/or complexity, and the fluent approach actually handles that really nicely.

Consider:

var graph = entityManager.createEntityGraph(Book.class);
graph.addSubgraph(Book_.publisher);
Book book = 
        entityManager.find(Book.class)
                .fetching(graph)
                .with(lockModeType)
                .with(cacheStoreMode)
                .withTimout(2000)
                .get(id);

This is all nice and typesafe, and only requires adding one new interface EntityFinder, or whatever.

Now, I guess I can make it work with FindOptions something like this:

var graph = entityManager.createEntityGraph(Book.class);
graph.addSubgraph(Book_.publisher);
var options =   // this is a FindOptions<Book>
        graph.fetch()
                .with(lockModeType)
                .with(cacheStoreMode)
                .withTimeout(2000);
Book book = entityManager.find(Book.class, id, options);

And the signature would be:

<E> E find(Class<T>, Object, FindOptions<? super T> options)

And there would also be a way to obtain a FindOptions<Object> which would never have any entity graph. Something like:

var options =   // this is a FindOptions<Object>
    entityManager.options()
                .with(lockModeType)
                .with(cacheStoreMode)
                .withTimeout(2000);

So it works, I guess. But that just seems a bit more complex for no real advantage.

[gavinking]

I mean, look, the following variation works, and I don't hate it, but it still doesn't seem better:

var graph = entityManager.createEntityGraph(Book.class);
graph.addSubgraph(Book_.publisher);
var options =   // this is a FindOptions<Book>
        FindOptions.fetching(graph)
                .with(lockModeType)
                .with(cacheStoreMode)
                .withTimeout(2000);
Book book = entityManager.find(Book.class, id, options);

var options =   // this is a FindOptions<Object>
        FindOptions.create()
                .with(lockModeType)
                .with(cacheStoreMode)
                .withTimeout(2000);
Book book = entityManager.find(Book.class, id, options);

[gavinking]

Folks, please review the new API proposed in https://github.com/jakartaee/persistence/pull/415/files.

[gavinking]

Everyone, please take a look at my proposed API in #436.

[gavinking]

@lukasj has suggested a different approach, modeled on CopyOption in the java.nio.file API. I really like this idea. In fact, it's actually an idea I mentioned above, but didn't really do it justice, and then forgot about:

• It might even be worth introducing a superinterface of CacheStoreMode, CacheRetrieveMode, and LockModeType, calling it FindOption, for example, along with new overloads of find() and refresh() with signature T find(Class<T> entityClass, Object primaryKey, FindOption... options).

So we will:

1. make enums like LockModeType, CacheStoreMode, CacheRetrieveMode into implementations of a new FindOption interface, and add other subclasses like Timeout, etc, and then
2. add a new overload to EntityManager:

   <T> T find(Class<T> entityClass, Object primaryKey, FindOption... options);

This has the advantage that it's quite extensible by providers.

[gavinking]

Okay, folks, the approach taken in #454 is:

1. Add the marker interface FindOption.
2. Make the enums LockModeType, CacheStoreMode, and friends implement FindOption, and introduce a new Timeout class.

With this proposal you can write:

Book book = em.find(Book.class, bookId, 
                    PESSIMISTIC_WRITE, 
                    CacheStoreMode.BYPASS, 
                    Timeout.ms(200));

And, even better:

var bookGraph = em.createEntityGraph(Books.class);
bookGraph.addAttributeNode(Book_.authors);
Book book = em.find(bookGraph, bookId, 
                    PESSIMISTIC_WRITE, 
                    CacheStoreMode.BYPASS);

The graph is treated as a "load graph". Now, the issue with that is that there's no way to suppress loading of associations mapped EAGER. But that's an issue I need to think about and I'll update the proposal later.

[gavinking]

Now, the issue with that is that there's no way to suppress loading of associations mapped EAGER.

I mean, the simplest possible solution would be to just add removeAssociationNodes() to Graph.

Another possibility would be to add createFetchGraph() and createLoadGraph() operations to EntityManager, so that graphs would come with a "type". But there's two problems with that:

1. I'm just not convinced we should double down on what I always felt was just not a good design in the first place.
2. In principle if you use a "load graph" to suppress EAGER association fetching, you need explicitly to add back all the basic/scalar attributes you would like loaded. This isn't ergonomic at all, and it would create portability problems, since, for example, in Hibernate you actually wouldn't in practice need to do that.

[gavinking]

OK, so, how about adding these methods to allow you to suppress association loading in a load graph.

[gavinking]

We need to decide whether FindOptions can be passed to refresh(), or whether we need a separate RefreshOption interface.