Fix #71: implement $inc update operator
#83
Conversation
tatu-at-datastax
changed the title
$inc update operator$inc update operator
| /** | ||
| * Implementation of {@code $inc} update operation used to modify numeric field values in documents. | ||
| */ | ||
| public class IncOperation extends UpdateOperation { |
There was a problem hiding this comment.
So now we have two types of operations? The ones that are in the the commands, and then operations that execute the queries. Can we find another name to avoid confusion (because I was just like wtf is going on)
There was a problem hiding this comment.
My suggestion is to change the ones extending Operation interface as Executors (Eg: FindOperation -> FindExecutor). The ones in FilterClause and UpdateClause to be left as is. @amorton thoughts?
There was a problem hiding this comment.
I could also change Operation part to Operator to have IncOperator etc (and renaming UpdateOperator enum) if that would help. But I like @maheshrajamani 's suggestion.
src/main/java/io/stargate/sgv2/jsonapi/api/model/command/clause/update/IncOperation.java
Outdated
Show resolved
Hide resolved
...io/stargate/sgv2/jsonapi/service/operation/model/command/clause/update/IncOperationTest.java
Outdated
Show resolved
Hide resolved
| """ | ||
| { "integer" : 1, "fp" : 0.25, "text" : "value" } | ||
| """); |
There was a problem hiding this comment.
It's so hard to understand the test when in every method I need to scroll down here to see what's the test setup.. Would you inline this in every test?
There was a problem hiding this comment.
Hmmh. I guess it's matter of taste really; duplication vs extraction. But I guess for this particular case content is small enough I could inline.
Indentation is due to IDE and mvn fmt:format which is bit odd but hard to control. :-/
There was a problem hiding this comment.
Yea indentation needs some manual work as this formatting model does not support it.. Btw this formatting model is terrible..
src/main/java/io/stargate/sgv2/jsonapi/api/model/command/clause/update/IncOperation.java
Outdated
Show resolved
Hide resolved
...io/stargate/sgv2/jsonapi/service/operation/model/command/clause/update/IncOperationTest.java
Outdated
Show resolved
Hide resolved
src/main/java/io/stargate/sgv2/jsonapi/api/model/command/clause/update/IncOperation.java
Show resolved
Hide resolved
src/main/java/io/stargate/sgv2/jsonapi/api/model/command/clause/update/IncOperation.java
Outdated
Show resolved
Hide resolved
src/main/java/io/stargate/sgv2/jsonapi/api/model/command/clause/update/IncOperation.java
Outdated
Show resolved
Hide resolved
src/main/java/io/stargate/sgv2/jsonapi/api/model/command/clause/update/IncOperation.java
Show resolved
Hide resolved
src/main/java/io/stargate/sgv2/jsonapi/api/model/command/clause/update/IncOperation.java
Show resolved
Hide resolved
| { | ||
| "updateOne": { | ||
| "filter" : {"_id" : "update_doc_inc"}, | ||
| "update" : {"$inc" : {"number": -4 }} |
There was a problem hiding this comment.
Would be great to test $inc on multiple fields, and $inc on a field that doesn't already exist.
Also, would it be difficult to handle dotted paths with $inc? I don't think that's necessary for first release, but worth thinking about.
There was a problem hiding this comment.
@vkarpov15 Unit tests below handle many more cases: Integration test only tests subset.
There was a problem hiding this comment.
Also, would it be difficult to handle dotted paths with
$inc? I don't think that's necessary for first release, but worth thinking about.
Yes, there is separate issue for dotted path support: I hope to support it similar way for all update operations.
And it's definitely high priority.
There was a problem hiding this comment.
Added unit test for trying to $inc an explicit null; caught as expected.
…ust existing), as per code review suggestion
| /** | ||
| * Implementation of {@code $inc} update operation used to modify numeric field values in documents. | ||
| */ | ||
| public class IncOperation extends UpdateOperation { |
There was a problem hiding this comment.
My suggestion is to change the ones extending Operation interface as Executors (Eg: FindOperation -> FindExecutor). The ones in FilterClause and UpdateClause to be left as is. @amorton thoughts?
src/main/java/io/stargate/sgv2/jsonapi/api/model/command/clause/update/IncOperation.java
Show resolved
Hide resolved
Implements
$incupdate operator, adds unit and integration tests for the same (and one IT for "$push" that was missing).