Destroy unnecessary MetaFile classes in formats.py #836

awwad · 2019-03-15T21:31:39Z

This PR is related to two efforts:

The consolidation of metadata definitions and destruction of conflicting or redundant internal metadata representations, related to work being done for Roles and Delegations are still confused in parts of the implementation #660
Uptane PR #171 related to timeserver key rotation

The primary effect is to replace six classes and their associated methods and some related external functions with one, simpler, general purpose function in formats.py, build_dict_conforming_to_schema. These functions are involved in the production of metadata.

This PR also:

corrects a mistake in TIMESTAMP_SCHEMA, the exclusion of spec_version (since writing timestamp metadata previously did not properly respect this schema, written metadata still included spec_version)
Adds a notion of the version of the TUF specification that this reference implementation intends and is expected to support -- tuf.SPECIFICATION_VERSION

Details

Kills classes MetaFile, RootFile, TargetsFile, MirrorsFile, SnapshotFile, and TimestampFile. They each had an unused from_ method and a used make_ method. They were all additional, unnecessary representations of the same metadata, and it is very important that metadata formats be defined once in the reference implementation, in the schemas that are already used more broadly, in formats.py.

Replaces the classes, their methods, some related external functions, and some associated variables with a single short function called build_dict_conforming_to_schema that takes keyword arguments and builds a dictionary, then checks to make sure that the result conforms to the given schema.

This commit shifts repository_lib from use of the old classes to the new function.

In later commits, we should use this function more broadly, since it can be of use in all schema construction.

There are several TODOs added to the code, mostly for post-#660 tasks.

The code follows the Code Style Guidelines
Tests have been added for the bug fix or new feature
Docs have been added for the bug fix or new feature

Kills classes MetaFile, RootFile, TargetsFile, MirrorsFile, SnapshotFile, and TimestampFile. They each had an unused from_ method and a used make_ method. They were all additional, unnecessary representations of the same metadata, and it is very important that metadata formats be defined once in the reference implementation, in the schemas that are already used more broadly, in foramts.py. Replaces the classes, their methods, and some associated variables with a single short function called build_dict_conforming_to_schema that takes keyword arguments and builds a dictionary, then checks to make sure that the result conforms to the given schema. This commit shifts repository_lib from use of the old classes to the new function. In later commits, we should use this function more broadly, since it can be of use in all schema construction. There are several TODOs added to the code, mostly for post-#660 tasks. Signed-off-by: Sebastien Awwad <sebastien.awwad@gmail.com>

Add a tuf-level variable in tuf/__init__.py indicating the version of the TUF specification that the code in this repository is intended and expected to conform to. This will be used when writing metadata. Signed-off-by: Sebastien Awwad <sebastien.awwad@gmail.com>

It was excluded from the Timestamp schema definition in error. In the process of making metadata writing use the Timestamp schema strictly, this bug was discovered. Metadata previously written included specification version, but the schema check did not. Signed-off-by: Sebastien Awwad <sebastien.awwad@gmail.com>

It pertains to now-deleted metadata classes. Signed-off-by: Sebastien Awwad <sebastien.awwad@gmail.com>

Testing will now use (and test) build_dict_conforming_to_schema. Signed-off-by: Sebastien Awwad <sebastien.awwad@gmail.com>

awwad · 2019-03-29T15:39:53Z

Rebased after #844

awwad · 2019-03-29T16:57:41Z

See sister PR and review comments here in Uptane's TUF fork.

tuf.formats.make_role_metadata concerned itself with exclusivity checks for paths and path_hash_prefixes, but no code actually used it for relevant data. It's yet another custom metadata writer replaced by build_dict_conforming_to_schema. Signed-off-by: Sebastien Awwad <sebastien.awwad@gmail.com>

Make a copy of the provided fields so that the caller's provided values do not change when the returned values are changed. Signed-off-by: Sebastien Awwad <sebastien.awwad@gmail.com>

in tuf.formats.build_dict_conforming_to_schema Populate _type with the expected value for the given schema, and populate spec_version with tuf.SPECIFICATION_VERSION. Do this only when the values are not provided, and support overriding them. Also adds testing for the above and takes advantage of the above in repository_lib's _generate metadata functions. Signed-off-by: Sebastien Awwad <sebastien.awwad@gmail.com>

since make_role_metadata is being replaced by build_dict_conforming_to_schema Signed-off-by: Sebastien Awwad <sebastien.awwad@gmail.com>

awwad · 2019-03-29T19:41:31Z

I think I've addressed your comments on the sister (upTUF) PR, @lukpueh. I've also expanded this PR just a little bit. Should be a quick re-review, I hope. :)

Signed-off-by: Sebastien Awwad <sebastien.awwad@gmail.com>

lukpueh · 2019-04-01T14:04:28Z

tuf/formats.py

@@ -24,8 +24,7 @@
  module should be read and understood before tackling this module.

  'formats.py' can be broken down into three sections.  (1) Schemas and object


s/three/two/

lukpueh · 2019-04-01T14:14:55Z

tuf/formats.py

-    tuf.formats.TARGETS_SCHEMA.check_match(result)
+    else:
+      # If the field has not been provided and is required, check to see if
+      # the field is one of the one of the fields we automatically fill.


s/one of the one of the/one of the/

lukpueh · 2019-04-01T14:32:24Z

tuf/formats.py

+  #
+  for schema_element in schema._required: #pylint: disable=protected-access
+    key = schema_element[0]
+    element_type = schema_element[1]


Why check above if the schema has a check_match method and here just assume that it has a _required property, which itself and whose elements are subscriptable?

I recommend to get rid of above hasattr-check and instead use the commented out isinstance-check but with a schema.Object which should be guaranteed to have a _required-list and, as a child of schema.Schema, also a check_match-method.

Also, consider my comment in awwad#23, about how this function only makes sense for Object-type schemas.

Btw. you can unpack the _required-tuples in the loop definition, which imho looks nicer:

for key, element_type in schema._required: ...

Re. the additional typing requirements: I forgot to edit the type check after also requiring _required. You're right: I should switch to a type check now.

I'll add a comment noting that this is intended for Object-type schemas (which are for the most part the ones for which it's of interest).

On the 2-tuples: quite right, of course -- dunno why I didn't unpack that way. Fixing.

lukpueh · 2019-04-01T15:22:27Z

tuf/formats.py

+      elif (key == 'spec_version' and
+          element_type == SPECIFICATION_VERSION_SCHEMA):
+        # If not provided, use the specification version in tuf/__init__.py
+        dictionary[key] = tuf.SPECIFICATION_VERSION


I know, I suggested this, but now that I look at it I think it's pretty specific, at least for the function name build_dict_conforming_to_schema. Do you think there is a better name? If not, disregard this comment.

The function is still intended to be a general purpose (Object-type) schema builder, so I would rather remove this convenience functionality than change that scope. There may be a better name, but I don't want to narrow its purpose to Role metadata construction, if that's what you mean.

I think it's fine as it is.

lukpueh · 2019-04-01T15:27:24Z

tuf/repository_lib.py

+        tuf.formats.TARGETS_SCHEMA,
+        version=version,
+        expires=expiration_date,
+        targets=filedict)


So I couldn't sell the kwargs alternative to you? :)

Whatcha mean? The extras bit (e.g. delegations) in the calls below this one?

The function uses kwargs and returns deepcopy(kwargs) (after some modifications).

Gonna merge and use this in some other work, now that the rest has been addressed, but if I missed something, please LMK. I'll keep the extras-style optional arguments construction in mind if there are other places calls like this start to need to happen.

Oh yes, I was referring to passing something like an extra_kwargs that may or may not contain the delegations (sorry for being unclear).

lukpueh · 2019-04-01T15:39:03Z

tuf/formats.py

@@ -356,6 +348,7 @@
 TIMESTAMP_SCHEMA = SCHEMA.Object(
  object_name = 'TIMESTAMP_SCHEMA',
  _type = SCHEMA.String('timestamp'),
+  spec_version = SPECIFICATION_VERSION_SCHEMA,


Does this require new tests?

This in itself, no. The spec version was simply missing in the schema for Timestamp. It was already being written in metadata despite the fact that it was missing in the schema, because this schema was not really used to build the metadata before. Now the schema is used to build metadata.

I want to add more validation that uses these schemas in updater, and to add some additional testing for formats.py related to the schemas more generally, but that's not really related to this change, and it'll have to wait.

Signed-off-by: Sebastien Awwad <sebastien.awwad@gmail.com>

Edits prompted by Lukas's review. Signed-off-by: Sebastien Awwad <sebastien.awwad@gmail.com>

- Expand on the arguments/kwargs - Note that the function is particular to schema.Object objects. Signed-off-by: Sebastien Awwad <sebastien.awwad@gmail.com>

May the Python gods forgive me for having three different names with the same names and different casing on one line.... Perhaps I should address the import module as.... Signed-off-by: Sebastien Awwad <sebastien.awwad@gmail.com>

awwad · 2019-04-02T17:27:29Z

Thanks for the review, @lukpueh!

lukpueh

Thanks for addressing my comments. I think you want to do an isinstance-check on SCHEMA.Object in build_dict_conforming_to_schema. Otherwise it looks good.

lukpueh · 2019-04-03T10:03:00Z

tuf/formats.py

-    filedict = targets_metadata.get('targets')
-    delegations = targets_metadata.get('delegations')
+  # Check the schema argument type (must provide check_match and _required).
+  if not isinstance(schema, SCHEMA.Schema):


s/Schema/Object/

lukpueh · 2019-04-03T10:03:10Z

tuf/formats.py

+  # Check the schema argument type (must provide check_match and _required).
+  if not isinstance(schema, SCHEMA.Schema):
+    raise ValueError(
+        'The first argument must be a schema.Schema object, but is not. '


s/Schema/Object/

lukpueh · 2019-04-03T10:06:19Z

tuf/formats.py

+      elif (key == 'spec_version' and
+          element_type == SPECIFICATION_VERSION_SCHEMA):
+        # If not provided, use the specification version in tuf/__init__.py
+        dictionary[key] = tuf.SPECIFICATION_VERSION


I think it's fine as it is.

lukpueh · 2019-04-03T10:09:46Z

tuf/repository_lib.py

+        tuf.formats.TARGETS_SCHEMA,
+        version=version,
+        expires=expiration_date,
+        targets=filedict)


Oh yes, I was referring to passing something like an extra_kwargs that may or may not contain the delegations (sorry for being unclear).

Raise an error if it's not a schema.Object instance (not just if it's not a schema.Schema instance). Also adds a test for this. Signed-off-by: Sebastien Awwad <sebastien.awwad@gmail.com>

awwad mentioned this pull request Mar 18, 2019

Destroy MetaFile classes with fire awwad/tuf#23

Merged

awwad force-pushed the destroy_unnecessary_classes_in_formats branch 3 times, most recently from 8c0338a to 1b994fd Compare March 28, 2019 23:14

awwad added 5 commits March 29, 2019 11:36

Destroy get_role_class in formats.py (not needed)

ce21d8d

It pertains to now-deleted metadata classes. Signed-off-by: Sebastien Awwad <sebastien.awwad@gmail.com>

Update testing following MetaFile(etc) class removals

e7c8229

Testing will now use (and test) build_dict_conforming_to_schema. Signed-off-by: Sebastien Awwad <sebastien.awwad@gmail.com>

awwad force-pushed the destroy_unnecessary_classes_in_formats branch from 1b994fd to e7c8229 Compare March 29, 2019 15:39

awwad requested a review from lukpueh March 29, 2019 16:55

awwad added 3 commits March 29, 2019 15:22

Perform deep copies in build_dict_conforming_to_schema

491577c

Make a copy of the provided fields so that the caller's provided values do not change when the returned values are changed. Signed-off-by: Sebastien Awwad <sebastien.awwad@gmail.com>

awwad force-pushed the destroy_unnecessary_classes_in_formats branch from bf84ad5 to 7ecf522 Compare March 29, 2019 19:23

Use build_dict... instead of make_role_... in test_sig.py

3ac174f

since make_role_metadata is being replaced by build_dict_conforming_to_schema Signed-off-by: Sebastien Awwad <sebastien.awwad@gmail.com>

Beg pylint for forgiveness re. protected access of SCHEMAs

b52ff2d

Signed-off-by: Sebastien Awwad <sebastien.awwad@gmail.com>

lukpueh requested changes Apr 1, 2019

View reviewed changes

awwad added 4 commits April 2, 2019 12:46

minor DOC: typo fixes

40f4202

Signed-off-by: Sebastien Awwad <sebastien.awwad@gmail.com>

PR revision: use type checking, minor readability edit

915291e

Edits prompted by Lukas's review. Signed-off-by: Sebastien Awwad <sebastien.awwad@gmail.com>

DOC: improve build_dict_conforming_to_schema docstring

188bc84

- Expand on the arguments/kwargs - Note that the function is particular to schema.Object objects. Signed-off-by: Sebastien Awwad <sebastien.awwad@gmail.com>

PR revision: minor bugfix in casing

9aa2246

May the Python gods forgive me for having three different names with the same names and different casing on one line.... Perhaps I should address the import module as.... Signed-off-by: Sebastien Awwad <sebastien.awwad@gmail.com>

lukpueh requested changes Apr 3, 2019

View reviewed changes

PR revision: test build_dict_conforming... arg for schema type

25aa71d

Raise an error if it's not a schema.Object instance (not just if it's not a schema.Schema instance). Also adds a test for this. Signed-off-by: Sebastien Awwad <sebastien.awwad@gmail.com>

lukpueh approved these changes Apr 3, 2019

View reviewed changes

awwad merged commit 254e0c4 into develop Apr 3, 2019

awwad mentioned this pull request Apr 3, 2019

(WIP/Collaboration) rewrites for delegation graph issues #846

Closed

awwad deleted the destroy_unnecessary_classes_in_formats branch May 2, 2019 16:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Destroy unnecessary MetaFile classes in formats.py #836

Destroy unnecessary MetaFile classes in formats.py #836

awwad commented Mar 15, 2019 •

edited

Loading

awwad commented Mar 29, 2019

awwad commented Mar 29, 2019 •

edited

Loading

awwad commented Mar 29, 2019 •

edited

Loading

lukpueh Apr 1, 2019

lukpueh Apr 1, 2019

lukpueh Apr 1, 2019

lukpueh Apr 1, 2019

awwad Apr 1, 2019

lukpueh Apr 1, 2019

awwad Apr 2, 2019

lukpueh Apr 3, 2019

lukpueh Apr 1, 2019

awwad Apr 1, 2019

awwad Apr 1, 2019

awwad Apr 2, 2019

lukpueh Apr 3, 2019

lukpueh Apr 1, 2019

awwad Apr 2, 2019

awwad commented Apr 2, 2019

lukpueh left a comment

lukpueh Apr 3, 2019

lukpueh Apr 3, 2019

lukpueh Apr 3, 2019

lukpueh Apr 3, 2019

		@@ -24,8 +24,7 @@
		module should be read and understood before tackling this module.

		'formats.py' can be broken down into three sections. (1) Schemas and object

Destroy unnecessary MetaFile classes in formats.py #836

Destroy unnecessary MetaFile classes in formats.py #836

Conversation

awwad commented Mar 15, 2019 • edited Loading

Details

awwad commented Mar 29, 2019

awwad commented Mar 29, 2019 • edited Loading

awwad commented Mar 29, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awwad commented Apr 2, 2019

lukpueh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awwad commented Mar 15, 2019 •

edited

Loading

awwad commented Mar 29, 2019 •

edited

Loading

awwad commented Mar 29, 2019 •

edited

Loading