Score:2

Unique constraint on several entity fields fails when >1 jsonapi POST requests happen right after another

cn flag

In a fully decoupled drupal 9 project I have a custom entity type and added a unique constraint for several fields just as described here. This works well and adding a second entity with the same field values is not possible. However, I'm using JSONAPI POST requests to create the entities. I noticed that when issuing multiple POST requests with the exact same field values right after another, the validator method (using the entityTypeManager->getStorage(...)->getQuery(...)->condition(...)->execute() to check the DB) does not return other entities, as no duplicate entity exists yet. I.e. it happens so fast that multiple identical-valued entities are created at the exact same timestamp (The created values of the entities are identical)!

Bypassing the constraint is dangerous and must be prevented.

What can I do to solve this problem?

Update This is the function that is called inside the ConstraintValidator

public function validate($entity, Constraint $constraint)
{
  ...
  if (!$this->isUnique($entity))
    $this->context->addViolation($constraint->notUnique);
  ...
}
private function isUnique(CustomType $entity) {
  $date = $entity->get('date')->value;
  $type = $entity->bundle();
  $employee = $entity->get('employee')->target_id;
  $query = $this->entityTypeManager->getStorage('custom_type')->getQuery()
    ->condition('status', 1)
    ->condition('type', $type)
    ->condition('employee', $employee)
    ->condition('date', $date);

  if (!is_null($entity->id()))
    $query->condition('id', $entity->id(), '<>');

  $workIds = $query->execute();
  return empty($workIds);
}

I'm happy to find any flaws. So far this code works well in all other cases.

Update Drupal::lock()

I've implemented 2 event subscribers to add and release \Drupal::lock() as mentioned in the comments. Using xdebug I can confirm that the code is run, however, the lock does not seem to have any effect. The documentation for lock() is rather limited. Not sure what's wrong here.

<?php

namespace Drupal\custom_entities\EventSubscriber;

use Symfony\Component\EventDispatcher\EventSubscriberInterface;
use Symfony\Component\HttpKernel\Event\RequestEvent;
use Symfony\Component\HttpKernel\KernelEvents;

class JsonApiRequestDBLock implements EventSubscriberInterface {

  /**
   * Adds a lock for JSON:API requests.
   *
   * @param \Symfony\Component\HttpKernel\Event\RequestEvent $event
   *   The event to process.
   */
  public function onRequest(RequestEvent $event) {
    $request = $event->getRequest();
    if ($request->getRequestFormat() !== 'api_json') {
      return;
    }

    if ($request->attributes->get('_route') === 'jsonapi.custom_type--work.collection.post' &&
      $request->attributes->get('_controller') === 'jsonapi.entity_resource:createIndividual'
    ) {
      $lock = \Drupal::lock();
      $lock->acquire('custom_create_lock');
    }

  }

  /**
   * {@inheritdoc}
   */
  public static function getSubscribedEvents() {
    $events[KernelEvents::REQUEST][] = ['onRequest'];
    return $events;
  }

}

and release the lock after the response

<?php

namespace Drupal\custom_entities\EventSubscriber;

use Symfony\Component\EventDispatcher\EventSubscriberInterface;
use Symfony\Component\HttpKernel\Event\ResponseEvent;
use Symfony\Component\HttpKernel\KernelEvents;

class JsonApiResponseDBRelease implements EventSubscriberInterface {

  /**
   * {@inheritdoc}
   */
  public static function getSubscribedEvents() {
    $events[KernelEvents::RESPONSE][] = ['onResponse'];
    return $events;
  }


  /**
   * Release JSON:API responses.
   *
   * @param \Symfony\Component\HttpKernel\Event\ResponseEvent $event
   *   The event to process.
   */
  public function onResponse(ResponseEvent $event) {
    $response = $event->getResponse();
    if (strpos($response->headers->get('Content-Type'), 'application/vnd.api+json') === FALSE) {
      return;
    }
    $request = $event->getRequest();
    if ($request->attributes->get('_route') === 'jsonapi.custom_type--work.collection.post' &&
      $request->attributes->get('_controller') === 'jsonapi.entity_resource:createIndividual'
    ) {
      // Release the lock.
      $lock = \Drupal::lock();
      if (!$lock->lockMayBeAvailable('custom_create_lock'))
        $lock->release('custom_create_lock');
    }
  }

}

This was added to the services.yml

  # Event subscribers.
  custom_entities.jsonapi_db_lock.subscriber:
    class: Drupal\custom_entities\EventSubscriber\JsonApiRequestDBLock
    tags:
      - { name: event_subscriber }
  custom_entities.jsonapi_response_db_release.subscriber:
    class: Drupal\custom_entities\EventSubscriber\JsonApiResponseDBRelease
    tags:
      - { name: event_subscriber }
Jaypan avatar
de flag
That doesn't seem like it should happen, as it would be almost impossible to save two entities at the exact same time, one has to happen before the other, so the second should get caught by the constraint. Are you sure your constraint code is correct?
cn flag
I do agree and I'm rather confused at the moment. I added a `\Drupal::logger` statement inside the `isUnique()`, logging the current time. It gets called multiple times at the exact same second
apaderno avatar
us flag
The `$this->entityTypeManager->getStorage('custom_type')->getQuery()` line is missing a call to `accessCheck(FALSE)`, which is necessary for the query to ignore any access permission the logged-in user may have for the queried entities.
4uk4 avatar
cn flag
I don't think this is the issue here. It's about concurrent requests. As @Jaypan noted it's very unlikely that the same field data is submitted twice within a second or two. But if this can happen you need some kind of locking mechanism. It's probably not possible to make a database lock on the tables involved, so you need an independent locking mechanism like https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Lock%21LockBackendInterface.php/group/lock
cn flag
@4k4 I agree, it's very unlikely that the same field data is submitted twice and it never happened to me before. However, it can happen. I've thought about DB locking too and thanks to your comment I'll check the locking mechanisms link
4uk4 avatar
cn flag
You need to apply the lock on the entire request, not only the constraint. Either by overriding the controller or by using event subscribers, KernelEvents::REQUEST to acquire the lock, KernelEvents::RESPONSE to release the lock and KernelEvents::EXCEPTION to handle exceptions raised because of the lock.
cn flag
@4k4 I've updated the question showing my attempt to work with locks. Does not seem to have an effect. I might still be missing smth
4uk4 avatar
cn flag
Your locking is not doing anything. If you fail to acquire the lock you have to raise an exception. Optionally you can put in a wait loop before you do this.
Score:2
us flag

Without seeing all the code used for the custom_type entity, including the code for its handlers, and answering to why the code doesn't find duplicates, there are two "flaws" I can imagine possible in the shown code.

The first "flaw" is that the query searching for existing entities is more restrictive than it should. This means it checks for entity fields that should be irrelevant for the entities to be duplicates, for example:

  • The status field, assuming it's the status field used by the Drupal core entities
  • The date field, assuming it contains the creation date/timestamp

The only Drupal core entity that uses an entity validation to avoid duplicate entities are created is the path_alias entity, implemented by the PathAlias class. That entity has a status field, it supports revisions, but it doesn't have a field to store when it has been created, nor does it have an owner entity (like nodes do).
UniquePathAliasConstraintValidator is its entity constraint validator; the code for UniquePathAliasConstraintValidator::validate() is the following one.

  $path = $entity->getPath();
  $alias = $entity->getAlias();
  $langcode = $entity->language()->getId();
  $storage = $this->entityTypeManager->getStorage('path_alias');
  $query = $storage->getQuery()
    ->accessCheck(FALSE)
    ->condition('alias', $alias, '=')
    ->condition('langcode', $langcode, '=');
  if (!$entity->isNew()) {
    $query->condition('id', $entity->id(), '<>');
  }
  if ($path) {
    $query->condition('path', $path, '<>');
  }
  if ($result = $query->range(0, 1)->execute()) {
    $existing_alias_id = reset($result);
    $existing_alias = $storage->load($existing_alias_id);
    if ($existing_alias->getAlias() !== $alias) {
      $this->context->buildViolation($constraint->differentCapitalizationMessage, ['%alias' => $alias, '%stored_alias' => $existing_alias->getAlias()])
        ->addViolation();
    }
    else {
      $this->context->buildViolation($constraint->message, ['%alias' => $alias])
        ->addViolation();
    }
  }
}

Using that code as example, and using only the code that strictly checks for duplicates, in your case I would use the following code. (I am showing only the code for isUnique().)

private function isUnique(CustomType $entity) {
  $date = $entity->get('date')->value;
  $type = $entity->bundle();
  $employee = $entity->get('employee')->target_id;
  $query = $this->entityTypeManager->getStorage('custom_type')
    ->getQuery()
    ->accessCheck(FALSE)
    ->condition('type', $type)
    ->condition('employee', $employee)
    ->condition('date', $date);

  if (!$entity->isNew()) {
    $query->condition('id', $entity->id(), '<>');
  }

  $result = $query->range(0, 1)->execute();
  return empty($result);
}

I added the call to accessCheck(FALSE) because, without seeing the code used by the entity access handler, and without knowing if the entity has an owner entity, I cannot exclude the implementation of isUnique() isn't finding duplicates because the currently logged-in user doesn't have access to the duplicates (or any entity of that type). Without calling accessCheck(FALSE), the query will return the entities to which the currently logged-in user has access.
(The missing call to accessCheck(FALSE) is the other, possible, "flaw" I can see in the shown code.)

cn flag
Thanks! I've tried this code, played around and tested many times. However, the actual issue remains, with 2 concurrent requests my validator does not prevent the duplicated entities. I'm digging deeper and keep this issue up to date.
apaderno avatar
us flag
Is the *date* field used by your code like the *created* field used for nodes? If that is the case, I would not search for existing entities with the same value for *date*.
cn flag
not quite sure what exactly you mean but the date is highly important for the uniqueness check
apaderno avatar
us flag
I mean that if that field contains when the entity has been created, I would not include it in the query to find duplicates. Clearly, if the entity is for events, for example, and *date* is the event scheduled date, then I would include it.
mangohost

Post an answer

Most people don’t grasp that asking a lot of questions unlocks learning and improves interpersonal bonding. In Alison’s studies, for example, though people could accurately recall how many questions had been asked in their conversations, they didn’t intuit the link between questions and liking. Across four studies, in which participants were engaged in conversations themselves or read transcripts of others’ conversations, people tended not to realize that question asking would influence—or had influenced—the level of amity between the conversationalists.