../assets/{asset_id}/ PUT: clarify that new asset is created

[yarikoptic]

"Update" (or "change", both words are used, see blow) IMHO implies in-place operations which is not the case here: if metadata is different, we get a new asset returned, hence there is really no "update" to an existing asset, but rather CoW style of operation on an asset upon change.

To be frank: there is a notion of "update" as the new created asset is assigned to the same dandiset, hence it is replacing previous asset present in the dandiset. But as we are talking about a new asset object with new asset id, I think it is important to get the only visible API documentation "straight".

• Decide on either RF (update|change)_asset into e.g. create_updated_asset (since not always creates one). ATM we have

❯ git grep '\(change\|update\)_asset\>' -- dandiapi | grep -v test_
dandiapi/api/migrations/0010_auditrecord.py:                            ('update_asset', 'update_asset'),
dandiapi/api/models/audit.py:    'update_asset',
dandiapi/api/services/asset/__init__.py:def change_asset(  # noqa: PLR0913
dandiapi/api/services/asset/__init__.py:        audit.update_asset(dandiset=version.dandiset, user=user, asset=new_asset)
dandiapi/api/services/audit/__init__.py:def update_asset(*, dandiset: Dandiset, user: User, asset: Asset) -> AuditRecord:
dandiapi/api/services/audit/__init__.py:        dandiset=dandiset, user=user, record_type='update_asset', details=details
dandiapi/api/views/asset.py:    change_asset,
dandiapi/api/views/asset.py:            asset, _ = change_asset(

WDYT @waxlamp @jjnesbitt ?

[waxlamp]

Thanks, @yarikoptic.

I'm against this change, for two reasons:

1. It is leaking implementation details up to the interface level. We selected CoW semantics for the PUT operation out of a desire to keep some notion of history of changes around (which has now been replaced by the audit record feature), but to the end user it was always intended to fulfill a "replace/change asset" operation (hence our use of the PUT verb). Therefore, describing the operation as creating a new asset is confusing at best and misleading at worst (reflecting what you wrote about there being a notion of update).
2. In a recent meeting (about Zarr versioning, I believe) we discussed the idea that the CoW semantics for assets may no longer be needed (specifically, we could not definitively remember why we selected those semantics in the first place, and we did not identify any reason it is needed now, in light of audit being deployed). I have a plan (not yet designed) to replace CoW with in-place updates (only for assets in draft version). We need to have a full discussion about this, but it has several advantages (one of which is eliminating the confusion over why the asset ID is changing under a PUT operation).

Basically, I would like to keep the existing docstrings because they cause less confusion, and because I have a vision for how to simplify our "update" operation.

If you're ok with this reasoning, I can close the PR.

[yarikoptic]

1 ... it was always intended ...

;-) are you sure about "always" and what was the intention?

COW operation is what was intended in our design and was implemented. Documentation here is simply factually wrong since metadata of an existing asset with the original asset_id is not updated! It is not just a matter of intents, but rather description of what it does and an effect of the operation. As a result it is actually misleading user.

2 ... we discussed the idea that the CoW semantics for assets may no longer be needed

it may or may not -- yet to be checked and also redesigned/reimplemented, and docstrings to be adjusted accordingly. Not a factor IMHO for documentation describing current behavior.

[waxlamp]

1 ... it was always intended ...

;-) are you sure about "always" and what was the intention?

COW operation is what was intended in our design and was implemented. Documentation here is simply factually wrong since metadata of an existing asset with the original asset_id is not updated! It is not just a matter of intents, but rather description of what it does and an effect of the operation. As a result it is actually misleading user.

Since we implemented this behavior under the PUT endpoint, yes, I stand by the idea that the intention to the end user is for an "update asset" operation. You are right that we selected CoW semantics for the implementation, and at this point I consider that to be one of the "quirks" of our API.

2 ... we discussed the idea that the CoW semantics for assets may no longer be needed

it may or may not -- yet to be checked and also redesigned/reimplemented, and docstrings to be adjusted accordingly. Not a factor IMHO for documentation describing current behavior.

Let's have the discussion about this. If there is no longer a need for CoW (which I believe is the case) then keeping it around only has disadvantages without any upside. If we conclude that this is the case, we should modify how the API works.

For the moment, you've convinced me that we should update these docstrings. We'll just have to update them back if we do change the API's behavior to be more consistent with the RESTful concept of a PUT.

[dandibot]

🚀 PR was released in v0.3.104 🚀