Metadata Attribute research: version #1418

Activity

1. MVrachev

   added

   backlogIssues to address with priority for current development goalsIssues to address with priority for current development goals

   on May 26, 2021
2. sechkova

   added this to the weeks22-23 milestone on May 26, 2021
3. sechkova

   added

   discussionDiscussions related to the design, implementation and operation of the projectDiscussions related to the design, implementation and operation of the project

   on May 26, 2021
4. sechkova

   assigned

   MVrachev

   on May 26, 2021
5. MVrachev commented on May 26, 2021

   

   MVrachev

   on May 26, 2021

   Last edited by MVrachev

   CollaboratorAuthor

   More actions

   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
6. MVrachev commented on May 26, 2021

   

   MVrachev

   on May 26, 2021

   Last edited by MVrachev

   CollaboratorAuthor

   More actions

   @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.
7. MVrachev commented on May 26, 2021

   

   MVrachev

   on May 26, 2021

   Last edited by MVrachev

   CollaboratorAuthor

   More actions

   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.
8. MVrachev commented on May 26, 2021

   

   MVrachev

   on May 26, 2021

   CollaboratorAuthor

   More actions

   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.
   
9. MVrachev commented on Jun 1, 2021

   

   MVrachev

   on Jun 1, 2021

   CollaboratorAuthor

   More actions

   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().
10. MVrachev

    closed this as completedon Jun 1, 2021
11. MVrachev

    added 3 commits that reference this issue on Jun 1, 2021

    Remove question about version validation place

    ...

    b39f198

    Remove question about version validation place

    ...

    4596f91

    Remove question about version validation place

    ...

    15eb0d9