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

Metadata API: Improve metadata.signed "typing" #1433

Closed
jku opened this issue Jun 3, 2021 · 2 comments · Fixed by #1457
Closed

Metadata API: Improve metadata.signed "typing" #1433

jku opened this issue Jun 3, 2021 · 2 comments · Fixed by #1457
Assignees
@jku
Milestone
Sprint 5

Comments

@jku
Copy link
Member

jku commented Jun 3, 2021 •
edited
Loading

There's two case I'd like to see improved in metadata signed handling:

  1. Usually I call Metadata factory constructors (e.g. Metadata.from_bytes()) knowing what signed type should be, yet I can't enforce this
  2. Using metadata.signed in a type annotated fashion (so that it's annotated as the actual type: Root/Snapshot/Timestamp/Targets) would be fantastic.

Item 1 could be fixed with just adding an optional type argument to the constructors (Metadata.from_bytes(data, type=Root)) and raising if it does not match.

But I wonder if some Python wizard could fix both by making Metadata generic over Root/Snapshot/Timestamp/Targets:

try:
    root = Metadata[Root].from_bytes(data)
except DeserializationError as e:
    # this happens (among other things) if signed type does not match Root
    raise exceptions.RepositoryError("Failed to load root") from e 

for key in root.signed.keys.items():
    # MAGIC!: keys, key and keytype are all annotated here because root
    # is Metadata[Root] and so root.signed is Root
    print(key.keytype)
@jku jku mentioned this issue Jun 3, 2021
Merged
@jku
Copy link
Member Author

jku commented Jun 15, 2021 •
edited
Loading

so this does not work:

root = Metadata[Root].from_bytes(data)

because the "specific generic type" (Root in this case) is not an actual type available during runtime at all: there is no way to affect runtime based on that type (e.g. to raise an exception if it does not match).

It might still work if the type is given as parameter or the method contains it as constant (since then we obviously have the type available at runtime):

root = Metadata.from_bytes(data, signed_type=Root)
# or
root = Metadata.root_from_bytes(data)

The signatures for those alternative could be:

Metadata.from_bytes(data: bytes, signed_type: Optional[Type[T]] = None) -> Metadata[T]
Metadata.root_from_bytes(data: bytes) -> Metadata[Root]

and I think here A) metadata.signed could be correctly typed and B) we could raise if the signed type does not match. Will have to test this...

jku pushed a commit to jku/python-tuf that referenced this issue Jun 20, 2021
Metadata API: Make Metadata Generic
b7e8d88 
The purpose is two-fold:
1. When we deserialize metadata, we usually know what signed type we
   expect: make it easy to enforce that
2. When we use Metadata, it is helpful if the specific signed type (and
   all of the classes attribute types are correctly annotated

Making Metadata Generic over T, where

    T = TypeVar("T", "Root", "Timestamp", "Snapshot", "Targets")

allows both of these cases to work. Using Generics is completely
optional so all existing code still works. For case 1, the following
calls will now raise a Deserialization error if the expected type is
incorrect:

    md = Metadata.from_bytes(data, signed_type=Snapshot)
    md = Metadata.from_file(filename, signed_type=Snapshot)

For case 2, the return value md of those calls is now of type
"Metadata[Snapshot]", and md.signed is now of type "Snapshot" allowing
IDE annotations and static type checking.

Adding a type argument is an unconventional way to do this: the reason
for it is that the specific type (e.g. Snapshot) is not otherwise
available at runtime. A call like this works fine and md is annotated:

    md = Metadata[Snapshot].from_bytes(data)

but it's not possible to validate that "data" contains a "Snapshot",
because the value "Snapshot" is not defined at runtime at all, it is
purely an annotation. So an actual argument is needed.

Fixes theupdateframework#1433

Signed-off-by: Jussi Kukkonen <jkukkonen@vmware.com>
jku pushed a commit to jku/python-tuf that referenced this issue Jun 21, 2021
Metadata API: Make Metadata Generic
0baff95 
The purpose is two-fold:
1. When we deserialize metadata, we usually know what signed type we
   expect: make it easy to enforce that
2. When we use Metadata, it is helpful if the specific signed type (and
   all of the classes attribute types are correctly annotated

Making Metadata Generic over T, where

    T = TypeVar("T", "Root", "Timestamp", "Snapshot", "Targets")

allows both of these cases to work. Using Generics is completely
optional so all existing code still works. For case 1, the following
calls will now raise a Deserialization error if the expected type is
incorrect:

    md = Metadata.from_bytes(data, signed_type=Snapshot)
    md = Metadata.from_file(filename, signed_type=Snapshot)

For case 2, the return value md of those calls is now of type
"Metadata[Snapshot]", and md.signed is now of type "Snapshot" allowing
IDE annotations and static type checking.

Adding a type argument is an unconventional way to do this: the reason
for it is that the specific type (e.g. Snapshot) is not otherwise
available at runtime. A call like this works fine and md is annotated:

    md = Metadata[Snapshot].from_bytes(data)

but it's not possible to validate that "data" contains a "Snapshot",
because the value "Snapshot" is not defined at runtime at all, it is
purely an annotation. So an actual argument is needed.

Fixes theupdateframework#1433

Signed-off-by: Jussi Kukkonen <jkukkonen@vmware.com>
@jku jku mentioned this issue Jun 21, 2021
Merged
2 tasks
@jku
Copy link
Member Author

jku commented Jun 22, 2021

Forgot to mention here: there's a linked PR that implements the
Metadata.from_bytes(data: bytes, signed_type: Optional[Type[T]] = None) -> Metadata[T]
version.

It's not the prettiest thing in the world with the type argument but implementation is not too complex and using it feels quite nice.

@sechkova sechkova added this to the weeks26-27 milestone Jun 23, 2021
@joshuagl joshuagl added the backlog Issues to address with priority for current development goals label Jul 7, 2021
@joshuagl joshuagl modified the milestones: weeks26-27, Sprint 4 Jul 7, 2021
@joshuagl joshuagl removed the backlog Issues to address with priority for current development goals label Jul 7, 2021
@joshuagl joshuagl modified the milestones: Sprint 4, Sprint 5 Jul 28, 2021
jku pushed a commit to jku/python-tuf that referenced this issue Aug 16, 2021
Metadata API: Make Metadata Generic
0e644c1 
When we use Metadata, it is helpful if the specific signed type (and all of
the signed types attribute types are correctly annotated. Currently this is
not possible.

Making Metadata Generic with constraint T, where

    T = TypeVar("T", "Root", "Timestamp", "Snapshot", "Targets")

allows these annotations. Using Generic annotations is completely
optional so all existing code still works -- the changes in test code
are done to make IDE annotations more useful in the test code, not
because they are required.

Examples:

    md = Metadata[Root].from_bytes(data)
    md:Root = Metadata.from_bytes(data)

In both examples md.signed is now statically typed as "Root" allowing IDE
annotations and static type checking by mypy.

Note that it's not possible to validate that "data" actually contains a
root metadata at runtime in these examples as the annotations are _not_
visible at runtime at all: new constructors would have to be added for that.

Partially fixes theupdateframework#1433

Signed-off-by: Jussi Kukkonen <jkukkonen@vmware.com>
jku pushed a commit to jku/python-tuf that referenced this issue Aug 16, 2021
Metadata API: Make Metadata Generic
d9fbb55 
When we use Metadata, it is helpful if the specific signed type (and all of
the signed types attribute types are correctly annotated. Currently this is
not possible.

Making Metadata Generic with constraint T, where

    T = TypeVar("T", "Root", "Timestamp", "Snapshot", "Targets")

allows these annotations. Using Generic annotations is completely
optional so all existing code still works -- the changes in test code
are done to make IDE annotations more useful in the test code, not
because they are required.

Examples:

    md = Metadata[Root].from_bytes(data)
    md:Metadata[Root] = Metadata.from_bytes(data)

In both examples md.signed is now statically typed as "Root" allowing IDE
annotations and static type checking by mypy.

Note that it's not possible to validate that "data" actually contains a
root metadata at runtime in these examples as the annotations are _not_
visible at runtime at all: new constructors would have to be added for that.

Partially fixes theupdateframework#1433

Signed-off-by: Jussi Kukkonen <jkukkonen@vmware.com>
jku pushed a commit to jku/python-tuf that referenced this issue Aug 16, 2021
Metadata API: Make Metadata Generic
2aeed7d 
When we use Metadata, it is helpful if the specific signed type (and all of
the signed types attribute types are correctly annotated. Currently this is
not possible.

Making Metadata Generic with constraint T, where

    T = TypeVar("T", "Root", "Timestamp", "Snapshot", "Targets")

allows these annotations. Using Generic annotations is completely
optional so all existing code still works -- the changes in test code
are done to make IDE annotations more useful in the test code, not
because they are required.

Examples:

    md = Metadata[Root].from_bytes(data)
    md:Metadata[Root] = Metadata.from_bytes(data)

In both examples md.signed is now statically typed as "Root" allowing IDE
annotations and static type checking by mypy.

Note that it's not possible to validate that "data" actually contains a
root metadata at runtime in these examples as the annotations are _not_
visible at runtime at all: new constructors would have to be added for that.

Partially fixes theupdateframework#1433

Signed-off-by: Jussi Kukkonen <jkukkonen@vmware.com>
jku pushed a commit to jku/python-tuf that referenced this issue Aug 16, 2021
Metadata API: Make Metadata Generic
13e20e9 
When we use Metadata, it is helpful if the specific signed type (and all of
the signed types attribute types are correctly annotated. Currently this is
not possible.

Making Metadata Generic with constraint T, where

    T = TypeVar("T", "Root", "Timestamp", "Snapshot", "Targets")

allows these annotations. Using Generic annotations is completely
optional so all existing code still works -- the changes in test code
are done to make IDE annotations more useful in the test code, not
because they are required.

Examples:

    md = Metadata[Root].from_bytes(data)
    md:Metadata[Root] = Metadata.from_bytes(data)

In both examples md.signed is now statically typed as "Root" allowing IDE
annotations and static type checking by mypy.

Note that it's not possible to validate that "data" actually contains a
root metadata at runtime in these examples as the annotations are _not_
visible at runtime at all: new constructors would have to be added for that.

from_file() is now a class method like from_bytes() to make sure both
have the same definition of "T" when from_file() calls from_bytes():
This makes mypy happy.

Partially fixes theupdateframework#1433

Signed-off-by: Jussi Kukkonen <jkukkonen@vmware.com>
@jku jku closed this as completed in #1457 Aug 18, 2021
mnm678 pushed a commit to mnm678/tuf that referenced this issue Aug 24, 2021
@mnm678
Metadata API: Make Metadata Generic
34a055e 
When we use Metadata, it is helpful if the specific signed type (and all of
the signed types attribute types are correctly annotated. Currently this is
not possible.

Making Metadata Generic with constraint T, where

    T = TypeVar("T", "Root", "Timestamp", "Snapshot", "Targets")

allows these annotations. Using Generic annotations is completely
optional so all existing code still works -- the changes in test code
are done to make IDE annotations more useful in the test code, not
because they are required.

Examples:

    md = Metadata[Root].from_bytes(data)
    md:Metadata[Root] = Metadata.from_bytes(data)

In both examples md.signed is now statically typed as "Root" allowing IDE
annotations and static type checking by mypy.

Note that it's not possible to validate that "data" actually contains a
root metadata at runtime in these examples as the annotations are _not_
visible at runtime at all: new constructors would have to be added for that.

from_file() is now a class method like from_bytes() to make sure both
have the same definition of "T" when from_file() calls from_bytes():
This makes mypy happy.

Partially fixes theupdateframework#1433

Signed-off-by: Jussi Kukkonen <jkukkonen@vmware.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jku jku

Labels
None yet
Projects
None yet
Milestone
Sprint 5
Development

Successfully merging a pull request may close this issue.

Improve signed typing jku/python-tuf
3 participants
@jku @joshuagl @sechkova