Repository: ClickHouse/ClickHouse
File: src/Access/AccessRights.cpp (C++)

Code before:
            for (const auto & this_child : *children)
            {
                Node other_child = other.tryGetLeaf(this_child.node_name, this_child.level, !this_child.isLeaf());
                if (!this_child.contains(other_child))
                    return false;
            }
        }

        return true;
    }

    void makeUnion(const Node & other)
    {
        /// We need these tmp nodes because union/intersect operations use the `getLeaf` function, which can't be made const.
        /// Potentially, we can use tryGetLeaf, but it's very complicated to traverse both trees at the same time:
        ///
        /// Tree1:
        ///              f (SELECT)
        ///           /            \
        ///        ancy (SELECT)   oo (SELECT)
        ///          |                 |
        ///    "" (SELECT, INSERT)    "" (SELECT, INSERT)
        ///
        /// Tree2:
        ///          foo (SELECT)
        ///              |
        ///         "" (SELECT, INSERT)
        ///
        /// Here Tree2.getLeaf("f") will return a valid node, but Tree2.tryGetLeaf("f") will return nullptr

        Node result;
        Node rhs = other;
        makeUnionRec(result, rhs);
        children = std::move(result.children);
        flags |= other.flags;
        optimizeTree();
    }

    void makeIntersection(const Node & other)
    {
        Node result;
        Node rhs = other;
        makeIntersectionRec(result, rhs);
        children = std::move(result.children);
        flags &= other.flags;
        optimizeTree();
    }

    void makeDifference(const Node & other)
    {
        Node rhs = other;

Review comment (bug):
`contains()` now recursively calls itself on `Node` instances returned by `tryGetLeaf()`. However, the documented contract for `tryGetLeaf()` explicitly warns that reading `.children` on returned ephemeral nodes may break the grants tree logic. Since `contains()` traverses `.children`, this can violate that contract and risks incorrect results or fragile behavior. Consider changing `contains()` to traverse the original tree nodes directly (e.g., via a `const Node * tryFindNode(...)`-style API returning pointers/references into the existing structure), or update `tryGetLeaf()` to return nodes safe for read-only recursive traversal in this context (e.g., ensuring a consistent/owned subtree view for the returned node).
```suggestion
        /// Traverse the real trees on both sides and use tryGetLeaf() only for
        /// flag comparison, never for traversing children of ephemeral nodes.
        auto check_direction = [&](auto && self, const Node & src, const Node & dst) -> bool
        {
            if (!src.children)
                return true;

            for (const auto & src_child : *src.children)
            {
                Node dst_at_child = dst.tryGetLeaf(src_child.node_name, src_child.level, !src_child.isLeaf());
                if (!dst_at_child.flags.contains(src_child.flags))
                    return false;

                if (!self(self, src_child, dst))
                    return false;
            }

            return true;
        };

        if (!check_direction(check_direction, other, *this))
            return false;

        if (!check_direction(check_direction, *this, other))
            return false;
```

Code after:
        /// Step 1 alone would pass because `other` has no children to check against, but `this` does not contain `other` because
        /// `this` has revoked SELECT on "foo".

        if (!flags.contains(other.flags))
            return false;

        if (other.children)
        {
            for (const auto & other_child : *other.children)
            {
                Node this_child = tryGetLeaf(other_child.node_name, other_child.level, !other_child.isLeaf());
                if (!this_child.contains(other_child))
                    return false;
            }
        }

        if (children)
        {
            for (const auto & this_child : *children)
            {
                Node other_child = other.tryGetLeaf(this_child.node_name, this_child.level, !this_child.isLeaf());
                if (!this_child.contains(other_child))
                    return false;
            }
        }

        return true;
    }

    void makeUnion(const Node & other)
    {
        /// We need these tmp nodes because union/intersect operations use the `getLeaf` function, which can't be made const.
        /// Potentially, we can use tryGetLeaf, but it's very complicated to traverse both trees at the same time:
        ///
        /// Tree1:
        ///              f (SELECT)
        ///           /            \
        ///        ancy (SELECT)   oo (SELECT)
        ///          |                 |
        ///    "" (SELECT, INSERT)    "" (SELECT, INSERT)
        ///
        /// Tree2:
        ///          foo (SELECT)
        ///              |
        ///         "" (SELECT, INSERT)
        ///
        /// Here Tree2.getLeaf("f") will return a valid node, but Tree2.tryGetLeaf("f") will return nullptr

        Node result;
        Node rhs = other;
        makeUnionRec(result, rhs);