Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve signed typing #1457

Merged
merged 1 commit into from
Aug 18, 2021
Merged

Improve signed typing #1457

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 12 additions & 12 deletions tests/test_api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ def test_to_from_bytes(self):

def test_sign_verify(self):
root_path = os.path.join(self.repo_dir, 'metadata', 'root.json')
root:Root = Metadata.from_file(root_path).signed
root = Metadata[Root].from_file(root_path).signed

# Locate the public keys we need from root
targets_keyid = next(iter(root.roles["targets"].keyids))
Expand Down Expand Up @@ -302,7 +302,7 @@ def test_metadata_base(self):
def test_metadata_snapshot(self):
snapshot_path = os.path.join(
self.repo_dir, 'metadata', 'snapshot.json')
snapshot = Metadata.from_file(snapshot_path)
snapshot = Metadata[Snapshot].from_file(snapshot_path)

# Create a MetaFile instance representing what we expect
# the updated data to be.
Expand All @@ -321,7 +321,7 @@ def test_metadata_snapshot(self):
def test_metadata_timestamp(self):
timestamp_path = os.path.join(
self.repo_dir, 'metadata', 'timestamp.json')
timestamp = Metadata.from_file(timestamp_path)
timestamp = Metadata[Timestamp].from_file(timestamp_path)

self.assertEqual(timestamp.signed.version, 1)
timestamp.signed.bump_version()
Expand Down Expand Up @@ -358,19 +358,19 @@ def test_metadata_timestamp(self):

def test_metadata_verify_delegate(self):
root_path = os.path.join(self.repo_dir, 'metadata', 'root.json')
root = Metadata.from_file(root_path)
root = Metadata[Root].from_file(root_path)
snapshot_path = os.path.join(
self.repo_dir, 'metadata', 'snapshot.json')
snapshot = Metadata.from_file(snapshot_path)
snapshot = Metadata[Snapshot].from_file(snapshot_path)
targets_path = os.path.join(
self.repo_dir, 'metadata', 'targets.json')
targets = Metadata.from_file(targets_path)
targets = Metadata[Targets].from_file(targets_path)
role1_path = os.path.join(
self.repo_dir, 'metadata', 'role1.json')
role1 = Metadata.from_file(role1_path)
role1 = Metadata[Targets].from_file(role1_path)
role2_path = os.path.join(
self.repo_dir, 'metadata', 'role2.json')
role2 = Metadata.from_file(role2_path)
role2 = Metadata[Targets].from_file(role2_path)

# test the expected delegation tree
root.verify_delegate('root', root)
Expand Down Expand Up @@ -468,7 +468,7 @@ def test_role_class(self):
def test_metadata_root(self):
root_path = os.path.join(
self.repo_dir, 'metadata', 'root.json')
root = Metadata.from_file(root_path)
root = Metadata[Root].from_file(root_path)

# Add a second key to root role
root_key2 = import_ed25519_publickey_from_file(
Expand Down Expand Up @@ -530,7 +530,7 @@ def test_delegation_class(self):
def test_metadata_targets(self):
targets_path = os.path.join(
self.repo_dir, 'metadata', 'targets.json')
targets = Metadata.from_file(targets_path)
targets = Metadata[Targets].from_file(targets_path)

# Create a fileinfo dict representing what we expect the updated data to be
filename = 'file2.txt'
Expand Down Expand Up @@ -560,7 +560,7 @@ def test_length_and_hash_validation(self):
# for untrusted metadata file to verify.
timestamp_path = os.path.join(
self.repo_dir, 'metadata', 'timestamp.json')
timestamp = Metadata.from_file(timestamp_path)
timestamp = Metadata[Timestamp].from_file(timestamp_path)
snapshot_metafile = timestamp.signed.meta["snapshot.json"]

snapshot_path = os.path.join(
Expand Down Expand Up @@ -603,7 +603,7 @@ def test_length_and_hash_validation(self):
# Test target files' hash and length verification
targets_path = os.path.join(
self.repo_dir, 'metadata', 'targets.json')
targets = Metadata.from_file(targets_path)
targets = Metadata[Targets].from_file(targets_path)
file1_targetfile = targets.signed.targets['file1.txt']
filepath = os.path.join(
self.repo_dir, 'targets', 'file1.txt')
Expand Down
39 changes: 30 additions & 9 deletions tuf/api/metadata.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,15 @@
BinaryIO,
ClassVar,
Dict,
Generic,
List,
Mapping,
Optional,
Tuple,
Type,
TypeVar,
Union,
cast,
)

from securesystemslib import exceptions as sslib_exceptions
Expand All @@ -56,24 +59,40 @@
# files to have the same major version (the first number) as ours.
SPECIFICATION_VERSION = ["1", "0", "19"]

# T is a Generic type constraint for Metadata.signed
T = TypeVar("T", "Root", "Timestamp", "Snapshot", "Targets")

class Metadata:

class Metadata(Generic[T]):
"""A container for signed TUF metadata.

Provides methods to convert to and from dictionary, read and write to and
from file and to create and verify metadata signatures.

Metadata[T] is a generic container type where T can be any one type of
[Root, Timestamp, Snapshot, Targets]. The purpose of this is to allow
static type checking of the signed attribute in code using Metadata::

root_md = Metadata[Root].from_file("root.json")
# root_md type is now Metadata[Root]. This means signed and its
# attributes like consistent_snapshot are now statically typed and the
# types can be verified by static type checkers and shown by IDEs
print(root_md.signed.consistent_snapshot)

Using a type constraint is not required but not doing so means T is not a
specific type so static typing cannot happen. Note that the type constraint
"[Root]" is not validated at runtime (as pure annotations are not available
then).

Attributes:
signed: A subclass of Signed, which has the actual metadata payload,
i.e. one of Targets, Snapshot, Timestamp or Root.
signatures: An ordered dictionary of keyids to Signature objects, each
signing the canonical serialized representation of 'signed'.
"""

def __init__(
self, signed: "Signed", signatures: "OrderedDict[str, Signature]"
):
self.signed = signed
def __init__(self, signed: T, signatures: "OrderedDict[str, Signature]"):
self.signed: T = signed
self.signatures = signatures

@classmethod
Expand Down Expand Up @@ -119,7 +138,8 @@ def from_dict(cls, metadata: Dict[str, Any]) -> "Metadata":
signatures[sig.keyid] = sig

return cls(
signed=inner_cls.from_dict(metadata.pop("signed")),
# Specific type T is not known at static type check time: use cast
signed=cast(T, inner_cls.from_dict(metadata.pop("signed"))),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I understand, you are using cast here to signal the type checker that you expect the type of the returned metadata.signed to be of type T
Wondering when does T gets actually assigned?

Copy link
Member Author

@jku jku Jul 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well, it's useful to remember that T and cast() have zero runtime effect: cast() is me promising to mypy (not cpython) that signed will be one of the types T once the code runs, and telling mypy to not worry about it. The cpython runtime doesn't know anything about T or that promise.

For the static typing to work __init__() / from_dict() themselves do not need to define what type T is: calling methods like Root.metadata_from_file() do define it (as the return value is Metadata[Root]).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can see this in action if you use the current constructor with this branch: Metadata.from_file() still works but return value is not statically typed with the correct type (it's just Metadata[T]) because there is nothing the static type check could use to figure it out.

conveniently at least VS Code offers me all the possible completions (Root,Targets,Snapshot,Timestamp) when T is not defined so that's a win-win.

signatures=signatures,
)

Expand All @@ -129,7 +149,7 @@ def from_file(
filename: str,
deserializer: Optional[MetadataDeserializer] = None,
storage_backend: Optional[StorageBackendInterface] = None,
) -> "Metadata":
) -> "Metadata[T]":
"""Loads TUF metadata from file storage.

Arguments:
Expand All @@ -156,11 +176,12 @@ def from_file(
with storage_backend.get(filename) as file_obj:
return cls.from_bytes(file_obj.read(), deserializer)

@staticmethod
@classmethod
def from_bytes(
cls,
data: bytes,
deserializer: Optional[MetadataDeserializer] = None,
) -> "Metadata":
) -> "Metadata[T]":
"""Loads TUF metadata from raw data.

Arguments:
Expand Down