ngclient: Avoid loading targets metadata twice #1593

sechkova · 2021-09-27T14:00:19Z

Description of the changes being introduced by the pull request:
When traversing the delegations tree looking for targets, avoid re-loading already verified targets metadata.

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

coveralls · 2021-09-27T14:29:08Z

Pull Request Test Coverage Report for Build 1470873479

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

5 of 5 (100.0%) changed or added relevant lines in 1 file are covered.
6 unchanged lines in 1 file lost coverage.
Overall coverage increased (+1.0%) to 98.472%

Files with Coverage Reduction	New Missed Lines	%
tuf/ngclient/updater.py	6	95.22%

Totals
Change from base Build 1470751886:	1.0%
Covered Lines:	3283
Relevant Lines:	3303

💛 - Coveralls

jku · 2021-09-28T06:36:02Z

Did you consider if we could use RepositorySimulator to test this (to continue avoiding the actual web server tests)?

A similar mocking approach could work or this could even be a feature in RespositorySimulator if you think there might be more demand for it

EDIT: actually the second loading of course loads the local metadata only so the repository side is irrelevant and your mocking should work as is in a RepositorySimulator test

sechkova · 2021-09-28T10:07:07Z

Did you consider if we could use RepositorySimulator to test this (to continue avoiding the actual web server tests)?

It has to be extended a bit for delegations but why not doing it now.

jku · 2021-09-29T08:16:37Z

We discussed this already but documenting here: An implementation of delegation support for RepositorySimulator exists but not in develop yet

The options for this PR are:

merge as is with expectation that we update the tests when the delegation support is in
leave this on back burner until delegation support is in

I think the latter is fine: this is not an urgent change

jku · 2021-10-20T08:41:53Z

okay, current RepositorySimulator should have basic delegation support now

jku · 2021-10-22T08:00:27Z

tests/test_updater_ng.py

+        with patch.object(
+            self.repository_updater,
+            "_load_targets",
+            wraps=self.repository_updater._load_targets,
+        ) as wrapped_load_targets:


I think this could mock builtins.open to actually test for file loads: the test wouldn't then depend so much on knowledge of updater implementation details.

It's not a major thing I guess: If mocking open() is significantly more complex then let's not do it.

sechkova · 2021-11-16T14:41:11Z

Added the test in test_updater_with_simulator until a better place comes up (#1641).

sechkova · 2021-11-16T16:12:56Z

Now these changes are not compatible with #1680 ...

jku

I like it, mocking open() feels like the correct thing to check for and is readable.

LGTM to merge. Left two comments for you to decide.

jku · 2021-11-17T08:11:43Z

tuf/ngclient/updater.py

-            self._load_targets(role_name, parent_role)
+            # The metadata for 'role_name' must be loaded
+            # before its targets and delegations can be inspected.
+            if not self._trusted_set.get(role_name):


more pythonic IMO:

Suggested change

if not self._trusted_set.get(role_name):

if role_name not in self._trusted_set:

yeah, very pythonic :) it is updated now.

jku · 2021-11-17T08:17:06Z

tests/test_updater_with_simulator.py

+        # Run refresh, top-level roles are loaded
+        updater = self._run_refresh()
+        # Clean up calls to open during refresh()
+        wrapped_open.reset_mock()


you're adding a test for a specific bug fix, that makes sense... but this obviously leads to "shouldn't we check that refresh opens correct files the correct number of times as well?" Maybe a follow up issue if you don't feel like handling here?

I was planning to open an issue about testing the loading of metadata. Doing it now ... #1681

jku · 2021-11-18T08:38:14Z

Now these changes are not compatible with #1680 ...

oof, I just understood what you mean (originally thought you just referred to a plain merge conflict)... this change looks trivial but with that other PR it means you need to handle the else-case in _preorder_depth_first_walk() as well making a it a bit ugly. Let's make sure the approach is reasonable WRT 1680 before merging this.

I think that _preorder_depth_first_walk() should not have to handle these different cases (it's complex enough):
So should _load_targets() handle the "already loaded" case and return the cached one if it is already loaded? I think that makes sense: the top-level metadata is "immutable" so if we have loaded a specific targets, it is the correct one. It'll mean _load_targets() operates a little differently than other _load_*() but I think that's natural: loading targets is a special case as we do it implicitly during get_targetinfo().

jku

marking "request changes" for the merge conflict plus previous comment

When traversing the delegations tree looking for targets, avoid re-loading already verified targets metadata. Signed-off-by: Teodora Sechkova <tsechkova@vmware.com>

sechkova · 2021-11-23T10:27:59Z

It should be ready now.

jku

LGTM, thanks

sechkova changed the title ~~Avoid loading targets metadata twice~~ ngclient: Avoid loading targets metadata twice Sep 27, 2021

sechkova marked this pull request as draft October 1, 2021 12:37

jku reviewed Oct 22, 2021

View reviewed changes

sechkova force-pushed the avoid_reloading_targets branch 2 times, most recently from f86b045 to 3f6b439 Compare November 16, 2021 14:39

sechkova marked this pull request as ready for review November 16, 2021 14:39

jku approved these changes Nov 17, 2021

View reviewed changes

sechkova force-pushed the avoid_reloading_targets branch from 3f6b439 to 1792f1e Compare November 17, 2021 09:23

sechkova mentioned this pull request Nov 17, 2021

ngtests: test loading of cached metadata #1681

Closed

jku self-requested a review November 23, 2021 10:11

jku requested changes Nov 23, 2021

View reviewed changes

Avoid loading targets metadata twice

4db4737

When traversing the delegations tree looking for targets, avoid re-loading already verified targets metadata. Signed-off-by: Teodora Sechkova <tsechkova@vmware.com>

sechkova force-pushed the avoid_reloading_targets branch from 1792f1e to 4db4737 Compare November 23, 2021 10:24

jku approved these changes Nov 24, 2021

View reviewed changes

jku merged commit a24c4e9 into theupdateframework:develop Nov 24, 2021

lukpueh mentioned this pull request Dec 13, 2021

Prepare v0.20.0 #1723

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ngclient: Avoid loading targets metadata twice #1593

ngclient: Avoid loading targets metadata twice #1593

sechkova commented Sep 27, 2021

coveralls commented Sep 27, 2021 •

edited

Loading

jku commented Sep 28, 2021 •

edited

Loading

sechkova commented Sep 28, 2021

jku commented Sep 29, 2021

jku commented Oct 20, 2021

jku Oct 22, 2021 •

edited

Loading

sechkova commented Nov 16, 2021

sechkova commented Nov 16, 2021

jku left a comment

jku Nov 17, 2021

sechkova Nov 17, 2021

jku Nov 17, 2021 •

edited

Loading

sechkova Nov 17, 2021 •

edited

Loading

jku commented Nov 18, 2021 •

edited

Loading

jku left a comment

sechkova commented Nov 23, 2021

jku left a comment

	if not self._trusted_set.get(role_name):
	if role_name not in self._trusted_set:

ngclient: Avoid loading targets metadata twice #1593

ngclient: Avoid loading targets metadata twice #1593

Conversation

sechkova commented Sep 27, 2021

coveralls commented Sep 27, 2021 • edited Loading

Pull Request Test Coverage Report for Build 1470873479

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

jku commented Sep 28, 2021 • edited Loading

sechkova commented Sep 28, 2021

jku commented Sep 29, 2021

jku commented Oct 20, 2021

jku Oct 22, 2021 • edited Loading

Choose a reason for hiding this comment

sechkova commented Nov 16, 2021

sechkova commented Nov 16, 2021

jku left a comment

Choose a reason for hiding this comment

jku Nov 17, 2021

Choose a reason for hiding this comment

sechkova Nov 17, 2021

Choose a reason for hiding this comment

jku Nov 17, 2021 • edited Loading

Choose a reason for hiding this comment

sechkova Nov 17, 2021 • edited Loading

Choose a reason for hiding this comment

jku commented Nov 18, 2021 • edited Loading

jku left a comment

Choose a reason for hiding this comment

sechkova commented Nov 23, 2021

jku left a comment

Choose a reason for hiding this comment

coveralls commented Sep 27, 2021 •

edited

Loading

jku commented Sep 28, 2021 •

edited

Loading

jku Oct 22, 2021 •

edited

Loading

jku Nov 17, 2021 •

edited

Loading

sechkova Nov 17, 2021 •

edited

Loading

jku commented Nov 18, 2021 •

edited

Loading