Proposal: Spec update - replacePin should support "update" like operations

[SgtPooki]

Please read ipfs-shipyard/pinning-service-compliance#124 and web3-storage/web3.storage#1547 for more details.

[lidel]

Yes, I believe the intention behind replace operation was to cover atomic "update" operations, including ones with the same CID.

User should be able to replace an existing pin with one that pins the same CID, but has new/modified Pin.name, Pin.origins (to unblock stuck pinning job) or Pin.meta (to add/modify metadata attributes).

If this is unclear from the spec, then we should update the description of this method.

[SgtPooki]

If this is unclear from the spec, then we should update the description of this method.

I think a one part in the spec makes this somewhat unclear:

Replace an existing pin object (shortcut for executing remove and add operations in one step to avoid unnecessary garbage collection of blocks present in both recursive pins)
-- https://ipfs.github.io/pinning-services-api-spec/#tag/pins/paths/~1pins~1{requestid}/post

As I mentioned in pinning-service-compliance#124, I interpret this exactly as a replacement operation. Especially because the CID is required. If the method were described as update instead, with CID being optional, then it would make sense that anything/everything could be updated/changed.

maybe that's just me. I would love to hear @2color and @en0ma's interpretations and feedback.

[SgtPooki]

P.S. Replace should definitely delete/remove any data that's not provided during a replace call, but for update, I feel like it should only modify provided data. There are significant connotations that come with calling something an update operation, and replace does not come with those exact meanings.

Connotations that come with update/replace operations

That I know of, and your experience may vary, but I thought this could help the conversation:

Update

1. Do not corrupt/change/modify existing data that were not specified to update.
   • Didn't supply Meta field? provider should leave it alone.
2. Only require uniquely identifying properties for update operations.
   • Requiring anything additional is unnecessary and can result in accidental updates that modify/corrupt data unintentionally.
3. Merging may be possible
   • Highly varies by services/products, but update operations for objects frequently include merge support. (unless you want to write add/edit/delete APIs for each object that exists across your ecosystem).

Replace

1. The item being replaced will be thought of as "garbage" for either deletion/cleanup after a successful replacement.
2. The item being provided (replacer/new item) should match the same type as the replaced object, but there are zero other expectations on it's shape.
3. There is a unique identifier that acts as a pointer to an object, and a replacement operation is simply altering the location being pointed to.
   • This should be required as well as any required properties for the update operation's type

[en0ma]

If this is unclear from the spec, then we should update the description of this method.

I think a one part in the spec makes this somewhat unclear:

Replace an existing pin object (shortcut for executing remove and add operations in one step to avoid unnecessary garbage collection of blocks present in both recursive pins)
-- https://ipfs.github.io/pinning-services-api-spec/#tag/pins/paths/1pins1{requestid}/post

As I mentioned in pinning-service-compliance#124, I interpret this exactly as a replacement operation. Especially because the CID is required. If the method were described as update instead, with CID being optional, then it would make sense that anything/everything could be updated/changed.

maybe that's just me. I would love to hear @2color and @en0ma's interpretations and feedback.

My interpretation is the same as this comment from the spec;

Replace an existing pin object (shortcut for executing remove and add operations in one step to avoid unnecessary garbage collection of blocks present in both recursive pins)

2 operations (delete and add) via 1 request

[flea89]

Hey, I'm looking at the web3-storage/web3.storage#1547 and I had the same doubts raised by @SgtPooki here.

We interpreted the spec to be a replace operation rather than an update.
To make it obvious we are also returning an error if the same CID is provided.

From the discussion in ipfs-shipyard/pinning-service-compliance#124, it looks like we reached a consensus it should support update operations as well.
I'm going to update our implementation, please shout if that shouldn't be the case.

On top of what already discussed, and the fact the endpoint is meant to do updates (on top of replace), it'd make more sense to update the same psa_pin_request (using the same requestid) rather than create and return a new instance.
This isn't the case atm in web3.storage (we create a new psa_pin_request while deleting the old one), but would you agree that'd be a better approach?
@lidel @SgtPooki keen to hear your thoughts!

[SgtPooki]

Looking at this now but not in the right headspace. Will have to come back to this one

[SgtPooki]

Just gave things another read, but on mobile and just jotting down some thoughts.

User should be able to replace an existing pin with one that pins the same CID, but has new/modified Pin.name, Pin.origins (to unblock stuck pinning job) or Pin.meta (to add/modify metadata attributes).

Pin === requestid, so CID should be able to be the same.

scenario 1 - same CID

Require one of name|origins|meta. If none of those are different, we could fail “No_changes_required”, or silently succeed, or always loop over fields.

For each field, update the pin’s relevant field with the new value.

scenario 2 - different CID

If CID is not the same, inputs should be a partial of those required by “addPin” and in addition, requestid is also required.

This scenario seems to be the original intent behind replace; allowing the service to combine the deletion of the old CID, with the pinning of different content, while allowing consumers to keep the same requestid.

[SgtPooki]

I think CID should not be required by replace, but should be allowed to be the same as already contained CID.

If a requestid already exists(PinStatus), it must already have a CID. If it does, and I want the same CID and just to replace other data in the Pin, then the service already knows the CID.

If i pass a CID, and it’s the same, it shouldn't matter. I should be able to pin the same CID with different name/metadata (different origins shouldnt matter for successfully pinned content, right?).

Edgecases

If i try to pin the exact same name, CID, and metadata, under different requestid’s, that should be blocked with a “already have that at x requestid.” or does this have some particular benefit i’m not aware of?

What are the minimal viable differences required for a separate Pin to be relevant? CID+name probably, right?

I feel like an error should only be thrown when the same CID is passed if the replacement has no increase in value to the existing pin, but I don’t think the spec calls that out And as long as the same CID is allowed to be added or replaced multiple times, the spec is satisfied.

However, i think the spec should grant providers some lenience in setting up their customers for success; i.e. Erroring out in cases where the customer might be doing something unnecessary.