feat(comid): enforce acyclic constraint on dependency-triples#260
Conversation
thomas-fossati
left a comment
There was a problem hiding this comment.
Thanks very much! LGTM.
I have a couple of comments which could be addressed either in this PR or in a future changeset:
- The
DomainDependencyTriple::Validfunction could also check for duplicate trustees DomainDependencyTriples::Validcould do more than call theValidfunction for individual triples; it could also check the overall consistency of the dependency graph expressed by all the DDTs present and flag cycles across triples.
5d8183d to
37de8b1
Compare
|
@thomas-fossati good call on the second validation, I added here since it is open and relevant to recent changes! Thanks for the feedback! |
Extend DomainDependencyTriple.Valid() to reject direct self-references and duplicate trustees. Extend DomainDependencyTriples.Valid() with DFS cycle detection across the full dependency graph (§5.1.11.2). Signed-off-by: Rodrigo Sampaio Vaz <rvaz@nvidia.com>
37de8b1 to
572b665
Compare
thomas-fossati
left a comment
There was a problem hiding this comment.
Thank you again!
Your cycle-detection choice is an interesting one. Personally, I’d have used topo sorting and, because I am a lazy individual, I’d have pilfered one of the existing packages (e.g., stevenle/topsort/v2 or similar) :-)
I went with DFS because it was compact enough for this use case, agree that topo sorting would also work :) |
Per draft-ietf-rats-corim 5.1.11.2, a domain-id MUST NOT appear in its trustees list. Add the check to DomainDependencyTriple.Valid() and extend the test suite accordingly.