🚀[RFC] - Allow conditional request for PUT and PATCH to avoid lost update problem

[GregoireHebert]

Description
This could help avoid the Lost Update Problem.

Conditional requests are useful as a way of avoiding data loss when using unsafe HTTP methods like PUT and PATCH.
Suppose Greg and Jo are using different API clients to edit a common grocery list.

Greg start by making an HTTP requests:

    GET /groceries/{id} HTTP/1.1
    Host: www.example.com

And Both have now an identical representation:

    HTTP/1.1 200 OK
    Content-Type: text/plain
    ETag: "7359b7-a37c-45b333d7"
    Last-Modified: Mon, 27 May 2020 07:57:10 GMT
    {
        "ingredients": [
            "Guinness",
            "Sausage rolls",
            "Cod",
        ]
    }

Jo adds an item to the list and PUTs back the new representation:

    PUT /groceries/{id} HTTP/1.1
    Host: www.example.com
    Content-Type: text/plain
    {
        "ingredients": [
            "Guinness",
            "Sausage rolls",
            "Cod",
            "Eggs"
        ]
    }

She gets a response of 200 (OK).
Greg, unaware of what Jo is doing, adds items to the list and PUTs back his new representation:

    PUT /groceries/{id} HTTP/1.1
    Host: www.example.com
    Content-Type: text/plain
    {
        "ingredients": [
            "Guinness",
            "Sausage rolls",
            "Cod",
            "Flour",
            "Potatoes"
        ]
    }

Greg also gets a response of 200 (OK).
But Jo's version of the list—the version that included "Eggs" has been lost. Greg never even knew about that version.

This sort of tragedy can be avoided by making unsafe requests conditional. With conditional GET, we wanted the request to go through only if the representation had changed. Here, Greg wants his PUT request to go through only if the representation has not changed. The technique is the same, but the conditional is reversed. Instead of If-Match, the client uses the opposite header, If-None-Match. Instead of If-Modified-Since, the client uses If-Unmodified-Since.

Suppose Greg had made his PUT request conditional:

    PUT /groceries/{id} HTTP/1.1
    Host: www.example.com
    Content-Type: text/plain
    If-Match: "7359b7-a37c-45b333d7"
    If-Unmodified-Since: Mon, 27 May 2020 07:57:10 GMT
    {
        "ingredients": [
            "Guinness",
            "Sausage rolls",
            "Cod",
            "Flour",
            "Potatoes",
        ]
    }

Instead of 200 (OK), the server would have sent the status code 412 (Precondition Failed). Greg's client would then know that someone else had modified the grocery list. Instead of overwriting the current representation, Greg's client could send a GET request for the new representation, and try to merge it with Greg's version. Or it could escalate the issue and ask Greg to deal with it himself. It depends on the media type and the application.
In my opinion, your API implementations should require clients to make conditional PUT and PATCH requests. If a client tries to make an unconditional PUT or PATCH, you should send the status code 428 (Precondition Required).

Proposal

Add a new boolean prevent_lost_update_problem (this name ^^') item operation attribute opt-out by default available for PUT and PATCH operation.
Right after reading, from persistence layer, apply the checks.

<?php

declare(strict_types=1);

namespace App\EventSubscriber;

use ApiPlatform\Core\EventListener\EventPriorities;
use ApiPlatform\Core\Metadata\Resource\Factory\ResourceMetadataFactoryInterface;
use Symfony\Component\EventDispatcher\EventSubscriberInterface;
use Symfony\Component\HttpKernel\Event\RequestEvent;
use Symfony\Component\HttpKernel\Exception\PreconditionFailedHttpException;
use Symfony\Component\HttpKernel\Exception\PreconditionRequiredHttpException;
use Symfony\Component\HttpKernel\KernelEvents;

final class IsUnmodifiedResourceSubscriber implements EventSubscriberInterface
{
    private $resourceMetadataFactory;

    public function __construct(ResourceMetadataFactoryInterface $resourceMetadataFactory)
    {
        $this->resourceMetadataFactory = $resourceMetadataFactory;
    }

    public static function getSubscribedEvents()
    {
        return [
            KernelEvents::REQUEST => ['isUnmodifiedResource', EventPriorities::POST_READ]
        ];
    }

    public function isUnmodifiedResource(RequestEvent $event)
    {
        $request = $event->getRequest();

        if (!in_array($request->getMethod(), ['PATCH', 'PUT'])) {
            return;
        }

        $class = $request->attributes->get('_api_resource_class');
        $operationName = $request->attributes->get('_api_item_operation_name');
        $resource = $request->get('data');

        // @todo for PUT operation check that data has been merged from persistence layer, to avoid preventing resource creation. Any idea on how to achieve that cleanly would be great. Has it been already implemented for PUT operation?

        $metadata = $this->resourceMetadataFactory->create($class);

        $unmodifiedSince = $request->headers->get('If-Unmodified-Since');
        $noneMatch = $request->headers->get('If-None-Match');

        if (
            null === ($unmodifiedSince ?? $noneMatch ?? null) &&
            $metadata->getItemOperationAttribute($operationName, 'prevent_lost_update_problem', false)
        ) {
            throw new PreconditionRequiredHttpException('The request MUST provide either the `If-Unmodified-Since` or `If-None-Match` to fulfill the precondition.');
        }

        

        if (
            (null !== $unmodifiedSince && true /*check last modified*/) ||
            (null !== $noneMatch && true /*check last modified*/)
        ) {
            throw new PreconditionFailedHttpException('Your resource is out of sync. Please update.');
			// or 
			throw new ConflictHttpException('Your resource is out of sync. Please update.');
        }
    }
}

[dunglas]

Note that if you are using Doctrine, if a locking exception occurs, we already return the appropriate status code: https://www.doctrine-project.org/projects/doctrine-orm/en/2.7/reference/transactions-and-concurrency.html

[GregoireHebert]

yes :) I added the ConflictHttpException in the example.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.