Page MenuHomePhabricator

🟫 Replace item statement
Closed, ResolvedPublic8 Estimated Story Points

Description

As a tool developer I want to be able to replace the statement data with new structure so that the tool can store non trivial edits

PUT /entities/items/{item_id}/statements/{statement_id}
PUT /statements/{statement_id}

Meant to be as a counterpart of PATCH "endpoint" (T306934), allowing to replace the whole statement structure with new data, e.g. when the client cannot or does not want to calculate the diff/patch. This will only replace existing statements and will not create a new statement if the given statement ID does not exist.

The format of the statement in the request body is the same as for the POST body. The ID will be ignored.

Responses:

  • On the successful replacement of the statement content, API would respond with 200 and response body contains the statement data. Successful response should include ETag and Last-Modified headers with the relevant values
  • If the statement with the request ID does not exist, API would respond with 404 (error code and message as in POST responses)
  • If the subject item of the statement is a redirect, API would also respond with 404 (error code and message as in POST responses)
  • If the request contains id in the statement data, the API should respond with 400 (error code: invalid-operation-change-statement-id, error message: Cannot change the ID of the existing statement) unless the provided ID matches the the statement's ID in the request's path
  • If the request contains property in the statement data, the API should respond with 400 (error code: invalid-operation-change-property-of-statement, error message: Cannot change the property of the existing statement) unless the provided property ID matches the the existing statement's property

Other remarks:

  • If statement data in the request contains fields that are not "valid" parts of the statement data, or are not expected to be specified by the client (except guid and property, see above) those are to be silently ignored, i.e. they are not stored by the API, and the API does not respond with error response to such requests

Authenticating user and checking their permission to make a change (including on a protected item) are out of scope of this story, and will be addressed separately.

All editing REST API endpoints are meant to include a consistent "metadata" optional parameters

  • mediawiki "metadata" can be specified:
    • "comment" to be included in the edit summary (no automated edit summary part). Use the consistent empty "default" comment value if none provided.
    • mediawiki tag(s) for the edit
    • flag edit as made by a bot
  • to help Wikibase solve potential edit conflict ("lost update problem"), the latest revision of an item known by a client when making the request can also be provided using If-Match HTTP header. In case the ETag provided is not the most recent version if the item, the API would response in 412 Precondition Failed response code.
    • As a timestamp-based counterpart, also use If-Unmodified-Since header with a respective logic.
    • 412 response should not include any extra text in response body. The response should also not include ETag and Last-Modified header

For some inspiration might also see https://wmde.github.io/wikibase-rest-api-proposal/#/statements/put_statements__statement_id_ (non binding reference)

Note: The Statement ID should stay the same. The order of statements in the statement group should stay the same.

Event Timeline

Jakob_WMDE set the point value for this task to 8.

Notes from task breakdown:

Sub task: specs - @Silvan_WMDE

Sub task: use case happy path - @Ollie.Shotton_WMDE

  • add a StatementList::replaceStatement( StatementGuid $id, Statement $statement ): void which ignores the Statement object's id and sets it to $id
  • create a new ReplaceItemStatement use case, including ReplaceItemStatementRequest
  • retrieve Item metadata
  • replace the statement
  • use ItemRetriever, replace the statement, then save using ItemUpdater
  • create a ReplaceItemStatementSuccessResponse and return it

Sub task: validate - @Jakob_WMDE

  • validate item id if present, statement id, metadata, and statement serialization

Sub task: item specific statement replacement route handler - @Silvan_WMDE

  • middlewares: auth, unexpected errors, conditional requests

Sub task: short route statement replacement route handler - @Ollie.Shotton_WMDE

  • middlewares: auth, unexpected errors, conditional requests

Sub tasks: error handling for redirects and statement not found - @Jakob_WMDE

Sub tasks: spec tests - @Silvan_WMDE

Question (maybe):

  • We assume the order of statements should be kept when replacing an existing statement. We found some documentation about an edge case where the main snak of the statement changes the property ID, which has some influence on the statement group order (which we don't fully understand). It says "Be aware that when setting an index that specifies a position not next to a statement whose main snak does not feature the same property, the whole group of statements whose main snaks feature the same property is moved." - Is it fine for us (for now?) to just replace the statement in the list and keep the index, ignoring potential side effects?
  • Why is replacing the whole statement *including the Property* even allowed?
Ollie.Shotton_WMDE renamed this task from Replace item statement to 🟫 Replace item statement.Jul 13 2022, 3:12 PM

Should a successful PUT statement response include ETag and Last-Modified headers? This story doesn't say either way, but the Proposal Schema does include them.

9.3.4 PUT of RFC9110 says:

An origin server MUST NOT send a validator field (Section 8.8), such as an ETag or Last-Modified field, in a successful response to PUT unless the request's representation data was saved without any transformation applied to the content (i.e., the resource's new representation data is identical to the content received in the PUT request) and the validator field value reflects the new representation.

Which I think means we should include ETag and Last-Modified headers; BUT should raise an error response (400 Bad Request or 409 Conflict?) if the request body contains a statement ID that is different to the one in the URI. This is slightly different to what we talked about in the task breakdown of silently replacing the statement ID in the request body if it differs from the URI.

9.3.4 PUT of RFC9110 suggests a 409 Conflict response:

An origin server SHOULD verify that the PUT representation is consistent with its configured constraints for the target resource. For example, if an origin server determines a resource's representation metadata based on the URI, then the origin server needs to ensure that the content received in a successful PUT request is consistent with that metadata. When a PUT representation is inconsistent with the target resource, the origin server SHOULD either: make them consistent by transforming the representation or changing the resource configuration, or respond with an appropriate error message containing sufficient information to explain why the representation is unsuitable. The 409 (Conflict) or 415 (Unsupported Media Type) status codes are suggested, with the latter being specific to constraints on Content-Type values.

We assume the order of statements should be kept when replacing an existing statement. We found some documentation about an edge case where the main snak of the statement changes the property ID, which has some influence on the statement group order (which we don't fully understand). It says "Be aware that when setting an index that specifies a position not next to a statement whose main snak does not feature the same property, the whole group of statements whose main snaks feature the same property is moved." - Is it fine for us (for now?) to just replace the statement in the list and keep the index, ignoring potential side effects?

I am not sure I fully understand that remark either yet. What it says does not seem relevant as wbsetclaim seems to disallow changing the property of the statement? But need to do some more clicking, maybe it somehow related to qualifiers etc?
Unless I change my mind I'd think yes, let's just replace the "content" of the statement. The order of statements/claims/snaks does seem like some other/additional concern to be handled separately.

Verified that change the property of an existing statement is correctly prevented. Thanks

Also verified that changing the statement ID is prevented. All good, thanks!

@WMDE-leszek: Assuming this task is resolved. Thus setting resolved task status. Please reopen and add an active project tag if that is not the case. Thanks.