Enhance direct access to protected, package-private, private and superclass entity fields in Hibernate ORM

[yrodiere]

Fixes #32735

In essence, this changes two things, but for for Hibernate ORM entities only:

1. We used to only replace access to public fields with getter/setter calls. This overlooked a few access patterns such as access to protected/package-private fields from other classes in the same package, or access to private fields from inner classes.
   We now also replace access to protected, package-private and private fields with calls to $$_hibernate_{read/write}_* (though obviously not in the constructor, since that would fail, or the getter/setter, since Hibernate ORM itself will do that).
2. We used to not replace access to fields with getter/setter calls when inside the getter/setter implementation of the subclass. Since Hibernate ORM doesn't, either, this effectively meant that a getter/setter defined in a subclasss would bypass all Hibernate ORM enhancements (lazy initialization, dirty tracking).
   We now replace such access with a call to $$_hibernate_{read/write}_*, so that we call Hibernate ORM's enhancements.

I didn't apply these to Panache MongoDB, because there's simply no equivalent to $$_hibernate_{read/write}_* for MongoDB, and calling get*()/set*() (which was my initial approach) could cause other problems.

I will need some serious review from someone familiar with bytecode enhancement and Hibernate ORM (@Sanne, pretty please!), because this is a rather fundamental change with potential for breaking stuff. I worry in particular about:

• The potential impact on applications that don't rely on public fields.
  Such applications, until now, didn't even get most of the field access enhancement code executed, because they didn't need to.
  But now that we enhance protected/private field access, we do need to scan those applications, be it just to conclude that they only access these fields in the corresponding getters and don't need any enhancement in the end. So we may discover performance issues or simply bugs in those applications (even if they don't need field access enhancement).
• The potential impact on applications that rely on complex models where we don't detect fields correctly,
  in particular if we don't correctly detect fields marked as transient using XML mapping: we will then replace field access with calls to $$_hibernate_{read/write}_* methods that may not exist.
  That could be considered a pre-existing bug though, since such applications would already behave badly if they have public fields marked as transient using XML mapping (we'd generate an unnecessary getter/setter pair).
  Also, I suspect Hibernate ORM also doesn't detect those XML-mapped transient fields correctly, so it may generate those $$_hibernate_{read/write}_* methods anyway.

For the reasons mentioned above, I really don't want to backport this patch.

[quarkus-bot]

/cc @Sanne (hibernate-orm), @gsmet (hibernate-orm)

[quarkus-bot]

Thanks for your pull request!

The title of your pull request does not follow our editorial rules. Could you have a look?

• title should count at least 2 words to describe the change properly

This message is automatically generated by a bot.

[Sanne]

Very interesting, thanks. There's one sentence in your description giving me pause:

We now replace such access with a call to super.get* / super.set*, so that we (indirectly) call Hibernate ORM's enhancements.

Arguably if the user is overriding a get/set, they might mean it. What if they didn't want the parent method to be called? That's the contract when one doesn't call "super.get/set" - it's hard to say if the omission in a trivial method implementation is intentional or just because there is no need.

[yrodiere]

What if they didn't want the parent method to be called?

So first, it might not have existed in the first place, since we generate getters/setters. Sometimes users apparently define the field in a superclass and the getter/setter in a subclass; don't ask me why. If the getter/setter in superclasses are generated then the call to super is safe.

If the getter/setter in superclass is not generated... well, as you said, hard to say what the user meant. I suppose we could avoid calling super in that case, but that would require more pre-processing/metadata. And I'm pretty sure we'll get bug report from users who simply throught we'd replace field access anyway.

Alternatively, maybe we should just stop generating/calling get*/set* methods and simply call the internal Hibernate ORM accessors (hibernate$$get_stuff). This would remove the problem since users are not expected to define/override these accessors. But it could lead to problems if some fields are marked as transient using e.g. XML mapping.
Maybe use our own accessors (panache$$get_stuff) and somehow get those enhanced by Hibernate ORM afterwards if necessary... How, though? I think Hibernate ORM only enhances "standard" getter/setters when we don't do extended bytecode enhancement...

I wouldn't know what to do about MongoDB, though. I'm not sure why we replace public field access with getter/setter calls there anyway, since to the best of my knowledge it doesn't do change tracking or lazy initialization...

[Sanne]

[..] and simply call the internal Hibernate ORM accessors [..] This would remove the problem since users are not expected to define/override these accessors. But it could lead to problems if some fields are marked as transient using e.g. XML mapping.

To simply call the internal ORM accessors was my thought as well. Could you elaborate on the "problems" with transient? I'm not sure what the problem would be.

[yrodiere]

[..] and simply call the internal Hibernate ORM accessors [..] This would remove the problem since users are not expected to define/override these accessors. But it could lead to problems if some fields are marked as transient using e.g. XML mapping.

To simply call the internal ORM accessors was my thought as well. Could you elaborate on the "problems" with transient? I'm not sure what the problem would be.

Well if the field is marked as transient in XML mapping, I wouldn't expect the ORM accessor to work correctly (since, well, it's not a field ORM should care about). Maybe it does work though...

EDIT: Though I suppose we could process XML mapping during the build to extract that information.

[Sanne]

ah, got it know. I misunderstood by "XML mapping" that the object was also being mapped with e.g. JAXB annotations.

Yes right I wouldn't know if it creates any problem to mark a field as dirty even when the field is transient/ignored. (I don't think it hurts but I'm not sure); either way that's something we could do something about and I feel this is a more robust approach - that said I don't mean to say you should definitely do that now: up to you.

[yrodiere]

Yes right I wouldn't know if it creates any problem to mark a field as dirty even when the field is transient/ignored. (I don't think it hurts but I'm not sure); either way that's something we could do something about and I feel this is a more robust approach - that said I don't mean to say you should definitely do that now: up to you.

I'll see what can be done and will investigate the test failures.

Thanks for the feedback!

[FroMage]

LGTM, when the tests will pass ;)

[yrodiere]

LGTM, when the tests will pass ;)

Yep I'm on it, that's what #33832 was about :)

[yrodiere]

Yes right I wouldn't know if it creates any problem to mark a field as dirty even when the field is transient/ignored. (I don't think it hurts but I'm not sure); either way that's something we could do something about and I feel this is a more robust approach - that said I don't mean to say you should definitely do that now: up to you.

I'll see what can be done

I think I'll have to fix superclass field access directly in Hibernate ORM in the end. I will get a closer look tomorrow.

[yrodiere]

I rebased on #33844 because that's necessary for tests to pass. Converting to a draft PR until #33844 gets merged.

I also changed the handling of field access in subclasses as per Sanne's suggestion. We now replace such access with a call to $$_hibernate_{read/write}_*, so that we call Hibernate ORM's enhancements without mistakenly changing the meaning of the original code.

I suppose we could replace field access with $$_hibernate_{read/write}_* instead of getters/setters everywhere, but that would slightly change the behavior when developpers define non-anemic models (getters/setters that actually do something), so I'd rather not do that unless there's a reason.

[yrodiere]

Looks like the current approach causes problems when there are already getters, but with a slightly different return type (e.g. primitive vs. boxed):

2023-06-07T16:55:45.5372549Z Caused by: org.hibernate.boot.MappingException: HHH000474: Ambiguous persistent property methods detected on io.quarkus.it.hibernate.search.orm.elasticsearch.propertyaccess.TransientMethodAccessEntity; mark one as @Transient : [private java.lang.Long io.quarkus.it.hibernate.search.orm.elasticsearch.propertyaccess.TransientMethodAccessEntity.getId()] and [public long io.quarkus.it.hibernate.search.orm.elasticsearch.propertyaccess.TransientMethodAccessEntity.getId()] : origin(io.quarkus.it.hibernate.search.orm.elasticsearch.propertyaccess.TransientMethodAccessEntity)

That's arguably something we should solve independently from this PR (because it could affect applications that declare public field + getters/setters), but for the moment I chose to sidestep the problem: I updated the PR again to use $$_hibernate_{read/write}_* for both non-public field access and superclass field access. We won't generate getter/setter pairs for non-public field access, and won't be using existing ones. This has the advantage of greatly limiting the impact on applications that declare their own getters/setters.

I updated the PR description and force-pushed.

[yrodiere]

I addressed the bugs detected by the test suite. Let's see if everything is fine now.

[quarkus-bot]

✔️ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.