Query.setUnmodifiable(true) - 16.0.0-beta1 & 17.0.0-beta1 released

[rbygrave]

I am releasing versions:

• 16.0.0-beta1 ... try this if your on 14.x
• 17.0.0-beta1 ... try this if your on 15.x

These version have removed Query.setReadOnly() ... and replaced it with Query.setUnmodifiable(true).

PR

For the PR with related comments etc - #3570

Query.setUnmodifiable(true)

The beans / graph returned is effectively fully immutable(*1). Any attempt to mutate it will produce an exception.

• The beans can't be mutated via setters
• All the collections are unmodifiable collections
• Lazy loading is disabled and throws an exception if invoked
• The query uses PersistenceContextScope.QUERY

(*1) Some bean properties like say a @DbJson with mutable object ... is still going to be mutable. Something like java.util.Calendar is still mutable - but we can't do anything about those types.

Query.setReadOnly() removed

It was felt that keeping the existing "Read Only" was going to be too confusing as there is a lot of overlap with "Unmodifiable". Ultimately I'd suggest that "Unmodifiable" is what Read Only was trying to be.

As this change removes Query.setReadOnly() we are doing this with a bump to the major version. This means we are bumping 14.x to 16.x and bumping 15.x to 17.x.

Query Caching (change in behaviour)

Results of the Query cache are now ALWAYS immutable. Ebean previously supported returning shallow copies etc but that has changed and now the result of a query cache is always an unmodifiable.

Feedback desired

Looking for feedback along the lines

• Tried it and removing setReadOnly() was disappointing because ...
• Tried it and hit issues with using query caching [now unmodifiable] ... was a pain because ... [have to make our own copies of the beans etc?]
• Tried it and using setUnmodifiable() was disappointing because ...
• Tried it and using setUnmodifiable(true) was awesome, we immediately detected some unwanted lazy loading, great stuff, please release a non RC version!!

Robs initial feedback

Tried it on a couple of apps and yes I immediately found some undesired lazy loading. Do I have a lot of places where I would want to use Unmodifiable? Yes, it feels like I'm going to want to use unmodifiable quite a lot.

Downsides removed Read Only?
I honestly haven't used query.setReadOnly(true) as much so I've not hit any issues in that sense.

Downsides PersistenceContextScope.QUERY?
So it's using a "Per query scoped persistence context" meaning that IF the query is run inside a transaction, and there are prior queries executed in that transaction (that load up the persistence context) then an unmodifiable will NOT make use of those already loaded beans in the transaction scoped persistence context. Is that an issue? So far that's not an issue. Note that unmodifiable can't really include other [mutable] beans that previously loaded into the PC so this is not something that can change.

Downsides Query Caching?
No, in that query cache results were all treated as read only before anyway so no issue here.

Downsides Bean Caching?
Note that queries using unmodifiable true can't currently hit the bean cache. This is a limitation but there is a plan for this. If you have a query and want to hit the bean cache you CAN'T use unmodifiable true.

---

Followup questions:

So if setUnmodifiable() is successful, then there is the follow up question which is:

Should the results of query.setDisabledLazyLoading(true) ... FAIL and throw LazyInitialisationException when there is an attempt to lazy load an unloaded property?
That is, should we merge in: #3594

My current personal thought is that, YES - we should merge that in and throw LazyInitialisationException for setDisabledLazyLoading(true).

• This is good for the use case where we want mutable beans but want to ensure lazy loading doesn't occur
• This is BAD for the use case where have some mapping code that is mapping entity to dto ... [like MapStruct code say].

Can we support both of these use cases nicely?
Well, I think we can by perhaps explicitly setting a bean state to ... "skip any lazy loading".

Say:

MyEntityBean entityBean = ...;
MyDtoBean myDto = ...

// turn OFF lazy loading including any throwing of LazyInitialisationException 
// ... do this explicitly before we hit mapping code
EbeanLazyLoading.disable(entityBean);

// we have mapping code that maps ALL properties on the bean
// .. and does not know about load state / partially loaded entity beans

// entityBean will return NULL for any unloaded properties including unloaded collections
mappingEntityToDto(entityBean, myDto);

EbeanLazyLoading.disable(entityBean); is something that does NOT exist yet. The thought here is that it could, and we would use it explicitly before we use mapping code [e.g. MapStruct generated code].

[nedge]

The approach you have outlined for the implementation of Query.setUnmodifiable is consistent, intuitive and fixes the issues of setReadOnly (I was always against returning null instead of throwing an exception if LL was invoked on a readOnly bean,
Great work!

[rbygrave]

Followup questions:

Should the results of query.setDisabledLazyLoading(true) ... FAIL and throw LazyInitialisationException when there is an attempt to lazy load an unloaded property? That is, should we merge in: #3594

Well, no one is telling me NOT to merge it. My gut is telling me we merge this, and that:

• makes things more consistent with unmodifiable
• Returning NULL was never a great idea anyway ??
• Allowing lazy loading on collections also seemed flawed

Merging 3594 might hurt the use cases that are mapping entity beans, but maybe we can support that use case better with something like a future feature ~ EbeanLazyLoading.disable(entityBean);

So yes, lets merge #3594 and lets get another RC release going.

[tbee]

Hmmm. Not knowing exactly what the problem of readonly is, I personally view lazy loading as the materialising of parts of a (by the query) fully determined graph. So the "graph" is the fully interconnected thing, and readonly/immutable means that no changes can ever happen on that full graph. But the actual in-memory representation of that full graph is optimised to not always take all the memory; it can lazy load parts if required. So readonly/immutability for me does not exclude lazy loading. (This would resemble the version style of transactions in databases?)