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

Jump to bottom

Add exceptions docs for __init__ and from_dict() #1820

Merged
merged 1 commit into from
Feb 8, 2022
Merged

Add exceptions docs for __init__ and from_dict() #1820

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
96 changes: 84 additions & 12 deletions tuf/api/metadata.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,8 +124,7 @@ def from_dict(cls, metadata: Dict[str, Any]) -> "Metadata[T]":
metadata: TUF metadata in dict representation.

Raises:
KeyError: The metadata dict format is invalid.
ValueError: The metadata has an unrecognized signed._type field.
ValueError, KeyError, TypeError: Invalid arguments.

Side Effect:
Destroys the metadata dict passed by reference.
Expand Down Expand Up @@ -416,6 +415,9 @@ class Signed(metaclass=abc.ABCMeta):
spec_version: The supported TUF specification version number.
expires: The metadata expiry date.
unrecognized_fields: Dictionary of all unrecognized fields.

Raises:
ValueError: Invalid arguments.
Copy link
Member

Choose a reason for hiding this comment

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

Can you remind me, do we expect users to catch type errors by verifying our type annotations? If not, we'd also need to expect an AttributeError here in case the spec_version passed to the constructor does not have a split method, which we call in:

python-tuf/tuf/api/metadata.py

Line 440 in dd59ada

spec_list = spec_version.split(".")

OTOH, I noticed a few isinstance-checks in other constructors in the module, that we probably wouldn't need if we relied on users running mypy, e.g.:

python-tuf/tuf/api/metadata.py

Lines 549 to 558 in dd59ada

keyid: str,
keytype: str,
scheme: str,
keyval: Dict[str, str],
unrecognized_fields: Optional[Mapping[str, Any]] = None,
):
if not all(
isinstance(at, str) for at in [keyid, keytype, scheme]
) or not isinstance(keyval, dict):
raise TypeError("Unexpected Key attributes types!")

I guess the difference between those two example that in the former a user sees the issue right away, but not in the latter.

We don't have to solve this in this PR, I was just curious.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think we have a strong stance on when should we check for types.
I think I agree that in the Key example it's harder to notice that these fields are not strings.
I don't think we can rely on our users mypy as it's not an official requirement to run python-tuf, but at the same time, we don't want to overdo it with the type checks as we are working with untyped language.
@jku any opinion on this?

Copy link
Member

Choose a reason for hiding this comment

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

I noticed a few isinstance-checks in other constructors in the module, that we probably wouldn't need if we relied on users running mypy

For deserialization code path (including constructors as that's how the code path is built, for better or worse) we can't tell users to rely on mypy: the input is after all just random objects coming from json.loads().

I think in these cases documenting TypeError is as useful as documenting ValueError and KeyError... That is "not very useful at all" but if we do it we should try to have full coverage.

"""

# type is required for static reference without changing the API
Expand Down Expand Up @@ -551,6 +553,9 @@ class Key:
"rsassa-pss-sha256", "ed25519", and "ecdsa-sha2-nistp256".
keyval: Opaque key content
unrecognized_fields: Dictionary of all unrecognized fields.

Raises:
TypeError: Invalid type for an argument.
"""

def __init__(
Expand All @@ -573,7 +578,11 @@ def __init__(

@classmethod
def from_dict(cls, keyid: str, key_dict: Dict[str, Any]) -> "Key":
"""Creates Key object from its dict representation."""
"""Creates Key object from its dict representation.

Raises:
KeyError, TypeError: Invalid arguments.
"""
keytype = key_dict.pop("keytype")
scheme = key_dict.pop("scheme")
keyval = key_dict.pop("keyval")
Expand Down Expand Up @@ -680,6 +689,9 @@ class Role:
keyids: The roles signing key identifiers.
threshold: Number of keys required to sign this role's metadata.
unrecognized_fields: Dictionary of all unrecognized fields.

Raises:
ValueError: Invalid arguments.
"""

def __init__(
Expand All @@ -698,7 +710,11 @@ def __init__(

@classmethod
def from_dict(cls, role_dict: Dict[str, Any]) -> "Role":
"""Creates Role object from its dict representation."""
"""Creates Role object from its dict representation.

Raises:
ValueError, KeyError: Invalid arguments.
"""
keyids = role_dict.pop("keyids")
threshold = role_dict.pop("threshold")
# All fields left in the role_dict are unrecognized.
Expand Down Expand Up @@ -727,6 +743,9 @@ class Root(Signed):
required to sign the metadata for a specific role.
consistent_snapshot: Does repository support consistent snapshots.
unrecognized_fields: Dictionary of all unrecognized fields.

Raises:
ValueError: Invalid arguments.
"""

type = _ROOT
Expand All @@ -752,7 +771,11 @@ def __init__(

@classmethod
def from_dict(cls, signed_dict: Dict[str, Any]) -> "Root":
"""Creates Root object from its dict representation."""
"""Creates Root object from its dict representation.

Raises:
ValueError, KeyError, TypeError: Invalid arguments.
"""
common_args = cls._common_fields_from_dict(signed_dict)
consistent_snapshot = signed_dict.pop("consistent_snapshot", None)
keys = signed_dict.pop("keys")
Expand Down Expand Up @@ -902,6 +925,9 @@ class MetaFile(BaseFile):
hashes: Dictionary of hash algorithm names to hashes of the metadata
file content.
unrecognized_fields: Dictionary of all unrecognized fields.

Raises:
ValueError, TypeError: Invalid arguments.
"""

def __init__(
Expand All @@ -926,7 +952,11 @@ def __init__(

@classmethod
def from_dict(cls, meta_dict: Dict[str, Any]) -> "MetaFile":
"""Creates MetaFile object from its dict representation."""
"""Creates MetaFile object from its dict representation.

Raises:
ValueError, KeyError: Invalid arguments.
"""
version = meta_dict.pop("version")
length = meta_dict.pop("length", None)
hashes = meta_dict.pop("hashes", None)
Expand Down Expand Up @@ -981,6 +1011,9 @@ class Timestamp(Signed):
expires: The metadata expiry date.
unrecognized_fields: Dictionary of all unrecognized fields.
snapshot_meta: Meta information for snapshot metadata.

Raises:
ValueError: Invalid arguments.
"""

type = _TIMESTAMP
Expand All @@ -998,7 +1031,11 @@ def __init__(

@classmethod
def from_dict(cls, signed_dict: Dict[str, Any]) -> "Timestamp":
"""Creates Timestamp object from its dict representation."""
"""Creates Timestamp object from its dict representation.

Raises:
ValueError, KeyError: Invalid arguments.
"""
common_args = cls._common_fields_from_dict(signed_dict)
meta_dict = signed_dict.pop("meta")
snapshot_meta = MetaFile.from_dict(meta_dict["snapshot.json"])
Expand Down Expand Up @@ -1026,6 +1063,9 @@ class Snapshot(Signed):
expires: The metadata expiry date.
unrecognized_fields: Dictionary of all unrecognized fields.
meta: A dictionary of target metadata filenames to MetaFile objects.

Raises:
ValueError: Invalid arguments.
"""

type = _SNAPSHOT
Expand All @@ -1043,7 +1083,11 @@ def __init__(

@classmethod
def from_dict(cls, signed_dict: Dict[str, Any]) -> "Snapshot":
"""Creates Snapshot object from its dict representation."""
"""Creates Snapshot object from its dict representation.

Raises:
ValueError, KeyError: Invalid arguments.
"""
common_args = cls._common_fields_from_dict(signed_dict)
meta_dicts = signed_dict.pop("meta")
meta = {}
Expand Down Expand Up @@ -1086,6 +1130,9 @@ class DelegatedRole(Role):
paths: Path patterns. See note above.
path_hash_prefixes: Hash prefixes. See note above.
unrecognized_fields: Attributes not managed by TUF Metadata API.

Raises:
ValueError: Invalid arguments.
"""

def __init__(
Expand Down Expand Up @@ -1119,7 +1166,11 @@ def __init__(

@classmethod
def from_dict(cls, role_dict: Dict[str, Any]) -> "DelegatedRole":
"""Creates DelegatedRole object from its dict representation."""
"""Creates DelegatedRole object from its dict representation.

Raises:
ValueError, KeyError: Invalid arguments.
"""
name = role_dict.pop("name")
keyids = role_dict.pop("keyids")
threshold = role_dict.pop("threshold")
Expand Down Expand Up @@ -1218,6 +1269,9 @@ class Delegations:
role. The roles order also defines the order that role delegations
are considered during target searches.
unrecognized_fields: Dictionary of all unrecognized fields.

Raises:
ValueError: Invalid arguments.
"""

def __init__(
Expand All @@ -1239,7 +1293,11 @@ def __init__(

@classmethod
def from_dict(cls, delegations_dict: Dict[str, Any]) -> "Delegations":
"""Creates Delegations object from its dict representation."""
"""Creates Delegations object from its dict representation.

Raises:
ValueError, KeyError, TypeError: Invalid arguments.
"""
keys = delegations_dict.pop("keys")
keys_res = {}
for keyid, key_dict in keys.items():
Expand Down Expand Up @@ -1277,6 +1335,9 @@ class TargetFile(BaseFile):
file content.
path: URL path to a target file, relative to a base targets URL.
unrecognized_fields: Dictionary of all unrecognized fields.

Raises:
ValueError, TypeError: Invalid arguments.
"""

def __init__(
Expand All @@ -1303,7 +1364,11 @@ def custom(self) -> Any:

@classmethod
def from_dict(cls, target_dict: Dict[str, Any], path: str) -> "TargetFile":
"""Creates TargetFile object from its dict representation."""
"""Creates TargetFile object from its dict representation.

Raises:
ValueError, KeyError, TypeError: Invalid arguments.
"""
length = target_dict.pop("length")
hashes = target_dict.pop("hashes")

Expand Down Expand Up @@ -1422,6 +1487,9 @@ class Targets(Signed):
delegations: Defines how this Targets delegates responsibility to other
Targets Metadata files.
unrecognized_fields: Dictionary of all unrecognized fields.

Raises:
ValueError: Invalid arguments.
"""

type = _TARGETS
Expand All @@ -1442,7 +1510,11 @@ def __init__(

@classmethod
def from_dict(cls, signed_dict: Dict[str, Any]) -> "Targets":
"""Creates Targets object from its dict representation."""
"""Creates Targets object from its dict representation.

Raises:
ValueError, KeyError, TypeError: Invalid arguments.
"""
common_args = cls._common_fields_from_dict(signed_dict)
targets = signed_dict.pop(_TARGETS)
try:
Expand Down