Open SgtPooki opened 2 years ago
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.
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.
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.
That I know of, and your experience may vary, but I thought this could help the conversation:
type
as the replaced object, but there are zero other expectations on it's shape.type
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.
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
Hey, I'm looking at the https://github.com/web3-storage/web3.storage/issues/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 https://github.com/ipfs-shipyard/pinning-service-compliance/issues/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!
Looking at this now but not in the right headspace. Will have to come back to this one
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.
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.
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.
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?).
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.
Please read https://github.com/ipfs-shipyard/pinning-service-compliance/issues/124 and https://github.com/web3-storage/web3.storage/issues/1547 for more details.