Skip to content
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

basic update translation #59

Conversation

NathanQingyangXu
Copy link
Contributor

@NathanQingyangXu NathanQingyangXu commented Mar 2, 2025

@NathanQingyangXu NathanQingyangXu force-pushed the HIBERNATE-19-basic-update-translation branch from 607c442 to 6bb7138 Compare March 2, 2025 17:52
{"update": "books", "updates": [{"q": {"_id": {"$eq": {"$undefined": true}}}, "u": {"$set": {"author": {"$undefined": true}, "publishYear": {"$undefined": true}, "title": {"$undefined": true}}}, "multi": true}]}\
""");
}
}
Copy link
Contributor Author

@NathanQingyangXu NathanQingyangXu Mar 2, 2025

Choose a reason for hiding this comment

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

to prove that the udpating did happen (not one sinlge insertion with latest state), a good feature of hibernate-testing was used here to verify. Note that by default table mutation static update SQL will include all fields for perf reason (db side redundant updating is relatively cheap than dynamic SQL translating)

Copy link
Member

Choose a reason for hiding this comment

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

I tried annotating Book with @DynamicUpdate, and it just worked, barring the statementInspector.getSqlQueries assertion, of course. Should we add a test for that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point. I think we should cover this.

There is also a @DynamicInsert, which won't include fields with null values, so it will align with the null-free insertion case we discussed. I might add it later but not in this PR.

@@ -170,6 +172,46 @@ void testSimpleDeletion() {
}
}

@Nested
class UpdateTests {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Nested is used to accommodate the future new testing case after optimistic locking feature is taken into consideration, even though currently there is only one testing case in this PR.

@jyemin jyemin removed their request for review March 3, 2025 16:19
# Conflicts:
#	src/integrationTest/java/com/mongodb/hibernate/BasicCrudIntegrationTests.java
{"update": "books", "updates": [{"q": {"_id": {"$eq": {"$undefined": true}}}, "u": {"$set": {"author": {"$undefined": true}, "publishYear": {"$undefined": true}, "title": {"$undefined": true}}}, "multi": true}]}\
""");
}
}
Copy link
Member

Choose a reason for hiding this comment

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

I tried annotating Book with @DynamicUpdate, and it just worked, barring the statementInspector.getSqlQueries assertion, of course. Should we add a test for that?

@stIncMale
Copy link
Member

The last reviewed commit is e988b33.

@katcharov katcharov removed their request for review March 7, 2025 16:11
@katcharov
Copy link
Contributor

I will take myself off as a formal reviewer for now - if a secondary is needed, please do add me once the primary is complete.

@NathanQingyangXu
Copy link
Contributor Author

The last reviewed commit is e988b33.

I've finished resolving the comments, so the PR is ready for a new book.

Copy link
Contributor Author

@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.

Finished the first round of reivew comments resolving

Copy link
Member

@stIncMale stIncMale left a comment

Choose a reason for hiding this comment

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

The last reviewed commit is 90d7876.

Copy link
Member

@stIncMale stIncMale left a comment

Choose a reason for hiding this comment

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

The last reviewed commit is 3c4e08f.

Copy link
Member

@stIncMale stIncMale left a comment

Choose a reason for hiding this comment

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

The last reviewed commit is 8803d30.

@NathanQingyangXu NathanQingyangXu merged commit ccca670 into mongodb:main Mar 11, 2025
6 checks passed
@NathanQingyangXu NathanQingyangXu deleted the HIBERNATE-19-basic-update-translation branch March 11, 2025 16:26
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.

3 participants