Metadata API: include target target name in TargetFile #1514

MVrachev · 2021-08-03T14:00:51Z

Related to #1411, but it's not yet clear if it closes it.

Description of the changes being introduced by the pull request:

Currently, TargetFile instances do not contain the filename of the file
they represent. The API itself does not need it but it could be useful
for users of the API.

As an example, the current client returns a dict for
get_one_valid_targetinfo(): that dict contains a filepath field and
a targetinfo field (essentially TargetFile).
We would like to keep a similar API, but avoid hand-crafted dicts.
It would be much nicer to return a TargetFile that would contain the
full "metadata" of the targetfile.

Signed-off-by: Martin Vrachev [email protected]

Please verify and check that the pull request fulfills the following
requirements:

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

tuf/ngclient/updater.py

sechkova · 2021-08-03T14:59:50Z

tuf/api/metadata.py

        unrecognized_fields: Dictionary of all unrecognized fields.
    """

    def __init__(
        self,
        length: int,
        hashes: Dict[str, str],
+        targetname: Optional[str] = None,


I'm not a fan ofTargetFile.targetname. How about TargetFile.filename, TargetFile.path or TargetFile.targetpath if we insist on being verbose and stick to the spec.

(this is just a drive by comment, will have to think about the justification for thw whole thing a bit still... but thanks for starting this, I think it's useful to make discussion more concrete)

What teodora said, let's not use even more different names for the same thing... I think "targetpath" is used for this already so the attribute should probably be targetpath or path.

I believe it should not be optional though. Or is there a use case of a TargetFile without a targetpath?

I'm not a fan of TargetFile.targetname. How about TargetFile.filename, TargetFile.path or TargetFile.targetpath if we insist on being verbose and stick to the spec.

I had a misconception and didn't want to call it targetpath thinking it's not an actual path, but a file name.
Now, I read the spec and it clearly this is called TARGETPATH:
https://theupdateframework.github.io/specification/latest/#targetpath.

So, from both options to call it targetpath or path I would prefer path given it's shorter.

I believe it should not be optional though. Or is there a use case of a TargetFile without a targetpath?

I wasn't sure when I added it as optional. I did that because it's not required by the spec, but we can use TargetFile
in such a way that we would always pass the path.

So, in conclusion, I will rename the attribute to path and make it non-optional.

jku · 2021-08-06T07:06:36Z

I think this makes sense and implementation looks fine. You will have to do something about Targets.update() though (I still think removing it is correct -- at least currently it seems to have no purpose other than creating apparent complexity -- but I guess fixing it to match the new API is an option as well)

Maybe it would be good to document target path a little more: that it is a "path-relative-URL string" that will be used as part of the final download url to fetch the target

The high level question is whether anyone objects to making objects like this a bit smarter (in this case be aware of the path) even if the file format does not include that. I think this is a good way forward and seems immediately understandable by the reader -- it definitely makes the Updater API much better...

jku · 2021-08-09T12:58:19Z

For those not following that closely: this PR changes TargetFile interface (which on it's own is a minor change in the Metadata API implementation) but also the whole ngclient API so that TargetFile is returned and expected as argument instead of an undefined dictionary.

I think this is going in the right direction and allows us to continue with something like #1317 (comment): I believe the core Updater should not handle caching: this would make it much clearer what is actually part of the spec implementation and what is an additional thing we just wanted to implement in the same source directory

joshuagl · 2021-08-10T09:31:18Z

The high level question is whether anyone objects to making objects like this a bit smarter (in this case be aware of the path) even if the file format does not include that. I think this is a good way forward and seems immediately understandable by the reader -- it definitely makes the Updater API much better...

I do not object, in fact looking at it now having an object which describes a file without being able to explicitly identify the file does look odd. +1 from me.

jku · 2021-08-10T10:58:24Z

@MVrachev can you suggest a solution for Targets.update(): at the very least the function arguments need to be updated.

jku

marking changes requested for Targets.update() issue

MVrachev · 2021-08-17T11:03:42Z

I had to rebase to fix merge conflicts after the simplification of _preorder_depth_first_walk.
I updated the Targets.update() argument accordingly.
I will prefer if other changes or removal of the update method are done for all classes when resolving #1230.

jku

Looks nice. I left a couple of comments and a question.

tuf/ngclient/updater.py

tuf/api/metadata.py

Currently, TargetFile instances do not contain the path relative URL of the file they represent. The API itself does not need it but it could be useful for users of the API. As an example, the current client returns a dict for get_one_valid_targetinfo(): that dict contains a filepath field and a targetinfo field (essentially TargetFile). We would like to keep a similar API, but avoid hand-crafted dicts. It would be much nicer to return a TargetFile that would contain the full "metadata" of the targetfile. Signed-off-by: Martin Vrachev <[email protected]>

After the addition of "path" argument in the TargetFile class the filename argument in Targets.update() became redundant. Signed-off-by: Martin Vrachev <[email protected]>

MVrachev changed the title ~~Metadata API: include target name in targetfile~~ Metadata API: include target target name in TargetFile Aug 3, 2021

sechkova reviewed Aug 3, 2021

View reviewed changes

MVrachev force-pushed the filename-in-targetfile branch from ef5cce9 to 3c4d5dd Compare August 4, 2021 13:42

sechkova mentioned this pull request Aug 6, 2021

ngclient: review preorder dfs code #1463

Merged

3 tasks

jku requested changes Aug 17, 2021

View reviewed changes

MVrachev force-pushed the filename-in-targetfile branch from 3c4d5dd to 4969ad3 Compare August 17, 2021 11:01

jku added this to the Sprint 6 milestone Aug 18, 2021

MVrachev force-pushed the filename-in-targetfile branch from 4969ad3 to 440040c Compare August 18, 2021 19:53

jku reviewed Aug 26, 2021

View reviewed changes

tuf/ngclient/updater.py Show resolved Hide resolved

tuf/ngclient/updater.py Outdated Show resolved Hide resolved

tuf/api/metadata.py Outdated Show resolved Hide resolved

MVrachev force-pushed the filename-in-targetfile branch from 440040c to a98bdc0 Compare August 26, 2021 12:04

MVrachev requested a review from jku August 26, 2021 12:10

jku approved these changes Aug 26, 2021

View reviewed changes

tuf/api/metadata.py Outdated Show resolved Hide resolved

MVrachev added 2 commits August 26, 2021 19:48

Remove filename argument from Targets.update()

9229a40

After the addition of "path" argument in the TargetFile class the filename argument in Targets.update() became redundant. Signed-off-by: Martin Vrachev <[email protected]>

MVrachev force-pushed the filename-in-targetfile branch from a98bdc0 to 9229a40 Compare August 26, 2021 16:48

jku merged commit 7731738 into theupdateframework:develop Aug 27, 2021

MVrachev mentioned this pull request Aug 27, 2021

client refactor: consider API changes #1317

Closed

MVrachev deleted the filename-in-targetfile branch September 1, 2021 11:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Metadata API: include target target name in TargetFile #1514

Metadata API: include target target name in TargetFile #1514

MVrachev commented Aug 3, 2021 •

edited

Loading

sechkova Aug 3, 2021

jku Aug 4, 2021

MVrachev Aug 4, 2021 •

edited

Loading

jku commented Aug 6, 2021

jku commented Aug 9, 2021 •

edited

Loading

joshuagl commented Aug 10, 2021

jku commented Aug 10, 2021

jku left a comment

MVrachev commented Aug 17, 2021 •

edited

Loading

jku left a comment

Metadata API: include target target name in TargetFile #1514

Metadata API: include target target name in TargetFile #1514

Conversation

MVrachev commented Aug 3, 2021 • edited Loading

sechkova Aug 3, 2021

Choose a reason for hiding this comment

jku Aug 4, 2021

Choose a reason for hiding this comment

MVrachev Aug 4, 2021 • edited Loading

Choose a reason for hiding this comment

jku commented Aug 6, 2021

jku commented Aug 9, 2021 • edited Loading

joshuagl commented Aug 10, 2021

jku commented Aug 10, 2021

jku left a comment

Choose a reason for hiding this comment

MVrachev commented Aug 17, 2021 • edited Loading

jku left a comment

Choose a reason for hiding this comment

MVrachev commented Aug 3, 2021 •

edited

Loading

MVrachev Aug 4, 2021 •

edited

Loading

jku commented Aug 9, 2021 •

edited

Loading

MVrachev commented Aug 17, 2021 •

edited

Loading