Create validation constraint for ComponentTreeStructure

Created on 11 July 2024, 5 months ago
Updated 13 August 2024, 4 months ago

Overview

Follow-up to πŸ“Œ FieldType: Support storing component *trees* instead of *lists* Fixed . In that issue we made it so \Drupal\Core\TypedData\TypedData::__construct stores tree values(not an actual in json but a value we can construct into a tree).

But just added a simple exception for the validation.

We need move that to a constraint.

Proposed resolution

From @wim leers comment

We need to add a validation constraint for `ComponentTreeStructure`. Note that πŸ“Œ Lift most logic out of ComponentTreeItem::preSave() and into a new validation constraint Needs work is _not_ overlapping with this: that is about validating the `ComponentPropsValues` _for a given `ComponentTreeStructure`. But long before we get to that point, we need to ensure that at least the tree structure itself is valid (independent from the _values for the props of the components in that tree_, which is what πŸ“Œ Lift most logic out of ComponentTreeItem::preSave() and into a new validation constraint Needs work is about).

That validation constraint should validate that:

  1. the root UUID is present
  2. any top-level UUID appears as a UUID in a _parent branch/tree_ (except its own branch), if the component for that UUID is a component that has >=1 slots
  3. slots of every component: the stored slot names (firstColumn and secondColumn in the example at πŸ“Œ FieldType: Support storing component *trees* instead of *lists* Fixed ) MUST be actually existing slot names for this particular component instance.

User interface changes

πŸ“Œ Task
Status

Fixed

Component

Data model

Created by

πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @tedbow
  • Status changed to Postponed 5 months ago
  • πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA
  • πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
  • πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA
  • πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

    I see this now, after I already wrote #3455728-9: FieldType: Support storing component *trees* instead of *lists* β†’ …

    Actually … why don't we merge this with πŸ“Œ Lift most logic out of ComponentTreeItem::preSave() and into a new validation constraint Needs work β€” i.e. why don't we expand the scope of that? It's exactly the same problem space!

  • πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA
  • Assigned to tedbow
  • πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

    Per #3456024-15: Lift most logic out of ComponentTreeItem::preSave() and into a new validation constraint β†’ , keeping this issue around, not marking it as a duplicate. This is blocked on #3456024.

    This is the next most important issue, because it is in the critical path of πŸ“Œ Introduce an example set of representative SDC components; transition from "component list" to "component tree" Fixed , which is itself getting in the critical path of next steps on the UI side.

    Pre-emptively assigning to @tedbow β€” now that #3456024 is nearly done, this should go a lot faster. 🀞

  • Status changed to Active 5 months ago
  • πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

    Blocker is in.

  • πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA
  • Merge request !105Resolve #3460856 "Structure validation" β†’ (Merged) created by tedbow
  • Status changed to Needs work 5 months ago
  • πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA
  • πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

    This is going in the right direction πŸ‘

    1. ComponentTreeStructureTest looks good but is not yet testing all edge cases. I see at least two missing important edge cases:
      1. What if there is a tree of 3 levels deep, and the first level is valid (i.e. all UUIDs under the root also have top-level keys), and the third level is valid, but the second is not? (i.e. a child of the first level does not have its UUID does not exist as a top-level key).
      2. "dangling" aka leftover top-level keys, for components that have been removed. These would effectively be dead weight that continues to exist, taking up storage space while not being visible!
    2. Can you add a failing test case to \Drupal\Tests\experience_builder\Kernel\Config\DefaultFieldValueTest that triggers a failure for this new constraint for a config-defined component tree?
    3. I think the test cases would be much simpler to read if they weren't defined as JSON strings, but as json_encode([ … stuff … ]) instead.
  • πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

    I just realized an important validation piece is missing if this is to unblock πŸ“Œ Introduce an example set of representative SDC components; transition from "component list" to "component tree" Fixed .

  • πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA

    re #12

    1. 1. What if there is a tree of 3 levels deep, and the first level is valid (i.e. all UUIDs under the root also have top-level keys), and the third level is valid, but the second is not? (i.e. a child of the first level does not have its UUID does not exist as a top-level key).

      and

      πŸ‘† these are edge cases that I described succinctly in the issue summary as any top-level UUID appears as a UUID in a _parent branch/tree_(except its own branch)

      I don't think this first case is actually what is described in the summary but rather the inverse(unless we are still not talking about the same of constructing the tree from the storage)

      (i.e. all UUIDs under the root also have top-level keys)

      UUIDs under the root should not need to have top-level keys, some will but components without slots should not need any other information in the tree. I confirmed with Jesse Baker that Components are not required to have slots.

      (i.e. a child of the first level does not have its UUID does not exist as a top-level key).

      There is a typo here I think but again I think a child of the first level should not be required to exist as top level key because the child of the first level might not have a slot that needs representing in the tree.

      So I think this tree would valid but it does pass your suggested validation

      [
        "ROOT_UUID" => [
            [ "uuid" => "UUID-IN-ROOT-NOT-TOP-LEVEL" , "component" => "component-with-no-slots" ] , 
            [ "uuid" => "UUID-IN-ROOT-AT-TOP-LEVEL" , "component" => "component-with-slots" ] , 
        ],
        "UUID-IN-ROOT-AT-TOP-LEVEL" => [
          'firstCol' => [
              [ "uuid" => "UUID-UNDER-1ST-LEVEL-NOT-AT-TOP-LEVEL" , "component" => "component-with-no-slots" ] , 
              [ "uuid" => "UUID-UNDER-1ST-LEVEL-ALSO-TOP-LEVEL" , "component" => "component-with-slots" ] , 
          ]
           
        ],
        "UUID-UNDER-1ST-LEVEL-ALSO-TOP-LEVEL"  => [
            'firstCol' => [
                 [ "uuid" => "UUID-UNDER-2ND-LEVEL" , "component" => "component-with-no-slots" ] , 
                 [ "uuid" => "UUID-UNDER-2ND-LEVEL" , "component" => "component-with-no-slots" ] , 
           ]
          
        ]
      ]
      

      Sorry for the weird naming basically I thought "component-with-no-slots" will be dead-end in the tree and that is by design.

      I have updated the test cases to use either "component" => "component-no-slots" or "component" => "component-slots" to make this clearer.

    2. "dangling" aka leftover top-level keys, for components that have been removed. These would effectively be dead weight that continues to exist, taking up storage space while not being visible!

      I think that is what this existing case is catching

      'INVALID: valid tree, with unknown top level' => [
              [
                ComponentTreeStructure::ROOT_UUID => [["uuid" => "uuid-in-root-only", "component" => "component-no-slots"]],
                "uuid-unknown-top-level" => [["uuid" => "uuid-under-top-level", "component" => "component-no-slots"]],
              ],
              ['The UUID <em class="placeholder">uuid-unknown-top-level</em> is not in the tree.'],
            ],
      

      but I have expanded the tree here
      'INVALID: valid tree, with unknown top level' => [
      [
      ComponentTreeStructure::ROOT_UUID => [
      ["uuid" => "uuid-in-root-only", "component" => "component-no-slots"],
      ["uuid" => "uuid-in-root-and-top-level", "component" => "component-slots"],
      ],
      "uuid-in-root-and-top-level" => [
      "slot1" => [
      ["uuid" => "uuid-1st-tier-top-level", "component" => "component-slots"],
      ],
      ],
      "uuid-1st-tier-top-level" => [
      "slot1" => [
      ["uuid" => "uuid-under-1st-tier", "component" => "component-no-slots"],
      ],
      ],
      "uuid-unknown-top-level" => [
      "slot1" => [
      ["uuid" => "uuid-under-top-level", "component" => "component-no-slots"],
      ],
      ],
      ],
      ['The UUID uuid-unknown-top-level is not in the tree.'],
      ],

    3. I fixed some of the test cases that were not nested correctly.
    4. RE: Do we want to add validation that if component has slots it should have top level key? My guess is not because that would require use to load the actual component config and also maybe some have optional slots but just putting this out there we don't validate this.
    5. Can you add a failing test case to \Drupal\Tests\experience_builder\Kernel\Config\DefaultFieldValueTest that triggers a failure for this new constraint for a config-defined component tree?

      Yes, will do this, but not done yet.

      Should we also add the test cases to ComponentTreeItemTest::testInvalidField() to make it sure it triggers for field instances? Right now they use the same helper function, ComponentTreeTestTrait::getComponentTreeTestCases() to provide test cases so we test the same cases in both places

    6. I think the test cases would be much simpler to read if they weren't defined as JSON strings, but as json_encode([ … stuff … ]) instead.

      Agreed, fixed

    Leaving assigned to myself to do the remaining items but I need confirmation from @Wim Leers that I am right about 1)

  • πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA

    @Wim Leers I am working on this part now

    slots of every component: the stored slot names (firstColumn and secondColumn in the example at πŸ“Œ FieldType: Support storing component *trees* instead of *lists* Fixed ) MUST be actually existing slot names for this particular component instance.

    I haven't actually dealt with this part of the module at all. Any tips here. How do I load a component from an id? Is this always config component as defined by \Drupal\experience_builder\Entity\Component? there is no class comment so hard to tell. Or could this any sdc component. I have loaded a Component config entity but also there I can't see slot info there.

  • Assigned to wim leers
  • Status changed to Needs review 5 months ago
  • πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA

    @Wim Leers assigning back to you for questions on #14. If you have pointers for #15 that would be great(otherwise when I get back this issue I will keep looking)

  • Assigned to tedbow
  • Status changed to Needs work 5 months ago
  • πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

    #14:

    1. Sorry for the confusion on this one β€” my wording was a bit too succinct it seems πŸ™ˆ

      some will but components without slots should not need any other information in the tree
      …
      There is a typo here I think but again I think a child of the first level should not be required to exist as top level key because the child of the first level might not have a slot that needs representing in the tree

      Indeed πŸ‘

      So I think this tree would valid but it does pass your suggested validation

      Yes πŸ‘ (And β†’ I think it's excellent naming πŸ˜„)

    2. I think that is what this existing case is catching

      You're right β€” my bad! πŸ™ˆ

    3. πŸ‘
    4. Regardless of what choice we make here: we should have explicit test coverage for it. Either explicitly test the absence of a validation error, or the presence. AFAICT we don't have such a test case yet.
      • I'm not sure yet which way to go. I think (but it's subjective currently, not yet objective) it'd actually be clearer if we did require that.
      • An objective benefit of that would be that the completeness of the tree can be validated.
      • An objective downside is that this means more JSON to store.
      • Your wording seems to imply that loading the actual component config entity is a downside. I disagree with that. Because \Drupal\experience_builder\Plugin\Validation\Constraint\ValidComponentTreeConstraintValidator::validate() already needs to do that anyway β€” so either way, the caches would be warm.

      Overall, I'd rather err on the side of more detailed validation for now, to allow us to not have to ever think about this again after this issue β€” because this issue will immediately catch/flag any mistake we'll make on our way to the full XB functionality scope.

    5. Yes, that would be great.

    #15: you can load a component from an ID using \Drupal\experience_builder\Entity\Component::loadByComponentMachineName().

  • πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA

    @Wim Leers sorry for further confusion

    but originally in #14 I typed

    So I think this tree would valid but it does pass your suggested validation

    But meant this.

    So I think this tree would valid but it does NOT pass your suggested validation

    You responded to this in #17 so wanted to called that out.

    I edited #14

  • πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
    1. uuid-unknown-top-level is danging/leftover and is the expected validation error message … That's why I replied affirmatively. I don't see how that tree would be valid?
  • πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA

    re #19

    The correction in #18 I was referring to was actually for #14.1

    not #14.2

  • πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
    1. Yes, like I already wrote in #17.1 while specifically quoting what you wrote about slotless components. Which means I already confirmed that the list of things to validate was missing that important nuance.

      I agree that the example you provided in #14.1 MUST be considered valid.

      In fact, if UUID-IN-ROOT-NOT-TOP-LEVEL did appear as a top-level key, then THAT would be a violation.

    I see I didn't respond to this bit in #15:

    I have loaded a Component config entity but also there I can't see slot info there.

    • First get the component plugin ID (i.e. \Drupal\Core\Plugin\Component::$machineName) for a Component config entity, by using \Drupal\experience_builder\Entity\Component::convertIdToMachineName().
    • Then load the Component plugin instance using plugin.manager.sdc
    • Finally: given the plugin instance, use \Drupal\Core\Plugin\Component::$metadata to determine what the slots are.

    … adding a helper method to the config entity would be fine, of course.

  • Assigned to wim leers
  • Status changed to Needs review 5 months ago
  • πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA
  • Assigned to tedbow
  • Status changed to Needs work 5 months ago
  • πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

    This is making a pretty drastic change that is not captured in the issue summary: https://git.drupalcode.org/project/experience_builder/-/merge_requests/1... β€” in that comment I explain why we should revert that and leave such a change for 🌱 [META] Support component types other than SDC Needs work to make.

    I also found 2 critical gaps in the test coverage:

    … and a number of additional questions.

  • Assigned to wim leers
  • Status changed to Needs review 5 months ago
  • πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA

    @Wim Leers I changed this to use symfony/validator constraints. I think this is better.

    I still need to add some test cases for missing keys and type errors but this logic should already be covered by the constraints.

    So I have still have work on it. If you want to just wait until I am done, I can just continue working on it. But if you want to look at the approach feel free

  • Assigned to tedbow
  • Status changed to Needs work 5 months ago
  • πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

    Wow, that looks much better! πŸ‘

    I'd even use the word impressive β€” it's nice to see that when we use Symfony's Validation component "as intended", that it is so much clearer than if there's Drupal's layer of abstraction (either config schema or Entity/Field/Typed Data) in between 🀯

  • πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

    I know this issue is not yet marked as (especially because you wrote at https://git.drupalcode.org/project/experience_builder/-/merge_requests/1... that you need more test cases before addressing that), but given its importance, pre-emptively reviewed to make this go faster 😊

    I think this is now in its final stretch β€” the Symfony-based validation logic makes this much easier to understand! πŸ‘

  • Assigned to wim leers
  • Status changed to Needs review 5 months ago
  • πŸ‡ΊπŸ‡ΈUnited States tedbow Ithaca, NY, USA

    @Wim Leers see MR comments

  • Issue was unassigned.
  • Status changed to RTBC 5 months ago
  • πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

    This looked great, but given its foundational importance, I subjected it to a lot of scrutiny. Some bits I found hard to understand, so I worked to make that easier for the next person. And since there will be many next persons … I spent the entire day on this 😳

    Summary of changes I made:

    • add detailed documentation about the data structure, including establishing terminology
    • use that terminology consistently in validation error messages
    • make the validation error messages point to more precise property paths
    • properly integrate the "subvalidator"-triggered violations with the Drupal validator β€” no longer losing property path information
    • use the BasicRecursiveValidatorFactory to ensure validation error messages use TranslatableMarkup just like other validators β€” see https://www.drupal.org/node/3396203 β†’
    • made the (kernel) tests much faster β€” they were painfully slow πŸ˜…

    90% of what @tedbow wrote is still there. I just smoothened out the rough edges.

    So glad to have this done 😊

  • Pipeline finished with Skipped
    5 months ago
    #238583
  • Status changed to Fixed 5 months ago
  • πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

    🚒

    Unblocked:

    1. πŸ› Support propless SDCs Needs review
    2. πŸ“Œ Create validation constraint for ComponentTreeStructure Needs work
  • Automatically closed - issue fixed for 2 weeks with no activity.

Production build 0.71.5 2024