Create constants for top-level rolenames

[ivanayov]

Fixes #1648

This is a change in the metadata API to remove hardcoded role names
and use constants instead.

• The code follows the Code Style Guidelines
• Tests have been added for the bug fix or new feature - updated only
• Docs have been added for the bug fix or new feature - n/a

[MVrachev]

Thanks for your contribution @ivanayov!
I have one preference about the way you defined your constants.
Still, I would want to hear an opinion from someone from the maintainers.
What do you think @sechkova?
Do you agree with my comment?

[coveralls]

Pull Request Test Coverage Report for Build 1452565760

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

• For more information on this, see Tracking coverage changes with pull request builds.
• To avoid this issue with future PRs, see these Recommended CI Configurations.
• For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

• 33 of 34 (97.06%) changed or added relevant lines in 3 files are covered.
• 10 unchanged lines in 2 files lost coverage.
• Overall coverage increased (+1.1%) to 98.599%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
tuf/ngclient/updater.py	12	13	92.31%

Files with Coverage Reduction	New Missed Lines	%
tuf/api/metadata.py	5	98.0%
tuf/ngclient/updater.py	5	95.07%

Totals
Change from base Build 1449719184:	1.1%
Covered Lines:	3626
Relevant Lines:	3645

---

💛 - Coveralls

[sechkova]

I agree with both of you ...
On one hand, Root, Targets etc. classes already exist in metadata.py and now if we add similar constants for role names, we'll be importing things like

from tuf.api.metadata import Targets, TARGETS

On the other hand, names do grow in size.

I was thinking if it there is a convenient data structure than would:

• encapsulate rolenames
• act as iterator allowing us to call list(Rolenames); for role in Rolenames: and by doing this replaces TOP_LEVEL_ROLE_NAMES too
• have a short enough name 🤷

But this is only a suggestion and if implementing this feels like an overkill for python-tuf, I'm ok with simple constants.

[MVrachev]

@lukpueh do you have an opinion on the matter?
I am asking you because we had discussed such issues before with you.

[lukpueh]

FWIW, in warehouse they use an enum subclass to do this, maybe that's the data structure @sechkova was thinking of, it seems to at least allow iterating etc.? :)

Another way of encapsulation would be to make them class attributes of the corresponding metadata classes.

I'm really fine either way.

[ivanayov]

I think enum covers all the cases, as @lukpueh also mentioned.

[ivanayov]

I pushed an update with enum.

[MVrachev]

Honestly, I am still uncertain about the usage of an enum here compared to using separate constants, and given that they will be uppercase I don't think they will be mistaken with something else.

Still, if we decide to use enums I have two suggestions:

1. when defining RoleNames make sure you inherit str, this will shorten the comparisons as I explained in my comment
2. when iterating all of the roles make sure to do: for role in RoleNames instead of mentioning each of the roles explicitly.
   I left a comment about that too.

EDIT: I think you can directly just inherit StrEnum with the same success as inheriting Enum and str separatly.
For more info: https://docs.python.org/3/library/enum.html#others

[jku]

I don't have opinions on the actual data structure but I have a couple on the usage:

• a few legacy test files are modified: The split between legacy and metadata API is confusing so not your fault, but let's not start mixing the two implementations. Basically if the file did not import tuf.api.metadata already, it's probably legacy and we don't want to modify the file. so test_repository_tool, test_sig, test_updater should not be touched
• It's great that we try to avoid typos by avoiding hard-coded strings... but it should not come at at the expense of readability: I think going from "root" to RoleNames.ROOT.value is not a clear cut win as the latter is more than 3 times as long (!) and looks so complex -- I could not guess from the name that I'm looking at a simple role name.

[jku]

It seems like the major reason to use an enum or a class is to avoid top level variable name pollution (so to avoid from tuf.api.metadata import Root, ROOT, Timestamp, TIMESTAMP, ....), correct?

What if instead of a new class we used the classes we are already importing? Root and others already have a class variable _signed_type which is already exposed as an instance property type... Maybe we could just simplify that so the class variable is called type (and is public): then we could just refer to Root.type to get the string without instantiating a Root?

(for reference: the class variable _signed_type is probably currently private to make it very clear it should not be modified, but as python does not have "class properties" that means we can't keep using a property in this case(?). I think solving the problem described in this issue is more useful than keeping the type private)

[lukpueh]

What if instead of a new class we used the classes we are already importing? Root and others already have a class variable _signed_type which is already exposed as an instance property type... Maybe we could just simplify that so the class variable is called type (and is public): then we could just refer to Root.type to get the string without instantiating a Root?

This is what I meant above with ...

Another way of encapsulation would be to make them class attributes of the corresponding metadata classes.

One downside would be that we'd have to define a global TOP_LEVEL_ROLE_NAMES that uses these class variables after the class definitions, i.e. at the bottom of the metadata module. But I guess that's okay.

[jku]

One downside would be that we'd have to define a global TOP_LEVEL_ROLE_NAMES that uses these class variables after the class definitions, i.e. at the bottom of the metadata module.

I'm also not going to lose sleep over a single instance of duplicating the strings within the API implementation if that makes the API definition look neater: what's important is removing the magic strings from the code that imports tuf.api.metadata

[ivanayov]

I've put the constants within the role classes. Agree that it's more clear, simple and convenient.

As those are both required private (we don't want to be able to change them, whatever "be able" means in python for private properties :D ), but at the same time we need them for static reference within Updater, and we don't want to break the file format which uses _type, and don't want to do a breaking API change for such a simple reason with changing type to TYPE, we came to and agreement with @jku that changing _signed_type to type and keeping the _type property would be the best possible option.
Yep, it's still not very nice to reference the public variable from the private property, but that's the least painful and best to use change, unless someone comes with a genius idea that we cannot see yet.

[jku]

LGTM, left a couple of comments but I think none were blockers: please take a look and adapt or don't (leave a comment if you don't), either way works

[jku]

it's still not very nice to reference the public variable from the private property, but that's the least painful and best to use change, unless someone comes with a genius idea that we cannot see yet.

I think the genius idea is to just remove _type property: it's from the time when the API matching the file format was considered a bit more important than it is now. IMO the _type property is not worth the confusion it creates . But we can handle that in later PRs.

[lukpueh]

just remove _type property

Agreed!

it's from the time when the API matching the file format was considered a bit more important than it is now

I actually suspect that in this case the (Python) implementation influenced the naming decision in the spec, to avoid shadowing the type global on json-to-dict deserialisation.

Regardless of this specific case, I still think that it very important that the spec file format is recognisable in the reference implementation.

[ivanayov]

Thanks @jku. Just addressed your comments.