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

Unmodifiable - Improved read only immutable entity beans / graphs #3570

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

rbygrave
Copy link
Member

@rbygrave rbygrave commented Feb 21, 2025

Unmodifiable entities are:

  • immutable, will throw on attempts to mutate via setters
  • disabled lazy loading [don't mutate via lazy loading]
  • can be partially populated [with some properties unloaded]
  • throw on attempts to read unloaded properties including unloaded collections
  • collections "freeze" to unmodifiable collections
  • mutable types (e.g. DbArray) also "freeze" to unmodifiable if supported
  • Uses PersistenceContextScope.QUERY [must do so in order to avoid connecting to modifiable entities in PC]

Changes

  • Adds new UnloadedPropertyException and UnmodifiableEntityException
  • Changes from using IllegalStateException to UnloadedPropertyException, UnmodifiableEntityException, or UnsupportedOperationException
  • A query that is read only + disabled lazy loading "upgrades" to unmodifiable

Ugh

"Read Only" and "Unmodifiable" should probably be exactly the same thing/concept but read only existed before and so it slightly different [it does not include/imply disabled lazy loading + PersistenceContextScope.QUERY ]. Ideally we change query.setReadOnly(true) to be exactly the same as query.setUnmodifiable(true).

…LazyLoad=true]

Although this is ok, makes me think that another option is just to have query.setReadOnly(true)
to mean ... readOnly + disableLazyLoad + error reading unloaded property or collection. As in,
readOnly true without these extra things does not that good [as in the existing readOnly does
not seem very good/safe/useful to use].
… existing use of IllegalStateException

- Adds new exceptions UnloadedPropertyException and UnmodifiableEntityException
- Change InterceptReadOnly to use these exception instead of IllegalStateException
- Change collections BeanSet, BeanList, BeanMap from using IllegalStateException to UnsupportedOperationException [to bring these in line with JDK unmodifiable collections]
Add unmodified to DefaultOrmQuery query plan description
Fix incorrect merge conflict
@rbygrave
Copy link
Member Author

FYI @nedge

@rbygrave rbygrave changed the title Improved read only immutable Unmodifiable - Improved read only immutable entity beans / graphs Feb 21, 2025
Copy link
Contributor

@rPraml rPraml left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello Rob, I took a short look on this PR.
I'm looking forward to this change.

We've always used a cumbersome combination of persistence-scope-query + readonly + cache etc.

I have to say that we also use the BeanCache.

I haven't looked at your PR in detail about what happens if data is partially present in the BeanCache and a partial bean is created and the missing data then has to be reloaded using lazy load

@rPraml
Copy link
Contributor

rPraml commented Feb 24, 2025

"Read Only" and "Unmodifiable" should probably be exactly the same thing/concept but read only existed before and so it slightly different [it does not include/imply disabled lazy loading + PersistenceContextScope.QUERY ]. Ideally we change query.setReadOnly(true) to be exactly the same as query.setUnmodifiable(true).

I would agree that. (if really necessary, we couldbadd a config flag for backward compatibility)

But I think it still needs to be specified what should happen when caches are involved, as they already check the readOnly flag.

QueryCache:

Beans/IdLists in the queryCache should be always unmodifiable by default. (we use setUseQuerCache=true always together with setReadonly=true)
I can imagine only very few use cases, where this is not necessary (we have exacly one)

It is also necessary to set the persistence-scope to QUERY when beans are put into the queryCache, otherwise we got memory leaks.

So I have to pay attention always to use all 3 properties together correctly in my code.

Therefore, I would appreciate it if setUseQuerCache(true) would set everything correctly (or maybe an other convenient method like withQueryCaching())

BeanCache

Imagine if I do the query DB.find(...).select("prop1").setId(42).setUseBeanCache(true).findOne()

This will put partial data to the bean cache.

Next I do a select("*").setImmutable(true) and will have a beanCache hit.

What will we expect?

  • got a partial loaded bean that throws a LazyLoadException on access to an unloaded property (that would be bad)
  • or can a bean from the cache do lazy loads anyway
  • or has the cache to validate, if there are all properties present and treat this situation as cache miss

mutable types

There are mutable types like Calendar or @DbJson properties. The later we use a lot. (you may remember JsonMutationDetection)

How do we handle this and protect from modification?

I think there is not much we can do about this, which would be totally OK for me, if it is specified.
(But I could imagine that custom json objects also can implement the freeze method later)

Roland

Noting that the freeze needs to occur after secondary queries have executed
@rbygrave
Copy link
Member Author

query cache ... always to use all 3 properties together correctly ... appreciate it if setUseQuerCache(true) would set everything correctly

Yes that makes sense. Yes I think we should migrate to setUseQuerCache(true) using unmodifiable implicitly.

mutable types

Some we can freeze (and we do in this PR) and some we can't do anything about (like Calendar).

bean cache

We need to figure this out but maybe we can't use the existing bean cache with unmodifiable. I have some thoughts around a new/different "bean cache" made of immutable beans (actually immutable graphs). My intention is to look at that first.

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

Successfully merging this pull request may close these issues.

3 participants