bug: updating an item without getting it first resets it's version to 1

[bpsoos]

The Issue

Given a model with a version attribute:

    class TestModel(Model):
        """
        A model for testing
        """
        class Meta:
            region = 'us-east-1'
            table_name = 'pynamodb-ci'
            host = ddb_url
        forum = UnicodeAttribute(hash_key=True)
        score = NumberAttribute()
        version = VersionAttribute()

a simple update on the model without getting it first resets it's version to 1:

    obj = TestModel('1')
    obj.save()
    assert TestModel.get('1').version == 1
    obj.score = 1 
    obj.save()
    assert TestModel.get('1').version == 2

    obj_by_key = TestModel('1')  # try to update item without getting it first
    obj_by_key.update(
        actions=[
            TestModel.score.set(2),  # no version increment
        ],
        add_version_condition=False
    )
    updated_obj = TestModel.get('1')
    assert updated_obj.score == 2
    assert updated_obj.version == 2  # AssertionError: assert 1 == 2

manual version update also fails with "Two document paths overlap with each other" error as the update method automatically adds a SET operation for the version and they conflict:

    obj_2 = TestModel('2')
    obj_2.save()
    assert TestModel.get('2').version == 1
    obj_2.score = 1 
    obj_2.save()
    assert TestModel.get('2').version == 2

    obj_2_by_key = TestModel('2')  # try to update item without getting it first
    obj_2_by_key.update(
        actions=[
            TestModel.score.set(2),
            TestModel.version.set(TestModel.version + 1)  # increment version manually
        ],
        add_version_condition=False
    )  # pynamodb.exceptions.UpdateError: Failed to update item: An error occurred (ValidationException) on request (c46f93e9-5ffd-4c08-8b3a-59926cd2d8a8) on table (pynamodb-ci) when calling the UpdateItem operation: Invalid UpdateExpression: Two document paths overlap with each other; must remove or rewrite one of these paths; path one: [version], path two: [version]
    
    updated_obj_2 = TestModel.get('2')
    assert updated_obj_2.score == 2
    assert updated_obj_2.version == 3

The same issues apply to updates in transactions.
See the integration tests in the linked pull request for runnable versions of these examples.

Root cause

The root cause seems to be in Model._handle_version_attribute, which automatically tries to increment the models version and sets it to 1 if getattr(self, self._version_attribute_name) returns None.

Solution proposal

I propose to add an increment_version flag to the relevant Model methods, similar to add_version_condition.
For example:

    def update(
        self,
        actions: List[Action],
        condition: Optional[Condition] = None,
        *,
        add_version_condition: bool = True,
        increment_version: bool = True,
        ) -> Any:
            ...
            version_condition = self._handle_version_attribute(actions=actions, increment_version=increment_version)
            ...
        
    def _handle_version_attribute(
        self,
        *,
        attributes: Optional[Dict[str, Any]] = None,
        actions: Optional[List[Action]] = None,
        increment_version: bool = True,
    ) -> Optional[Condition]:
        """
        Handles modifying the request to set or increment the version attribute.
        """
        if self._version_attribute_name is None:
            return None

        version_attribute = self.get_attributes()[self._version_attribute_name]
        value = getattr(self, self._version_attribute_name)

        if value is not None:
            condition = version_attribute == value
            if not increment_version:
                return condition
            if attributes is not None:
                attributes[version_attribute.attr_name] = self._serialize_value(version_attribute, value + 1)
            if actions is not None:
                actions.append(version_attribute.add(1))
        else:
            condition = version_attribute.does_not_exist()
            if not increment_version:
                return condition
            if attributes is not None:
                attributes[version_attribute.attr_name] = self._serialize_value(version_attribute, 1)
            if actions is not None:
                actions.append(version_attribute.set(1))

        return condition

[half2me]

You shouldn't need to run a GetItem operation before running an UpdateItem.
If the existing version of record is say 6, we shouldn't write the update operation as SET version = 7 but rather SET version = version + 1 That way the increment operation doesn't need to know the previous value. This would also work for the case where there is no version attribute set yet.

Example: dynamodb lets you do this:

UpdateExpression: "SET version = if_not_exists(version, :initial) + :inc",
ExpressionAttributeValues:
  ":inc": 1,
  ":initial": 0,
},