-
Notifications
You must be signed in to change notification settings - Fork 13
Support arrays/collections mapping #95
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
Conversation
e93da8e to
8281ac4
Compare
| @Test | ||
| void testBoxedBytesArrayValue() { | ||
| var item = new ItemWithBoxedBytesArrayValue(); | ||
| { | ||
| item.id = 1; | ||
| item.bytes = new byte[] {1}; | ||
| item.boxedBytes = new Byte[] {2}; | ||
| } | ||
| // this is Hibernate ORM bug, and it goes away if the `ItemWithBoxedBytesArrayValue.bytes` field is removed | ||
| assertThatThrownBy(() -> sessionFactoryScope.inTransaction(session -> session.persist(item))) | ||
| .isInstanceOf(ClassCastException.class); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is likely a bug in Hibernate ORM causing this behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I digged a little bit. The root cause is BasicTypeRegistry registries both byte[] and Byte[] with value of the latter. If we use legacy as the hibernate.type.wrapper_array_handling property, the issue would be gone.
Not 100% sure but it seems this is as expected. We might need to know all the details of the new property introduced since 6.2 to fully grasp.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The org.hibernate.mapping.Property#isValid() provides some helpful context as below (https://github.com/hibernate/hibernate-orm/blob/main/hibernate-core/src/main/java/org/hibernate/mapping/Property.java#L293):
So from my understanding, if hibernate.type.wrapper_array_handling property is set to allow, Byte[] instead of byte[] is supposed to be used; otherwise legacy property might make more sense.
Still kind of defective but mixture of byte[] and Byte[] seems inconsistent with the allow configuration as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from my understanding, if
hibernate.type.wrapper_array_handlingproperty is set to allow,Byte[]instead ofbyte[]is supposed to be used
I did not find anything in the documentation, or even in the error message you linked, that points to such a limitation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can ask Christian on Zulip directly. Again he is the developer for this feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Christian created a Hibernate ORM ticket at https://hibernate.atlassian.net/browse/HHH-19573
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The bug has been fixed since v6.6.19. FYI.
...ionTest/java/com/mongodb/hibernate/embeddable/StructAggregateEmbeddableIntegrationTests.java
Outdated
Show resolved
Hide resolved
8c3bbe7 to
a32c9c4
Compare
6490332 to
82aa14c
Compare
5ed0734 to
cb2f2d3
Compare
HIBERNATE-58
HIBERNATE-58
HIBERNATE-58
…than requiring presence of any columns Addresses mongodb#95 (comment).
Addresses mongodb#95 (comment).
Addresses mongodb#95 (comment).
Addresses mongodb#95 (comment).
… an empty struct. Addresses mongodb#95 (comment).
src/main/java/com/mongodb/hibernate/internal/type/MongoStructJdbcType.java
Outdated
Show resolved
Hide resolved
NathanQingyangXu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my side, no LGTM blocker for me.
There is an interesting discussion regarding struct fetching robustness issue, but it could be done later, if the issue is valid.
…MongoAssertions.assertInstanceOf` method Addresses mongodb#95 (comment).
Addresses mongodb#95 (comment).
Addresses mongodb#95 (comment).
Addresses mongodb#95 (comment).
src/main/java/com/mongodb/hibernate/internal/type/ValueConversions.java
Outdated
Show resolved
Hide resolved
Addresses mongodb#95 (comment).
|
I should have raised this concern in last PR. I think we should create some testing case to showcase that Struct's field annotated with customized |
src/main/java/com/mongodb/hibernate/internal/type/MongoStructJdbcType.java
Show resolved
Hide resolved
| var embeddableMappingType = getEmbeddableMappingType(); | ||
| var jdbcValueCount = embeddableMappingType.getJdbcValueCount(); | ||
| var result = new Object[jdbcValueCount]; | ||
| for (int columnIndex = 0; columnIndex < jdbcValueCount; columnIndex++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there are still cases that var is not used consistently, though I am perfectly fine with it.
This PR introduces tests for
nullvalues, including tests for empty@Embeddablevalues (ones that havenullas the value of each of the persistence attributes), and the commit c4986fa has hacked-in implementation ofnullsupport to allow running those tests. You may identify such tests by searching for@Disabled.HIBERNATE-58