-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
MongoDB Update : Allow all update operators #14989
MongoDB Update : Allow all update operators #14989
Conversation
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 don't see a test that actually executes the updated update statement. one thing i'm trying to remember from my morphia work is the nuance around $set
. iirc, with $set
it will do a full document replacement in the database rather than updating just the fields listed but i can't recall exactly. I would like to see a test that 1) actually executes the updates in the database and 2) makes sure we're not replacing whole documents when we're not intending to do so.
...ntime/src/main/java/io/quarkus/mongodb/panache/reactive/runtime/ReactiveMongoOperations.java
Outdated
Show resolved
Hide resolved
...ntime/src/main/java/io/quarkus/mongodb/panache/reactive/runtime/ReactiveMongoOperations.java
Outdated
Show resolved
Hide resolved
de5821e
to
fc84da9
Compare
I didn't add a new one as we already have one inside the integration test: https://github.com/quarkusio/quarkus/blob/master/integration-tests/mongodb-panache/src/main/java/io/quarkus/it/mongodb/panache/test/TestResource.java#L132 but I agree we can add more tests.
$set is a partial update, from the documentation: The $set operator replaces the value of a field with the specified value. Again, I agree we should have more test, but testing the behaviour of $set is something the MongoDB team should do, what we need to do is assert that we generates the right query. That's why I only added tests on the MongoOperationsTest. |
Agreed about not testing As for testing the rest, I wanted to make sure the doc shape is correct but if there's an integration test that executes this code branch, that's great. but I'm not seeing one that exercises the new code. The test you link seems to only update fields and not update operators. Am I missing that? That code's pretty dense but I didn't see any $ operators when I scanned it. |
Yes, it updates fields without specifying any operator, so the $set operator will be added.
I only add tests to our update document binding test case, I'll add more tests on the integration test including some $ operators. |
fc84da9
to
c0ded6b
Compare
@evanchooly I added tests inside the integration test with both $set and $inc. |
Fixes #9956