Metadata Attribute research: version

[MVrachev]

This issue aims to document thoughts about the version attribute from the Signed class in order
to understand how we use that attribute, what might go wrong with it and how we can validate it.

We want to answer/address the following 6 questions/points based on my comment here:

1. How do we use it?
2. What might go wrong?
3. Rethink how we store it?
4. If, after the changes in step 3 there are concerns, then add a validation function
5. Make the necessary changes for points 1 - 4, think and propose a way to integrate it into the class/classes (decorators, descriptors, etc.)
6. create a pr for all additional changes on top of the validation functions

PS: The 7-th point is covered by documenting this issue.

[MVrachev]

My initial comments on those 6 steps:

1. How do we use it?

• Initialize it through init.
• We can change it through bump_version, but with a fixed amount of 1.

2. What might go wrong?

• Pass a non-positive version during initialization.
• Pass a float or bool version during initialization, but we want exactly an int.
• If you have a JSON with { "version": true }, then when reading that data with json.load, data[“version”] will be an instance of int!
  Modification or deletion of the version attribute directly from the metadata API, like this “metadata.signed.version = -10”

3. Rethink how we store it?

• version is mutable, but at the same time, we want to help the user not change the version directly like metadata.signed.version = -10. I didn’t found a way to forbid access to the version outside the class, but I found a way to enforce validation.
  We can achieve this by making version a property that sets a private version _version and makes sure it validates it before setting it.
  Sadly, I didn’t find a way to block the user from changing _version directly like: metadata.signed._version = "BAD".
  At least if the user is accessing the self._version he would know he is accessing a private field and it’s his responsibility.

4. If, after the changes in step 3 there are concerns, then add a validation function

• We will need a simple validation function making sure that validation is an instance exactly of int and that it’s above 0 during initialization.

5. Make the necessary changes for points 1 - 4, think and propose a way to integrate it into the class/classes (decorators, descriptors, etc.)

• The validation function could be called into the version.setter (version is defined as a property) just before assigning the value to “_version”.
  That way we make sure we won’t forget to validate the value when setting _version.

6. create a pr for all additional changes on top of the validation functions

• I made a simple commit where small prototypes for the different attributes can be seen https://github.com/MVrachev/tuf/commits/checks

[MVrachev]

@jku comments

What might go wrong?

the core concerns are handling invalid data from the network, making sure our data is spec-compliant, and ease of use:

• checking n > 0 is reasonable

• fighting the type system I'm not convinced about. It's more code that does not seem to give us much (the JSON may think version is a bool but we won't, for us, it's a 1)
• the check should be done in the deserialization case... as Teodora points out the "ease-of-use" case seems to be taken care of by bump_version so I think adding more checks is a bit much...
• in any case, I think a specific validation function is not needed if the validation code is short and there is only one place where that code is needed
• handling float also seems like a red herring: floats are not ints

Additionally, on this sentence shared by me:

Sadly, I didn’t find a way to block the user from changing _version directly like: metadata.signed._version = “2.10.10.
At least if the user is accessing the self._version he would know he is accessing a private field and it’s his responsibility.

@jku commented:

This is not the way: don't try to prevent someone from abusing the API. It's impossible.
Correct use should be easy, and incorrect use hard... and that's it.

[MVrachev]

Rethink how we store it?
version is mutable, but at the same time, we want to help the user not change the version directly like metadata.signed.version = -10. I didn’t found a way to forbid access to the version outside the class, but I found a way to enforce validation.
We can achieve this by making version a property that sets a private version _version and makes sure it validates it before setting it.

@sechkova commented addressing my sentences above:

Given that bump_version exists, why should we provide a public setter for the version property?
This makes bump_version sort of useless.
I think making it private as you say in the next sentence makes sense and then adding the validation calls to init and bump_version.

I commented back that:

I have to make a choice:

1. Either I make "version" a property with a setter and thus I automatically call 
"_validate_version()" on the assignment
2. or I make "version" a read-only property, but remember to manually call
"_validate_version" everywhere I change "version". Here, actually, 
I won't change "self.version", but will actually change "self._version".

I cannot have both, because when I make "version" a property
I actually return "self._version" and I have to leave a way to change "self._version"
(for example when calling self.bump_version()).

The question is which one is better for "version"?

@sechkova response was:

But we are talking internal calls inside the classes here.
You cannot expect to protect a "private" (in quotes because of python) class variable from access inside the class.
We should focus on the boundaries. So I guess this means my vote is for option 1.

[MVrachev]

That way we make sure we won’t forget to validate the value when setting _version.

@joshuagl commented on the quoted sentence shared by me:

If we're adding a setter for version, does it make sense to not only ensure it's > 0 but also that it's >= the current value?

I answered:

It won't be hard to add that check the question is should we do that check there? 
This is something manually checked in the MetadataBundle already. 
It's a separate sub-step in the update process for each of the metadata files in the spec. 
Probably it's good to leave that check there, so it's visible as a separate step.

[MVrachev]

All of the participants in this discussion discussed this again and decided we won't make additional changes for version considering that there is a public API function to change it - bump_version().