Metadata API: Consider if Metadata class should contain role name

[jku]

I'm not sure if this is worth the trouble, writing it out anyway...

Description of issue or feature request:

I've been looking at our data structures lately, trying to improve their usability... and one theme that comes up is that objects should include their identifier if they have one -- the json file format does not do this because it would be wasteful in a wire format but the same does not apply to python objects. There's a PR for Key already but I think the same logic applies to Metadata itself: Our Metadata objects always have a role name that is available when the Metadata is constructed, the name is immutable during lifetime of the object and often needed with the Metadata object (e.g. verify_delegate(), writing to file)

Quick estimates of what this means to API:

• Metadata.__init__(): Add argument role_name: str
• Metadata.from_bytes(): Add argument role_name: str
• Metadata.from_dict(): Add argument role_name: str
• Metadata.from_file(): No change -- the role name can be deciphered from file name. Alternatively the argument could be split to two (to match to_file() below): directory and role name
• Metadata.to_dict(): No change
• Metadata.to_file(): Hmm... not sure how to do this in a nice way: there's three components to the path: directory, basename and extension. Maybe the argument should be just the directory, the basename is then the role name and extension should be a class variable of Serializer?
• verify_delegate(): (once it exists) remove argument role_name -- this is included in metadata already

[jku]

So I've done a rough conversion... and I'm not convinced:

• changes are quite invasive to the API: the changes in Metadata itself are documented fairly well above but it also ends up modifying Serialization
• the gains are not as impressive as they seemed at first glance: most times when we deal with rolenames we may not have the metadata object yet
  • e.g. when updating a delegated targets we want to use the rolename for snapshot hashes/length check before we need to parse the metadata bytes
  • same in the updater when figuring out delegations: there's no metadata object yet

Conclusion: Considered adding role name to Metadata: suggestion is to not implement it at this time.

[joshuagl]

Thanks for doing the research. If you have your WIP changes somewhere, might be good to link them here?
Shall we close this issue?

[jku]

let me rebase before pushing a branch so we see at least the the verify_delegate() improvement...

Something I forgot to mention in the first comment: The non-targets roles of course would be super easy to work with (as the rolename is the signed type...) but that still leaves all targets. So if the Metadata constructors were in Signed implementations (as in the generics experiment branch) that might make things a bit simpler -- but still targets does need a solution.

[jku]

20662aa

• looks a bit better now (at least if you look at the verify_delegate() tests)
• still lot of API change

Copying comment from the commit:

• from_bytes() requires a rolename: this could be inferred
  for other roles but not for targets... but this seems hard to
  express in the API
• If Signed implementations had Metadata constructors (as in the
  Generics branch) the root/snapshot/timestamp implementations would
  automatically be able to infer the rolename but again... targets
  would still need it in the API so either the targets constructor
  would be different or there would be an optional rolename
  (that you would only need for delegated targets)

So this has some links to the other ongoing discussions... let's keep it open: maybe you or someone else has better ideas than I did

[joshuagl]

To be re-assessed when we have figured out #1457

[sechkova]

Seems like a major API change that we won't consider for now. Removing the backlog label but let's keep the discussion open.

[jku]

This looks unlikely to happen now: it would be a big change. I'll close this.