Background
For a background to this question with respect to ORM modelling of concatenated primary keys.
Update 13th Sept
So it's been stated: a bytecode transformer can't remove the final modifier of a record field. So ends the story.
For what its worth I'll add the thoughts that came out of our review.
This issue as we saw it really boiled down to the final modifier and the view record types are a language feature (not a jvm feature) and the issues around what that means.
In that way you can almost paraphrase the posted question as: Are records a language feature or a jvm feature? We could view the first part of the response as - Yes, records are a language feature (hence the requirements on javac with jdk support and semantic requirements of equals/hashCode etc).
All the various questions around breaking record semantics equals/hashCode, accessors, constructors, customisation of those etc - this all reinforced the view that record types are indeed a language feature. We were super happy to get those [false] claims because we could prove via tests that nothing was broken and we could explain the details on why that was.
Q: But it's dodgy removing the final modifier and we broke records right? Well, it went to effectively final / effectively immutable. Another way of looking at that is to play devils advocate and see how to stuff it up - e.g. If we were to create a record instance, partially populate it and hand it off in that partially populated state that would stuff up equals/hashCode. Obviously you don't hand off a partially loaded record / partially initialised instance. Where we ended was more the question around whether record type could go from being a language feature to a jvm feature (would a future jvm assume a final) and thoughts around that.
To be clear, we don't have a failing test or jvm error or anything like that we can point to - there is no case where the ebean bytecode transformer is applied to records and that breaks anything. What we do have is the question around the assumption that record types are a language feature vs jvm feature, and that question of effectively final/effectively immutable vs actual final/immutable [a question of semantics like equals/hashCode vs bytecode & Java memory model "proper construction" etc].
Update 12th Sept
Ultimately I think there are 2 ways to look at this:
Language view: Record types are extremely important, they allow pattern matching are a kind of golden key that will unlock lots of cool language features going forward. The details don't matter and the message is simple - don't anyone **** this up !!
Details view: When we look at the bytecode, semantics, java memory model and we compare to how we would write shallowly immutable types without records we see exactly nothing new. No new bytecode, no different semantics etc. This is typified by ORM @EmbeddedId being an exact match to record type. Similarly the changes ebean needed to make to support record type where exactly none.
Brian read 'mutable' and 'not final' and fired his bazooka and that is fair enough. What the question didn't say was 'effectively immutable', 'effectively final', 'late initialisation' - heck, its even a language feature in kotlin - lateinit.
A bytecode transformation agent that does not even know about record types is lined up for some choice words. What is it actually getting wrong? Well, once you get into the details - nothing.
Q: But the semantics of Record::equals() is new? Not to a bytecode transformer no. The only way to **** this up is for a dev to provide a customised equals/hashCode implementation and for that to not follow the semantics of Record::equals() - but that is on the dev providing the equals/hashCode implementation and not on the bytecode transformer.
Also noting the semantics of Record::equals() match the old and existing @EmbeddedId. This actually isn't new from an ORM perspective.
Q: So ebean supported java records by doing nothing?
Well yeah, ebean doesn't require a default constructor and hence we been supporting shallowly immutable types for years. Hence records presented as nothing new. Cool and useful but nothing new.
I'll write up all the details and we will have a review and go from there.
Update 11th Sept - Review session
I will look to organise a session for people who are interested in the details around this issue. The questions Brian has posed. What record type bytecode looks like, what the enhancement does and why it does it. What actually occurs when we deal with customisation code in constructor, equals/hashCode, accessors, toString etc (it's actually pretty simple once you understand what its doing).
I'll happily take any test using @Embeddable,record,junit5, Java 16. If it fails under enhancement I will buy you a beer! We likely will ask permission to add the test to our test suite (Apache2).
Ebean being in the ORM business deals with interesting problems like interception, lazy loading, partial objects, dirty checking etc. Bytecode transformation is a commonly used tool in this space because it can greatly simplify how we handle some tough problems. Record type are nice, interesting and useful but they also don't present anything new to ebean bytecode transformation and in fact have the same semantics and needs of EmbeddedId.
Next steps: Have the review session and determine how to proceed.
Update 11th Sept
- Still no actual evidence of incorrect behaviour, broken semantics, incorrect bytecode
- Brian has expressed concern that the ebean transformation does not support the semantics of
Record::equals(). This concern is misplaced, there is nothing new, different or difficult here as far as the bytecode transformation is concerned. We are now really solidly in the comfort zone of what the ebean transformation does. The chance of a real problem here has significantly dropped. The chance of an issue specific to record type has now gone to almost zero. To explain that, we have no problem with the record supplied equals/hashCode implementations. If people provide customized equals/hashCode implementation then these of course must honor the semantics of record but that aspect is on the author of those implementations - as far as ebean bytecode transformation is concerned it just needs to support a provided implementation (in interception terms) that but this no different to the non-record normal class case. There is nothing new or different here in terms of what the ebean transformation has been doing for 16 years.
Summary
- Currently there is no evidence of incorrect behaviour, broken semantics, incorrect bytecode
- Brian is concerned that the bytecode transformation might not handle the cases of customisation of constructor, accessors, equals, or hashCode methods. This is very good news indeed because I'd suggest Brian's concern is misplaced. To explain that, these customisations are not specific to record type and are cases that the bytecode transformation has to deal with in normal classes (normal non-record
@Entity and @Embeddable classes). These cases is what ebean has been dealing with for 16 years now and I'd suggest is highly battle tested.
- To state clearly, an issue in this area around customisation of those methods is extremely unlikely to be specific to record type. Let that sink in.
- Of course there is a chance we might manage to find a new edge case in this area where the bytecode transformation does work correctly. If you could pick someone to do that, Brian would be your pick. The good news here is that these cases and this area are absolutely in our wheelhouse and I'd be confident of fixing them.
Details
As the author of the bytecode transformation in question I'll just add some details.
TLDR: The intention and my expectation is that the semantics of record (as I understand those semantics) is still 100% honored. At this stage I need to get more clarity on the specific semantics that Brian is unhappy about.
- The transformed record still looks immutable and acts immutably to code using it (effectively immutable)
- The semantics of hashcode() and equals() is honored (unchanged)
- The semantics and result of accessor methods is unchanged
- The semantics and result of toString() is unchanged
- The semantics and result of constructor us unchanged
The effective change of this bytecode transformation is that the bytecode is going from 'strictly shallowly immutable at construction' to 'effectively shallowly immutable allowing for some late initialisation'.
The late initialisation that can occur is transparent to any code/bytecode that uses the transformed record - code using the transformed record will experience no difference in behaviour or result and I would suggest no difference in semantics.
Code using this transformed record will still think it is immutable and is not able to mutate it.
For folks familiar with Kotlin lateinit it is a bit similar to that - still effectively immutable but allows for late initialisation of the record fields in question. [With 'late' meaning after construction]
Also noting that the transformation is adding some extra fields, methods and some interception on the accessors all for 'internal use only' and nothing is added to the record publicly in terms of fields or methods - none of this is visible to code using the transformed record. My expectation is that they have not changed the semantics of record but more clarity is required here.
The fields have the final modifier removed to allow late initialisation. This means from a Java memory model perspective we have indeed lost the nice 'final on construction JMM semantic' that we get with final fields in general. I'd be super surprised if this was the specific issue but ideally we get that clarified.
In reviewing the bytecode again and reading the comments above it isn't yet clear to me what the specific semantics of records Brian is especially unhappy about. As I see it the possible options could be:
- No transformation of records is allowed at all?
- No extra fields or methods are allowed at all even if they are internal only?
- We can't go from 'strictly immutable at construction' to 'effectively immutable with late initialisation'? (Noting the associated slight JMM change due to loss of final modifier on those fields)
Again, the semantics and results of all record methods (hashcode, equals, toString, constructor) are unchanged so it would be good to get good clarity on what the specific party foul is and hence what specific semantics are in question.
Edit:
Quick overview example
Before transformation
@Embeddable
public record UserRoleId(Integer userId, String roleId) {}
After transformsation (without synthetic fields and methods, IntelliJ decompile to source form)
@Embeddable
public record UserRoleId(Integer userId, String roleId) implements EntityBean {
private Integer userId;
private String roleId;
public UserRoleId(Integer userId, String roleId) {
this._ebean_intercept = new InterceptReadWrite(this);
this._ebean_set_userId(userId);
this._ebean_set_roleId(roleId);
}
public Integer userId() {
return this._ebean_get_userId();
}
public String roleId() {
return this._ebean_get_roleId();
}
}
Diff in bytecode:
I've put the before and after in bytecode form in a second answer as otherwise we exceed the character limit here.
Folks reviewing the bytecode, please have a look at that second posted answer.
Edit re customisation of record types
Brian has suggested that "but it only seems to work for records that do not customize the constructor, accessors, equals, or hashCode methods".
This isn't the case. Customisation of all those is expected, allowed, handled + multiple constructors. To explain that a bit more, these cases are not different to the non-record class cases that we have already been dealing with for many years. Ebean is 16 years old, we have been doing bytecode transformation for that time.
Q: Have we made mistakes in the past?
Absolutely.
Q: Do we have a mistake in what the transformation is doing with @Embeddable record?
At the moment we have no evidence of a mistake. (ok, it's a bug that we have that _ebean_identity field there but I've just fixed that).
Although record type is new'ish (Java 16) the concept of shallow immutability isn't new and bytecode wise records are not too different from immutable types we have been able to code forever in java.
The JPA spec requires default constructor (btw: a restriction soon to be removed it seems) and getters/setters but ebean does not have those restrictions. This means that the customisations that Brian mentions are things that ebean transformation has had to deal with for a very long time - 16 years actually, as these are all the things with expect with non record entity classes. For the mutating entity class case there are other slightly interesting things that we need to deal with (around collections) that we do not for record types.
That is, there isn't any customisation of record types that would be new or different to what the ebean transformer has been dealing with.
Another detail to put in here is that the JVM didn't always enforce final. From vague memory from around Java 8 or so the JVM really did enforce final. This is the sort of little detail that might be concerning/nagging Brian.
Edit:
Absolutely we should not be taking things personally but lets put this in context. I've been in the Java community for 25 years, I'm the organiser of the local JUG, I have an open source project that is 16 years old that is taking a serious reputational hit right here.
Brian Geotz, a literal Java God has said "pretty serious party foul", "shamed out of the community", "ignorance", "poisoning the well" - to someone like me who is a Java fanboy these are literally hammer blows from a God. Being referred to as "the author" doesn't actually soften those blows in case you were wondering, in fact it hurts more because it suggests this isn't a really serious issue. In case it isn't obvious, I'm taking this issue really really really seriously and I'll be pushing to get right to the detail of this and confirm if there is indeed a problem with the bytecode transformation here.
24 hours in and I'm holding up. I might even foolishly be thinking I am starting to get to the heart of the matter. The current TLDR is probably that you should not take on bytecode transformation unless you really know what you are doing. For myself, I've got 16 years of taking bytecode transformation seriously. I'm not ignorant of the size of the challenge and the depth of knowledge you need to get this right. This is not tiddly winks.
At this stage we don't actually have evidence of wrong doing and it's more a suggestion that ebean might be incorrectly handling customisation of record types. This is actually really really good news to me because I've got 16 years of experience to fall back on that suggests that the bytecode transformation does indeed cover all the cases Brian is concerned about (plus other cases thrown in by Kotlin, Scala and Groovy compilers and other cases thrown in by mutable types).
Record types are actually the nice easy case as far as ebean transformation is concerned.
Next steps:
Can we get actual evidence of ebean transformation doing the wrong thing?
Brian might be able to give me a curly example to test out and report back the bytecode. I think this is where we are at.
