July 14 changes #4
Conversation
This reverts commit 15fee6b.
| for _, pattern := range d.Paths { | ||
| if matched, _ := filepath.Match(pattern, file); matched { | ||
| return true | ||
| return true, nil |
There was a problem hiding this comment.
This is what I wanted to avoid with the struct that enforces oneof Paths, PathHashPrefixes.
I'm more comfortable with the second approach ethan-lowman-dd@8eac49a
We are bubbling complexity:
- a user can create an invalid DelegatedRole with the struct
- we have to add error handling everywhere for no real error (can be checked at encoding and decoding)
There was a problem hiding this comment.
Unless the type system can fully enforce all validation (which it can't for more complex kinds of validation), I don't see a way to avoid storing invalid data in a struct. In my opinion, matching the type definition directly to the spec is much easier to understand (for library authors and users alike), and validating before use and when marshalling/unmarshalling provides the necessary properties, matching the spec as close as possible.
Using a boolean flag (MatchWithHashPrefixes) and field with two possible meanings (PathMatchers) to implement the "one of" constraint forces users to understand quirks of the library's implementation, rather than following the spec.
Additionally, the boolean field is not extensible. If the TUF spec introduces a third field that is also subject to the "one of" constraint, the boolean flag would no longer properly implement the constraint across the three fields.
raphaelgavache
merged commit ea6bcf8
into
raphaelgavache:raphael/support_delegations_client
* Clarify validation of signing role & metadata type * Add/update comments * Validation happens on both decode & encode * Bubble up an error if MatchesPath is called on an invalid DelegatedRole * Match spec for delegation traversal * Comment in the iterator * Add diamond test case * Revert "Match spec for delegation traversal" This reverts commit 15fee6b. * Rename IsTopLevelRole back to ValidRole to avoid breaking change
* Add delegation client * Add TUF3 php test * Use ioutil for go 1.15 * Add more delegations tests * Cleanups * Check new paths * Add suggestion * Add struct tags * Add tags and remove duplicate types * Formatting * Fix order of asserts (should be want, got) * Clean up DelegatedRole validation * Fix root to top targets delegation * July 14 changes (#4) * Clarify validation of signing role & metadata type * Add/update comments * Validation happens on both decode & encode * Bubble up an error if MatchesPath is called on an invalid DelegatedRole * Match spec for delegation traversal * Comment in the iterator * Add diamond test case * Revert "Match spec for delegation traversal" This reverts commit 15fee6b. * Rename IsTopLevelRole back to ValidRole to avoid breaking change * Update after reviews * Add back lower case check * Initialize with size and comment * Simplify iterator initialization * Revert back to "nodes seen" interpretation of delegation traversal spec (#6) (instead of true cycle detection with "edges seen"). This reverts commit cfbb024. * Update client/delegations.go Co-authored-by: Ethan Lowman <53835328+ethan-lowman-dd@users.noreply.github.com> * Update following reviews of 16th of july (#7) * Update name * Rename file to target * Nits * Move verifier in iterator and rename fields * Add tests * Update after 19th july review (#9) * Update delegations to err on top level role * Add simple cycle test * Remove duplicate check * Fix comment * Update comment Co-authored-by: Ethan Lowman <ethan.lowman@datadoghq.com> Co-authored-by: Ethan Lowman <53835328+ethan-lowman-dd@users.noreply.github.com>
See comments on theupdateframework/python-tuf#988.