Replace use of Getter.get/Getter.getForInsert/Setter.set and ReflectionOptimizer with Hibernate Accessors

[marko-bekhta]

So this is another approach to make the setter/getter functionality more pluggable and to open up the way to improve it even further with better build-time generated accessors...

In this attempt we took a different approach with @dreab8. Instead of trying to bite off too much and replace the getter/setter entirely and integrate the Hibernate Models and use the various *Details we decided to be more focused and only eliminate the usage of Getter.get/Getter.getForInsert/Setter.set and ReflectionOptimizer .

Key classes to look at here are

• PropertyValueAccessor -- see how it is a one-can-do-it-all, which was on purpose to keep it "monomorphic".
• PropertyAccessStrategy -- SPI change, we require PropertyAccessorService as an arg now.
• BytecodeProvider.java -- and dropped ReflectionOptimizer ... while it's easy enough to keep the Getter/Setter methods it's a bit more complex to preserve the ReflectionOptimizer and wire it through the new APIs so we just dropped it with the idea it being an SPI, and us going back to a 8.0 to deprecate it there ? 🤞🏻
• OrmHibernateAccessorFactory -- since accessors know nothing of ORM they cannot generate a multiaccessor the way ORM wants it with various checks for unfetched properties... so instead it delegates where it can and implements the multiaccessor generation on the ORM side. There are a couple of interesting things in there too, like OrmBridgeClassGenerator .. but I am still looking into making ORM pull more from accessors rather then repeat the code from it.

oh and yeah this won't build as it needs accessors that are ... nowhere 😔 🙂

---

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license
and can be relicensed under the terms of the LGPL v2.1 license in the future at the maintainers' discretion.
For more information on licensing, please check here.

---

[hibernate-github-bot]

Thanks for your pull request!

This pull request does not follow the contribution rules. Could you have a look?

❌ All commit messages should start with a JIRA issue key matching pattern HHH-\d+
    ↳ Offending commits: [43977e1, 3547fde, 888729d, 2a4f23b, d6d6975, 390e3c0, ea9acfc, 42318f1, 0de3b5a]

› This message was automatically generated.

[beikov]

The changes look ok to me, but what is the success criteria? Being able to reuse code without performance regressions? Improve performance even? Have new APIs to allow more efficient value access in a follow up PR? Not trying to talk it down or am against it, I just want to understand the scope that your targeting.

I understand that we can still improve the code a bit and that you chose to delegate rather than replace in a lot of places to keep changes isolated and reviewable. I would like to hear what improvements you think we could do next though, because for now, it seems like we're adding more code and more indirections.

[marko-bekhta]

The main goal is to isolate all property access so it goes through one route, which can be plugged in/replaced easily enough and use a single abstraction across all Hibernate projects for that.

A secondary goal is to make it easier to remove/replace Setter/Getter in the future. We've tried going that route right now and tried using various *Details from models more, but we've run into all sorts of issues because of hbm/enverse/reveng ... so instead of trying to replace it here, we are just removing the get/set functionality from them and keeping the "metadata" access ones in place.

As a side effect of the change, we are also getting better performance for single-value accessors since those do not go to reflection anymore (in general) and use a switch-per-class generated at runtime.
There are some limitations to what we can generate at runtime, so with "easy pluggability" in place, the idea is that we do things differently where we can generate accessors at build time.

There are some constraints we try to impose here too... like we aren't going to have any custom/inline impls of accessor interfaces unless the thing goes through that single path of service/factory (and even then think twice before doing so 🙂). Which also drives the decision of not trying to use any Map-backed readers/writers and instead having the PropertyValueAccessor, which is not trivial to look at 🙂 but it also should keep the perf on a good level.
Or another constraint to keep in mind is that this is used by Search/Validator, so we cannot just push ORM-specific logic into the accessor implementation.

As to what's next -- the obvious things like: remvoe get/set from Getter/Setter since we aren't using them anymore internally and we just need to make sure no integration relies on it since it's in the SPI. You did mention on the other PR a few things about records, so I'd like to have a look at what's going on in ORM right now with them.

maybe @dreab8 will have something to add too 🙂

[beikov]

Ok, that sounds promising. Thanks for the clarification.

[sonarqubecloud]

Quality Gate failed

Failed conditions
0.0% Coverage on New Code (required ≥ 80%)
5.4% Duplication on New Code (required ≤ 3%)
B Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE