Introduce Subtree Tags

[bwaidelich]

Currently we use so called "restriction edges" under the hood in order to keep track of disabled nodes.
They have some characteristics that would make them very useful to be used security / filter related features (see #3732).

Ideas

• Rename Restriction Edge to "Subtree Tag" and add it to the edge between nodes (replacing the restriction edge table)
• Make DisableNodeAggregate commands emit a (new) event type NodeAggregateAttributeWasAdded
  • Up-cast NodeAggregateWasDisabled events to the new event type
  • the same for EnableNodeAggregate (emits NodeAggregateAttributeWasRemoved and upcasting for NodeAggregateWasEnabled events)

As a result, projections would have to be adjusted from

private function whenNodeAggregateWasDisabled(NodeAggregateWasDisabled $event) {
    // ...
}

to something like

private function whenSubtreeTagWasAdded(SubtreeTagWasAdded $event): {
    if ($event->tag->value !== 'disabled') {
        return;
    }
    // ...
}

Questions / Discussions

I wonder whether the current behavior of the restriction edges might lead to confusion. E.g.: You could tag a node and that attribute would be "inherited" to all descendants. But you could additionally tag descendants with the same attribute.. Maybe just a matter of good documentation.

Related: #3732

[mhsdesign]

Would it be possible for a child to unset/override an inherited attribute?

[bwaidelich]

Would it be possible for a child to unset/override an inherited attribute?

@mhsdesign not exactly, it would be the same as with the "disabled" flag today: descendant nodes all inherit the attributes, but they can set the same attribute additionally (i.e. disable a node and then disable some ancestor).
When the "upper" attribute is removed, nodes below with the attribute explicitly set would still keep it (i.e. the ancestor is re-enabled but the previously disabled descendant stays disabled, recursively)

It is slightly confusing, but IMO the behavior makes sense (at least for the disable/enable example)

[mhsdesign]

Hi wow thanks for this implementation ❤️ I didnt read through it yet fully - so i might have missed this - but i wanted to ask how do we want to migrate existing 9.0 projects?

As far as i understand the _restrictionrelation table must be renamed to _attribute and probably more schema adjustments.

Currently we seem to handle migrations inside the projections like here:

neos-development-collection/Neos.Neos/Classes/PendingChangesProjection/ChangeProjection.php

Lines 95 to 108 in e0358f6

	// MIGRATIONS
	$currentSchema = $schemaManager->createSchema();
	if ($currentSchema->hasTable($this->tableNamePrefix)) {
	$tableSchema = $currentSchema->getTable($this->tableNamePrefix);
	// added 2023-03-18
	if ($tableSchema->hasColumn('nodeAggregateIdentifier')) {
	// table in old format -> we migrate to new.
	$connection->executeStatement(sprintf('ALTER TABLE %s CHANGE nodeAggregateIdentifier nodeAggregateId VARCHAR(255); ', $this->tableNamePrefix));
	}
	// added 2023-03-18
	if ($tableSchema->hasColumn('contentStreamIdentifier')) {
	$connection->executeStatement(sprintf('ALTER TABLE %s CHANGE contentStreamIdentifier contentStreamId VARCHAR(255); ', $this->tableNamePrefix));
	}
	}

i found this a little odd at first as i thought we would just dump the projection and rebuild it from scratch so i asked sebastian and he explained to me (on slack):

• there are multiple Content Repositories; this does not work with Migrations (as we would need to migrate multiple tables
• We try to migrate projections if easily possible, because this makes the system a lot better behaved for our users. Dumping and rebuilding the projection should be IMHO a last resort because that is quite invasive to our users 🙂

[kitsunet]

isn't the attribute value more an attribute type? Just read roughly over the PR and I think it might be more of a type of attribute. Also with the name attribute I would expect a type/name and a value, so lets say the attribute "topicrelation" with a value of "animals". I think that could be useful as well, but is a totally different feature. So I would rather use a different name for this here?

[mhsdesign]

@kitsunet hmm yes at first i also thought that attributes would have values but now they seem to be rather tags - or true attributes...

A third thing to discuss/ask:

In #3732 you suggested that one can build a subgraph for arbitrary attributes like VisibilityConstraints::forAttributes('disabled');. I like this idea and prefer this over the naming VisibilityConstraints::frontend as i think the escr core should not use the web term "frontend" as this is more just the Neos.Neos usecase.

On the other hand having this magic string 'disabled' hardcoded at many places might not be cool at all too - so maybe because this is a "core" attribute we could introduce a constant or create factory methods for this too.

[bwaidelich]

@mhsdesign @kitsunet thanks for your early feedback!

how do we want to migrate existing 9.0 projects?

Yes, that's missing still, but we can probably address that in the projection setup()

isn't the attribute value more an attribute type?

I called it "tag" at first, but that was also misleading.. Maybe we can find a better name.
Adding yet another dimension (attribute key & value) would make the whole thing quite complex

this magic string 'disabled' hardcoded

That's just WIP, it should definitely be a custom type or constant

[bwaidelich]

It turns out, with this approach making the tags (aka "attributes", "labels", but I'll stick to "tag" from now on) available in the Node read model is not easy and has performance implications.
We had the idea to add the to the hierarchy relation (aka edge) itself. This will not only allow for adding tags to the read model, we can even distinguish between explicitly added tags and inherited ones in the value object. That might come in very handy (and it might even render the NodeHiddenStateProjection redundant #4315).

For the record:
My little MySQL/MariaDB test:

SET @tag = '$.disabled';
SET @nodeId = 'a1';

# ADD TAG

UPDATE
  relation
SET
  tags = JSON_INSERT(tags, @tag, false)
WHERE parentnodeanchor IN (
  WITH RECURSIVE cte (id) AS (
    SELECT childnodeanchor FROM relation WHERE childnodeanchor = @nodeId AND JSON_EXTRACT(tags, @tag) IS NOT TRUE
    UNION ALL
    SELECT
      r.childnodeanchor
    FROM
      cte
      JOIN relation r ON r.parentnodeanchor = cte.id
    WHERE
      NOT JSON_CONTAINS_PATH(r.tags, 'one', @tag)
  )
  SELECT id FROM cte
);

UPDATE
  relation
SET
  tags = JSON_SET(tags, @tag, true)
WHERE
  childnodeanchor = @nodeId;


# REMOVE TAG
SELECT @inherited := EXISTS (SELECT
  p.tags
FROM
  relation p
  JOIN relation r ON r.parentnodeanchor = p.childnodeanchor
WHERE
  r.childnodeanchor = @nodeId
  AND JSON_CONTAINS_PATH(p.tags, 'one', @tag)
LIMIT 1);

UPDATE
  relation
SET
  tags = IF(@inherited, JSON_SET(tags, @tag, false), JSON_REMOVE(tags, @tag))
WHERE childnodeanchor IN (
  WITH RECURSIVE cte (id) AS (
    SELECT CAST(@nodeId AS CHAR(100)) id
    UNION ALL
    SELECT
      r.childnodeanchor
    FROM
      cte
      JOIN relation r ON r.parentnodeanchor = cte.id
  )
  SELECT id FROM cte
);

=>

tags are represented as JSON object in the form:

{
  "<tagName>": <explicit>
}

for example:

{
  "disabled": true,
  "someInheritedTag": false
}

[kitsunet]

mmm, ok but that works only for tags that actually are pertaining to a hierarchical relation then. Which I guess is fine for the use cases we have right now?

[bwaidelich]

but that works only for tags that actually are pertaining to a hierarchical relation then

What do you mean?
The tag is added to the node (aggregate id). The implementation would store it in the corresponding edge (and all descendant edges).

That means, that tags are always effective recursively – but this is a requirement anyways and would be the same no matter which approach we take

[kitsunet]

@bwaidelich i might have misinterpreted the original idea, but I thought originally you wanted to employ "tag" edges alongside the hierarchy edges, which would mean in theory they could also run along reference edges or anything else really. Which might be super useful int he future, but then I also would first have to come up with a specific usecase for that. Could be an extension point though. Either way that was my line of thinking and how it's now presented would not allow that. Fine by me just wanted to point out my thoughts.