Add storage exception that enforces unique content_entity_type_id and content_entity_id on the content moderation state content entity, and add access control handler to forbid all access

I think this is a case of an issue that really doesn't need a test. The UniqueField constraint is already tested, so this is overkill and could be tested by simply asserting the field definition has that constraint applied, but maybe it can be the start of tests pertaining to any other constraints that are introduced.

Seems to do the trick.

Really good catch. Ran into a similar issue in the views integration ticket where moderation two entity types at the same time (which the tests are light on) caused a conflict.

Attached is a fresh approach which would address this properly and resolve the @todo.

Coding standards failing the bot is cool.

Making it a count query, because we don't actually need the results.

Wrong KTB.

1. +++ b/core/modules/content_moderation/src/Plugin/Validation/Constraint/UniqueContentModerationStateConstraintValidator.php
   @@ -0,0 +1,38 @@
   +      $query->condition('id', $entity->id(), '<>');
   

   Does it matter if it has the same ID? isn't this validated somewhere else? don't we just care about the content_entity_type_id, content_entity_id, and content_entity_revision_id in this issue?

2. +++ b/core/modules/content_moderation/src/Plugin/Validation/Constraint/UniqueContentModerationStateConstraintValidator.php
   @@ -0,0 +1,38 @@
   +      $query->orConditionGroup()
   

   Nice! I hadn't considered the possibility of OR here.

The id check in the query ensures entities don't consider themselves duplicates when being updated.

ah ha, yes!

+++ b/core/modules/content_moderation/src/Plugin/Validation/Constraint/UniqueContentModerationStateConstraintValidator.php
@@ -0,0 +1,38 @@
+   */
+  public function validate($entity, Constraint $constraint) {
+    $query = \Drupal::entityQuery('content_moderation_state')
+      ->allRevisions()
+      ->condition('content_entity_type_id', $entity->content_entity_type_id->value)
+      ->count()
+      ->range(0, 1);
+

This should have an accessCheck(FALSE) even if we never expect anyone to implement access. But I also agree with @alexpott I'd rather see this at the data level if we can.

Wait, so not enforcing this with the schema? Does that mean the primary feedback is the access check?

Edit: Oh, I get it, lets not prevent multiple workflows on the data level, but still the other constraints.

When the suggestion in #22 is implemented we get a revision table that looks like:

mysql> select * from test28373200content_moderation_state_field_revision;
+----+-------------+----------+-----+-----------+------------------+------------------------+-------------------+----------------------------+------------------+
| id | revision_id | langcode | uid | workflow  | moderation_state | content_entity_type_id | content_entity_id | content_entity_revision_id | default_langcode |
+----+-------------+----------+-----+-----------+------------------+------------------------+-------------------+----------------------------+------------------+
|  1 |           1 | en       |   0 | editorial | draft            | node                   |                 1 |                          1 |                1 |
|  1 |           2 | en       |   0 | editorial | draft            | node                   |                 1 |                          1 |                1 |
|  1 |           2 | fr       |   0 | NULL      | draft            | node                   |                 1 |                          1 |                0 |
+----+-------------+----------+-----+-----------+------------------+------------------------+-------------------+----------------------------+------------------+

Under the hood we're creating a new revision and adding a language. I'm not totally sure, but I think this could be a valid state. The the revision ID is 2 for the french node, we would need a corresponding revision for the other languages, I think.

On the ways to make the translation test pass was to only create a new ContentModerationState revision if the entity under moderation was also a new revision. This passes that specific test, but we'll see what the testbot says about the rest of them.

+++ b/core/modules/content_moderation/tests/src/Kernel/UniqueContentModerationStateEntityTest.php
@@ -83,18 +83,6 @@ public function testContentModerationStateEntityValidation() {
-    // Only one match from entity id or revision ID should trigger a violation.
-    $this->assertContentModerationStateViolations([
-      'content_entity_type_id' => $node->getEntityTypeId(),
-      'content_entity_id' => 9999,
-      'content_entity_revision_id' => $node->getRevisionId(),
-    ], TRUE);
-    $this->assertContentModerationStateViolations([
-      'content_entity_type_id' => $node->getEntityTypeId(),
-      'content_entity_id' => $node->id(),
-      'content_entity_revision_id' => 9999,
-    ], TRUE);
-

The tests were quite handy to see how this behaved compared to the constraint. This is one of the test cases that is no longer valid by using the unique keys.

Previously I was validating that you could have no ContentModerationState entity that shared either an ID or revision ID, but now it's the combination of both.

Probably a fair trade off given the simplicity of this approach + perf win?

Test fixes, phpcs and removing another left over test case from the constraint.

Good catch re: adding the workflow key.

TIL re: indexes. Working with the database schema directly is a bit of a blind spot for me, so appreciate the feedback. Curse the golden handcuffs of abstraction! :)

Yeah, maybe the test should be named after the class it's testing. Removes artefacts from the constraint.

It doesn't look like the test covers langcode. Other than that looks good.

Good catch.

Nice one!

As a maintainer of the REST module and fixing lots and lots of Entity + REST support bugs lately, I've seen my share of entity validation constraints. Which is why I took a look at this issue.

1. +++ b/core/modules/content_moderation/src/Entity/ContentModerationState.php
   @@ -82,10 +83,6 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
   -    // @todo https://www.drupal.org/node/2779931 Add constraint that enforces
   -    //   unique content_entity_type_id, content_entity_id and
   -    //   content_entity_revision_id.
   

   So this issue fixes this todo.

2. +++ b/core/modules/content_moderation/src/ContentModerationStateStorageSchema.php
   @@ -16,11 +16,20 @@ class ContentModerationStateStorageSchema extends SqlContentEntityStorageSchema
   -    // Creates an index to ensure that the lookup in
   -    // \Drupal\content_moderation\Plugin\Field\ModerationStateFieldItemList::getModerationState()
   -    // is performant.
   -    $schema['content_moderation_state_field_data']['indexes'] += [
   -      'content_moderation_state__lookup' => ['content_entity_type_id', 'content_entity_id', 'content_entity_revision_id'],
   +    // Creates unique keys to guarantee the integrity of the entity and to make
   +    // the lookup in ModerationStateFieldItemList::getModerationState() fast.
   +    $unique_keys = [
   +      'content_entity_type_id',
   +      'content_entity_id',
   +      'content_entity_revision_id',
   +      'workflow',
   +      'langcode',
   +    ];
   +    $schema['content_moderation_state_field_data']['unique keys'] += [
   +      'content_moderation_state__lookup' => $unique_keys,
   +    ];
   +    $schema['content_moderation_state_field_revision']['unique keys'] += [
   +      'content_moderation_state__lookup' => $unique_keys,
        ];
   
   +++ b/core/modules/content_moderation/src/Entity/ContentModerationState.php
   @@ -74,6 +74,7 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
   +      ->setSetting('max_length', EntityTypeInterface::ID_MAX_LENGTH)
   

   Except this is the only constraint-related changes. We're setting a max length. And we're changing the storage schema to have a uniqueness requirement.

   But why not have an actual entity validation constraint?

   Like for example \Drupal\aggregator\Plugin\Validation\Constraint\FeedUrlConstraint?

3. +++ b/core/modules/content_moderation/tests/src/Kernel/ContentModerationStateStorageSchemaTest.php
   @@ -0,0 +1,142 @@
   +    $this->assertStorageException([
   

   This proves what I thought the problem was: we're relying on storage exceptions rather than doing actual uniqueness constraint checking.

Apparently the patch in #15 did this. But @alexpott wrote this in #19:

Regardless of whether we enforce via a constraint on not I definitely think we should enforce on the data level.

I don't disagree with that. It's fine to also enforce this on the data level.

But leaving this entirely to the storage level like this is doing right now means that you'll only ever get "storage exception" as feedback, rather than explicit validation errors. That's going too far.

If #2843753: Prevent ContentModerationState from being exposed by REST's EntityResource would already have been done, then I could easily add the extra REST test coverage to show that this would result in very unhelpful error responses via REST. If you'd have a validation constraint, then you'd get very helpful error responses.

But leaving this entirely to the storage level like this is doing right now means that you'll only ever get "storage exception" as feedback, rather than explicit validation errors. That's going too far.

I agree, these exceptions don't provide any additional semantics while entity validations do.

I'm happy to roll the constraint into the patch as well, it's already written. As pointed out it just add some extra cost to saving the field data, but justified if there's a good reason (like better REST support).

Yay!

Added the entity validations back into the patch.

Ideally, #2843753: Prevent ContentModerationState from being exposed by REST's EntityResource would already have been fixed, then this issue could be expanding the test coverage introduced in that issue. But it's not reasonable to block this on that , so … back to RTBC per #40 :)

At least for me the primary test coverage for entity constrains should not be on the REST level but rather on the pure entity api level,
because when you put it onto the rest level you are one level too high in the abstraction chain and will forget cases of the validation.

#48: absolutely!

The question of whether this entity type should be interacted with directly, not via the computed field has also come up recently in #2860889: ContentModerationStateInterface additions.

#51: what does "OTT" mean?

When doing bulk importing, the code doing that could just skip validation (assuming it was very thoroughly tested).

IOW:

1. we want validation on principle, and for REST clients/contrib modules and so on
2. particular implementations that are very well-tested could skip validation, for example bulk migration

I think that strikes the desired balance?

OTT = Over The Top.

I'm not sure on the validation + exception vs. exception-only question - part of me thinks we should commit the exception and open a follow-up for the validation.

follow-up for the validation

History suggests that follow-up is unlikely to be fixed.

+1 to #54.

Re: #55 - Maybe it doesn't get fixed for a reason? like it's not needed or there isn't a good solution.

Half of this patch is better than no patch at all.

like it's not needed

Suggesting this is a possible explanation is dishonest. Obviously there is a need: REST/JSON API/GraphQL/RELAXed Web Services need this.

There is a green patch that supports validation.

Hence the ambivalence in @alexpott's comment in #50.

I'm not completely sure that anything should be interacting with these entities directly.

I don't know. Why should anybody be interacting with FilterFormat config entities or Item content entities directly?

My intent is not to hold up this issue.

My intent is to force us to think about the consequences. To consider the integration with the rest of the system. Rather than just postponing that to a follow-up. If that follow-up must be resolved before content_moderation can become stable, then sure. But otherwise we're just creating more technical debt, because we have not actually solved the integration question.
A contrib module that gets moved into core must integrate with all relevant other core modules. If we don't, we just increase core's technical debt. (And in this case, also the amount of work for the API-first initiative, yes.)

Updating issue title to reflect current scope.

#61 makes it sound like there is never a reason to create these ContentModerationState entities via a HTTP API (REST, GraphQL or otherwise) or via code. If that's the case: okay, then there's no need for validation. But then we somehow need to be able to indicate that entities of this type can never be created/updated/deleted/viewed. Which we can do, by implementing an access control handler that forbids access for all operations.

This issue is still RTBC, which kinda means the community agrees on #44 being the way forward, although @alexpott makes a good point about the overhead of entity query in a validation.

Therefore, could we get the patch from #38 committed, then open a follow up to discuss, work on the interdiff in #44?

I think what everyone agrees on right now is that the ContentModerationState entity is a kind of internal implementation detail that we don't really want expose to users that heavily. For that reason, I think the access control handler is a really good idea.

I've opened #2873287: Dispatch events for changing content moderation states in order to try to squash another use case which brings these entities into the foreground, with the view that we could almost mark the whole thing as @internal.

Opened #2873291: Create an access control handler to deny CRUD access to the ContentModerationState entity for this.

I think #2873291: Create an access control handler to deny CRUD access to the ContentModerationState entity should be done in the scope of this issue, if we indeed think that this entity type is internal. I'll provide the implementation:

 .../ContentModerationStateAccessControlHandler.php | 31 ++++++++++++++++++++++
 .../src/Entity/ContentModerationState.php          |  1 +
 2 files changed, 32 insertions(+)

diff --git a/core/modules/content_moderation/src/ContentModerationStateAccessControlHandler.php b/core/modules/content_moderation/src/ContentModerationStateAccessControlHandler.php
new file mode 100644
index 0000000..844b313
--- /dev/null
+++ b/core/modules/content_moderation/src/ContentModerationStateAccessControlHandler.php
@@ -0,0 +1,31 @@
+<?php
+
+namespace Drupal\content_moderation_state;
+
+use Drupal\Core\Access\AccessResult;
+use Drupal\Core\Session\AccountInterface;
+use Drupal\Core\Entity\EntityAccessControlHandler;
+use Drupal\Core\Entity\EntityInterface;
+
+/**
+ * Defines the access control handler for the content_moderation_state entity type.
+ *
+ * @see \Drupal\content_moderation\Entity\ContentModerationState
+ */
+class ContentModerationStateAccessControlHandler extends EntityAccessControlHandler {
+
+  /**
+   * {@inheritdoc}
+   */
+  public function checkAccess(EntityInterface $entity, $operation, AccountInterface $account) {
+    return AccessResult::forbidden();
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  protected function checkCreateAccess(AccountInterface $account, array $context, $entity_bundle = NULL) {
+    return AccessResult::forbidden();
+  }
+
+}
diff --git a/core/modules/content_moderation/src/Entity/ContentModerationState.php b/core/modules/content_moderation/src/Entity/ContentModerationState.php
index cb1bc1f..a57ed8f 100644
--- a/core/modules/content_moderation/src/Entity/ContentModerationState.php
+++ b/core/modules/content_moderation/src/Entity/ContentModerationState.php
@@ -24,6 +24,7 @@
  *   handlers = {
  *     "storage_schema" = "Drupal\content_moderation\ContentModerationStateStorageSchema",
  *     "views_data" = "\Drupal\views\EntityViewsData",
+ *     "access" = "Drupal\content_moderation\ContentModerationStateAccessControlHandler"
  *   },
  *   base_table = "content_moderation_state",
  *   revision_table = "content_moderation_state_revision",

Then at least this issue/patch will leave us in a consistent state, rather than an inconsistent state.

I like it, but keen to hear what @alexpott thinks.

Why wouldn't we just do this in the follow up?

but as the code is written, lets just get it in.

Updating issue title.

Added the code from #65 and a quick test. Updated the issue summary as well.

Added comment.

Thanks! @timmillwood.

Unrelated failure.

Nice one! Thanks for the all the guidance on this one @alexpott and @Wim Leers.

Yay! Sorry for insisting, but I am convinced this will help our collective future sanity :)

This needs to be added to the upgrade path because of the change to max_length.