Destroy MetaFile classes with fire #23

awwad · 2019-03-15T19:42:45Z

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.

This PR 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 PRs, we should use this function more broadly, since it can be of use in all schema construction.

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. 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-18T18:09:24Z

All local upTUF tests expected to succeed do; Uptane tests continue to succeed when using this PR for their TUF dependency; the Uptane demo continues to run correctly.

I have to get back to the third timeserver rotation PR -- which is pressing -- using this, so while this should be reviewed, it can be done post-merge.

The sister PR in the proper TUF repository is here, needs a little bit more prodding, and will not be merged until it is finished and reviewed.

lukpueh

LGTM! See inline comments for a few suggestions/questions. I should mention that, while I thoroughly reviewed tuf/formats.py and tuf/repository_lib.py, I only skimmed the tests.

lukpueh · 2019-03-26T09:11:24Z

tuf/repository_lib.py

+  #       There are very few things that really need to be done differently.
+  return tuf.formats.build_dict_conforming_to_schema(
+      tuf.formats.ROOT_SCHEMA,
+      _type='Root',   # TODO: Does this have to be capitalized? -.-


The spec suggests lower-case (see e.g. 4.3. File formats: root.json), but the reference implementation mandates a capitalized _type (see e.g. ROOT_SCHEMA in formats.py).

Just a thought, would it make sense to make _type optional in build_dict_conforming_to_schema and use the one defined in the schema object, if it isn't passed?

On casing: the casing is at odds with the same content in theupdateframework/tuf. The modern reference implementation mandates lower-cased _type. I'm grumbling about that.

On _type being optional: I thought about something like that, but I'd like build_dict_conforming_to_schema to remain general enough to be used for every schema construction, and code handling _type specially feels like cheating. If the long list of function call arguments becomes an issue, then it might be wise to do this.

Re casing: I did have the feeling that your # TODO-question was rhetorical. :)

Re _type: You're right, I didn't think of other schemas. Though it might be okay to restrict this (or an additional wrapper) function to schemas that have a _type field (i.e. {ROOT, TARGETS, SNAPSHOT, TIMESTAMP}_SCHEMA). What do you think?

Note that the function as it currently is is already restricted to a subset of schemas, i.e. Object-type schemas...

>>> from securesystemslib.formats import HEX_SCHEMA >>> >>> HEX_SCHEMA.matches("beef") True >>> >>> build_dict_conforming_to_schema(HEX_SCHEMA, "beef") # [...] TypeError: build_dict_conforming_to_schema() takes 1 positional argument but 2 were given >>> >>> build_dict_conforming_to_schema(HEX_SCHEMA, dead="beef") # [...] securesystemslib.exceptions.FormatError: {'dead': 'beef'} did not match 'pattern /[a-fA-F0-9]+$/'

Obviously not a blocker ... especially since the PR is already merged :) ... just some thoughts...

lukpueh · 2019-03-26T09:34:26Z

tuf/formats.py

-  def from_metadata(object):
-    raise NotImplementedError
+  for key, value in kwargs.items():
+    d[key] = value


What's the reason of copying kwargs? Why not just:

schema.check_match(kwargs) return kwargs

If you want to cut ties to any passed references the loop won't do, as it only creates a shallow copy. So if creating a full is a requirement, you should maybe use copy.deepcopy?

Hm. That is quite pretty, you're right. I think I'll go with return deepcopy(kwargs).

lukpueh · 2019-03-26T09:37:14Z

tuf/formats.py

+  # if not isinstance(schema, schema.Schema):
+  #   raise ValueError(
+  #       'The first argument must be a schema.Schema object, but is not. '
+  #       'Given schema: ' + repr(schema))


Why do you favor the duck type check over the strict type check?

Just trying to be a good Python boy, I guess. /: All I really require is that whatever the object is supports check_match, and so that is all I'm asking for. I included the alternative code because it's a principle I'm less than sure about. :) What do you think?

AFAIK the good Python boy "doesn't ask for permission but for forgiveness":

try: schema.check_match(kwargs) except AttributeError: raise ValueError("...")

lukpueh · 2019-03-26T09:39:45Z

tuf/formats.py

@@ -684,161 +673,41 @@ def make_metadata(version, expiration_date, filedict):



Any reason you did not nuke TimestampFile and its from_metadata and make_metadata methods?

In the PR description you say you did.

Thanks -- missed this. Adding in another PR.

lukpueh · 2019-03-26T09:43:02Z

tuf/formats.py

-
-
+  Checks the result to make sure that it conforms to the given schema, raising
+  an error if not.


This does not really conform with our requirements for (API) function docstrings. However, I think it is informative enough.

lukpueh · 2019-03-26T10:08:59Z

tuf/repository_lib.py

+  #       not to include the delegations argument based on whether or not it is
+  #       None, consider instead adding a check in
+  #       build_dict_conforming_to_schema that skips a keyword if that keyword
+  #       is optional in the schema and the value passed in is set to None....


Here's another alternative:

... extra = {} if delegations is not None: extra = {"delegations": delegations} return tuf.formats.build_dict_conforming_to_schema( tuf.formats.TARGETS_SCHEMA, _type='Targets', version=version, expires=expiration_date, targets=filedict, **extra)

Hm. That's interesting. That might be a little too magic for this reference implementation. I do like it, though. Let's see if the need comes up more often.

lukpueh · 2019-03-26T10:23:24Z

tests/test_formats.py

+
+    # TODO: Later on, write a test looper that takes pairs of key-value args
+    #       to substitute in on each run to shorten this.... There's a lot of
+    #       test code that looks like this, and it'd be easier to use a looper.


Agreed! :)
I recently started to do a lot more table-driven testing (see in-toto/test_gpg.py for an example).

It's good stuff. I have some stuff like that in Uptane, in test_secondary.py for example. Can't remember where else.

@lukpueh

missed from the prior PR (github.com//pull/23) Thanks for spotting this, @lukpueh Signed-off-by: Sebastien Awwad <sebastien.awwad@gmail.com>

awwad added 3 commits March 15, 2019 15:35

Destroy get_role_class in formats.py (not needed)

2f53736

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

Update testing following MetaFile(etc) class removals

0c672fd

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

awwad requested a review from lukpueh March 18, 2019 18:01

awwad merged commit 90ee24a into develop Mar 18, 2019

lukpueh reviewed Mar 26, 2019

View reviewed changes

awwad added a commit that referenced this pull request Mar 28, 2019

Remove additional MetaFile class from tuf.formats,

3b6d682

missed from the prior PR (github.com//pull/23) Thanks for spotting this, @lukpueh Signed-off-by: Sebastien Awwad <sebastien.awwad@gmail.com>

This was referenced Mar 28, 2019

Remove additional MetaFile class from tuf.formats, #25

Merged

Destroy unnecessary MetaFile classes in formats.py theupdateframework/python-tuf#836

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Destroy MetaFile classes with fire #23

Destroy MetaFile classes with fire #23

awwad commented Mar 15, 2019

awwad commented Mar 18, 2019

lukpueh left a comment

lukpueh Mar 26, 2019

lukpueh Mar 26, 2019

awwad Mar 28, 2019

lukpueh Mar 29, 2019

lukpueh Mar 26, 2019

awwad Mar 29, 2019

lukpueh Mar 26, 2019

awwad Mar 26, 2019

lukpueh Mar 27, 2019

lukpueh Mar 26, 2019

lukpueh Mar 26, 2019

awwad Mar 28, 2019

lukpueh Mar 26, 2019

lukpueh Mar 26, 2019

awwad Mar 29, 2019

lukpueh Mar 26, 2019

awwad Mar 28, 2019

		@@ -684,161 +673,41 @@ def make_metadata(version, expiration_date, filedict):



		Checks the result to make sure that it conforms to the given schema, raising
		an error if not.

Destroy MetaFile classes with fire #23

Destroy MetaFile classes with fire #23

Conversation

awwad commented Mar 15, 2019

awwad commented Mar 18, 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

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