Dynamodbv2 atomic counters #1048
Conversation
…rement step, the save method will do the same, however the incr step won't overwrite the raw values of the field attributes if the object is empty but rather just perform the increment step
…either the save method or the incr method. The intent however is this field in the model can never be directly set but rather by policy the attribute is always added via the annotation @DynamoDBAtomicIncrementor
…n such that it generates the appropriate APPEND_SET behavior
…ies the value, but instead merely uses the value in the object model to perform the update. If the value is null, the counter is not incremented. If the value has a value during a save operation the save must be an append_set operation otherwise it will preclude the stored value.
There was a problem hiding this comment.
This seems like a very specific use-case that I'm not sure we want to introduce as a first-class concept within the SDK. The DynamoDB mapper already has a huge number of annotations to support various corner cases and I'd like to see us simplify the core implementation and allow customers to extend as they need to.
I'll review with the rest of the team, including our core DynamoDB mapper contributors as I am but one voice and others may have different opinions. It may be the case that we let customer demand drive this one.
If we did push ahead with this, are you OK to submit this under apache 2.0 license?
| @@ -124,6 +125,22 @@ protected final DynamoDBMapperConfig mergeConfig(DynamoDBMapperConfig overrides) | |||
| } | |||
|
|
|||
| @Override | |||
| public <T> void incr(T object) { | |||
There was a problem hiding this comment.
typically we'd go with full name here so increment
| * either calling save, or by calling the mapper method incr which only applies those attributes which intend | ||
| * to atomically increment for the specified key value. | ||
| * | ||
| * Created by [email protected] on 3/3/17. |
There was a problem hiding this comment.
All source (including contributions) are licensed under apache 2.0 - we'll have to update the copyright here
| @Retention(RetentionPolicy.RUNTIME) | ||
| @Target({ElementType.FIELD, ElementType.METHOD}) | ||
| public @interface DynamoDBAtomicIncrementor { | ||
| String attributeName() default ""; |
There was a problem hiding this comment.
Feels like the increment amount should be on the annotation (default 1) with the value of the property that's annotated being the "initial value" so for example:
@DynamoDBAtomicIncrementor(attributeName = "counter", increment = 5)
private Integer counter = 10;the counter value starts at 10 and increments by 5 each time.
There was a problem hiding this comment.
I had originally implemented the change this way, but it felt too limiting - I had thought let the POJO dictate the value sent to the service. I can revert if there's a preference push this in that manner, its not material to my use-case
There was a problem hiding this comment.
Either-way it's the pojo dictating it right? What's the difference between it being in the annotation vs in the property?
There was a problem hiding this comment.
@kiiadi the difference is whether the increment value is dynamic. In the case of the annotation the value is hard coded - each time thru when increment or save (using APPEND_SET) is called the value +N is hardcoded to be a constant value. When it's part of the POJO its dynamic and dictated via the client. The intent of this logic is the write operation is always an increment - that a client cannot 'reset' the value thru a PUT operation.
I could see the annotation maintaining the initial value but the POJO maintains the incremental value - on first save we write out the annotation value -- for all future writes we increment using the POJO value.
There was a problem hiding this comment.
If it makes this PR easier to merge I'd be happy to use the annotation to drive the behavior as that satisfies my use-case -- when I thought about the general solution the above came to mind. I'm willing to make this work either way. Cheers --Matt
|
Yes, I'd be ok with the Apache licensing |
|
Sorry for the delay on this one. We won't be able to introduce this as a first-class citizen into 1.11.x of the SDK, but it would be cool if we could make it easier to mix these in via third-party libraries in 2.0.x of the SDK. We'll make sure to consider that when we're working on this story: aws/aws-sdk-java-v2#35 Thanks for your contribution. I'm sorry we weren't able to integrate it as first-class into the SDK. |
Solves for the use-case where the attribute in the domain model is only ever intended to be incremented & not PUT - meaning we want to ensure that the attribute cannot be directly written to in the DB -- instead the the client intends to increment the value via a mapper annotation (@DynamoDBAtomicIncrementor).
Provides the ability to call mapper.save(T) and only increment a field when the DynamoDBMapperConfig setting for save policy is set to APPEND_SET. Also added a incr method to IDynamoDBMapper such that incr(T) with a provided domain model with only its key value performs a low level update with an ADD using the annotation and object's value.
This use-case is possible today by using the APPEND_SET on save, however, it does not prevent someone from squashing the value in the future by directly invoking save with a PUT operation. The new annotation DynamoDBAtomicIncrementor ensures that this attribute only be placed into the update as an ADD irrespective of what the caller is doing. Useful in cases where you want to count the frequency of a change which today necessitates using the update lower level calls.
The increment value is supplied in the model configuration via the following annotation
Each time model is saved the attribute 'counter' is incremented by 5. The existing pattern for accomplishing the same is to use the lower level UpdateItemRequest interface and call updateItem.
The incr behavior enforces the SaveBehavior#APPEND_SET if not called with a DynamoDBMapperConfig.
I've added test cases in the StandardModelFactoriesTest for the correctly generated autoIncrementor model / strategy.