Skip to content

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

Merged
merged 21 commits into from
Jul 3, 2025
Merged

Conversation

stIncMale
Copy link
Member

@stIncMale stIncMale commented May 9, 2025

This PR introduces tests for null values, including tests for empty @Embeddable values (ones that have null as the value of each of the persistence attributes), and the commit c4986fa has hacked-in implementation of null support to allow running those tests. You may identify such tests by searching for @Disabled.

HIBERNATE-58

@stIncMale stIncMale self-assigned this May 9, 2025
@stIncMale stIncMale changed the title Support collections mapping Support arrays/collections mapping May 9, 2025
@stIncMale stIncMale force-pushed the HIBERNATE-58 branch 2 times, most recently from e93da8e to 8281ac4 Compare May 9, 2025 14:00
Comment on lines 261 to 358
@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);
}
Copy link
Member Author

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.

Copy link
Contributor

@NathanQingyangXu NathanQingyangXu May 22, 2025

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.

Copy link
Contributor

@NathanQingyangXu NathanQingyangXu May 22, 2025

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):

Screenshot 2025-05-22 at 5 11 32 PM

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.

Copy link
Member Author

@stIncMale stIncMale May 27, 2025

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_handling property is set to allow, Byte[] instead of byte[] 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.

Copy link
Contributor

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

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

Copy link
Contributor

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.

@stIncMale stIncMale force-pushed the HIBERNATE-58 branch 6 times, most recently from 8c3bbe7 to a32c9c4 Compare June 2, 2025 16:26
@stIncMale stIncMale force-pushed the HIBERNATE-58 branch 7 times, most recently from 6490332 to 82aa14c Compare June 16, 2025 06:49
@stIncMale stIncMale force-pushed the HIBERNATE-58 branch 3 times, most recently from 5ed0734 to cb2f2d3 Compare June 17, 2025 14:49
@stIncMale stIncMale marked this pull request as ready for review June 18, 2025 09:16
Copy link
Contributor

@NathanQingyangXu NathanQingyangXu left a 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.

@NathanQingyangXu
Copy link
Contributor

NathanQingyangXu commented Jul 3, 2025

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 @Column should take effect. I tested it on main branch and found it works as expected, but it seems not to hurt to provide an integration testing case to protect us. An example testing case is like below:


@SessionFactory(exportSchema = false)
@DomainModel(
        annotatedClasses = {
                TestStructWithCustomizedColumnName.Book.class,
                TestStructWithCustomizedColumnName.Author.class
        })
@ExtendWith(MongoExtension.class)
class TestStructWithCustomizedColumnName {

    @InjectMongoCollection("books")
    private static MongoCollection<BsonDocument> mongoCollection;

    @Test
    void testFieldNameIsCustomized(SessionFactoryScope scope) {
        var book = new Book();
        book.id = 1;
        book.title = "Effective Java";
        var author = new Author();
        author.firstName = "Joshua";
        author.lastName = "Bloch";
        book.author = author;
        scope.inTransaction(session -> session.persist(book));
        assertThat(mongoCollection.find()).containsExactly(BsonDocument.parse(
                """
                {
                  "_id": 1,
                  "title": "Effective Java",
                  "author": {
                    "first_name": "Joshua",
                    "last_name": "Bloch"
                  }
                }
                """
        ));
    }

    @Entity(name = "Book")
    @Table(name = "books")
    static class Book {
        @Id
        int id;
        String title;
        Author author;
    }

    @Embeddable
    @Struct(name = "Author")
    static class Author {
        @Column(name = "first_name")
        String firstName;
        @Column(name = "last_name")
        String lastName;

    }
}

var embeddableMappingType = getEmbeddableMappingType();
var jdbcValueCount = embeddableMappingType.getJdbcValueCount();
var result = new Object[jdbcValueCount];
for (int columnIndex = 0; columnIndex < jdbcValueCount; columnIndex++) {
Copy link
Contributor

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.

@katcharov katcharov merged commit e19bd11 into mongodb:main Jul 3, 2025
6 checks passed
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.

4 participants