-
Notifications
You must be signed in to change notification settings - Fork 11
allow null value persistence and retrieval #111
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
base: main
Are you sure you want to change the base?
Conversation
|
||
private static final Set<String> COMPARISON_OPERATOR_NAMES_SUPPORTED = | ||
Set.of("$eq", "$ne", "$gt", "$gte", "$lt", "$lte"); | ||
|
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.
HIBERNATE-74 exceptions are intertwinced with HIBERNATE-48; we have to refrain from throwing HIBERNATE-74 exceptions from JDBC flow to unblock HIBERNATE-48. For this reason, I decoupled them and created a separate HIBERNATE-74 protected mechanism as above. It is more robust and is free of cache implication.
Given HIBERNATE-74 is to be done soon, we might well simply ignore the above protection.
if (domainValue == null) { | ||
throw new FeatureNotSupportedException( | ||
"TODO-HIBERNATE-48 https://jira.mongodb.org/browse/HIBERNATE-48 return toBsonValue(domainValue)"); | ||
return null; |
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.
we have to return null
instead of BsonNull
for we need to return domain value for bindValue
; Struct -> BsonDocument transformation should be the only exception; for that reason, I changed the method return type to better align with its purpose.
return extractor.getJavaType().wrap(domainObjects, options); | ||
} else { | ||
return extractor.getJavaType().wrap(array, options); | ||
} |
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 above code changes is to bypass a newly found Hibernate ORM bug. Luckily in this case we can work around.
I created the Hibernate ORM ticket at https://hibernate.atlassian.net/browse/HHH-19596. The bugfix has been merged to both v7 and v6.6.
Just a couple of days we could eliminate the above change after a new v6.6 Hibernate ORM version is released.
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 bumped the version to latest v6.6.21 and found the issue has been solved.
@@ -43,4 +42,9 @@ class Book { | |||
this.publishYear = publishYear; | |||
this.outOfStock = outOfStock; | |||
} | |||
|
|||
@Override | |||
public String toString() { |
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.
It looks like toString
was added mainly to make test output easier to read when troubleshooting. If that’s the goal, it might be worth including all fields so the log clearly reflects the full object state. Otherwise, partial output could still leave us guessing. Should we extend toString
to include all fields?
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.
In the current implementation, we use getBooksByIds(int... ids)
to go about the retrieval validation. If validation failed, we can easily figure out why based on the id after looking up in the testingBooks
list. From my opinion, id alone suffices and we don't need to update the toString()
method whenever we modify the entity.
...egrationTest/java/com/mongodb/hibernate/query/select/SortingSelectQueryIntegrationTests.java
Outdated
Show resolved
Hide resolved
src/main/java/com/mongodb/hibernate/jdbc/MongoPreparedStatement.java
Outdated
Show resolved
Hide resolved
src/main/java/com/mongodb/hibernate/jdbc/MongoPreparedStatement.java
Outdated
Show resolved
Hide resolved
# Conflicts: # src/integrationTest/java/com/mongodb/hibernate/query/Book.java # src/integrationTest/java/com/mongodb/hibernate/query/select/SortingSelectQueryIntegrationTests.java
db8489e
to
e1fff5f
Compare
…sertionIntegrationTests
void testReadNestedValuesMissingFields() { | ||
var insertResult = mongoCollection.insertOne( | ||
BsonDocument.parse( | ||
""" | ||
{ | ||
_id: 1, | ||
nested1: {}, | ||
nested1: {a: 0}, |
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.
Though Hibernate ORM v6.6.23 fixed the bug, we still can't make this testing pass without making the above change. The reason is Single
class's a
field is of primitive type, and without hibernate.create_empty_composites.enabled
enabled (deprecated since v7 and we decided not to use it though we are currently relying on v6.6), we could only setting default value to avoid exception.
So here is the dilemma. If a primitive type field is entity field, null or missing field would end up with default value assigned (due to JDBC ResultSet API spec); but if it shows up in a Struct field, exception would be thrown (Embeddable has its own setter logic different from JDBC API). Given the boolean flag has been removed since v7, it seems we can do nothing here.
@@ -18,7 +18,7 @@ assertj = "3.27.3" | |||
google-errorprone-core = "2.36.0" | |||
nullaway = "0.12.4" | |||
jspecify = "1.0.0" | |||
hibernate-orm = "6.6.9.Final" | |||
hibernate-orm = "6.6.23.Final" |
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.
this release will fix the annoying https://hibernate.atlassian.net/browse/HHH-19575 bug.
https://jira.mongodb.org/browse/HIBERNATE-48
The ticket has both tech design part and implementation part:
The main decisions from design are summarized as below:
The main work in this PR includes:
Some tech decisions in this PR:
@DynamicInsert
for two reasons: 1) it was disabled for protective development reason during previous PR; now the reason ceases to exist; 2) we can't avoid mixture of null and missing fields even if we disabled@DynamicInsert
for HQL partial insertion statement will end up with the mixture easily