Enable mypy for ngclient

[sechkova]

Fixes #1409
Description of the changes being introduced by the pull request:

Enable static type checking for ngclient.

• add all missing type annotations
• fix errors (partially for updater.py and trusted_metadata_set.py
• add urllib3 to ignored includes
• download suggested stub packages when running mypy

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

[jku]

I think there's a few cases here:

• TrustedMetadataSet.root probably does not actually need to be Optional: Unlike other properties it's guaranteed to be a Metadata after constructor, right?
• mypy gets confused about target_base_url: I'm sure it's possible to simplify the code so mypy understands that target_base_url cannot be None
• cases where we know a e.g. self._trusted_set.snapshot is Metadata but code does not: I think this is a great place for assert(self._trusted_set.snapshot is not None)
• many cases of Metadata.Signed typing problem...
  • I believe this would get fixed with Improve signed typing #1457 and Generics
  • alternatively we could just use calls like assert(isinstance(self._trusted_set.snapshot.signed, Snapshot)). Ugly but works and there are surprisingly few places where this is needed (like 10-12 or so?)
  • even more alternatively we could add properties to TrustedMetadataSet that returned Root, Snapshot, Timestamp and Targets (but that makes a lot of the code a bit ugly as you usually need both the Metadata and the Signed subclass)...

First two-three can be fixed now. The last one we can discuss... In any case this is useful work as it shows how much (or little) the lack of signed typing affects us...

The download related the changes should maybe only be done after the cleanup is done in #1448 (also the SlowRetrievalError change looks a bit fishy -- I would expect it's used in the legacy code as well?)

[joshuagl]

• mypy gets confused about target_base_url: I'm sure it's possible to simplify the code so mypy understands that target_base_url cannot be None

It's debatable whether this simplifies things for humans reading the code (edit: though, apart from the variable name ugliness and possibility for confusion of tgt_base_url, I think it probably is simpler) but I was able to make mypy happy with the following:

        # locally scoped variable that is definitely a string, even if empty
        tgt_base_url: str = self._target_base_url or ""
        if target_base_url is not None:
            tgt_base_url = _ensure_trailing_slash(target_base_url)

        # move the check for a base URL being set to after the locally scoped
        # variable assignment logic is bottomed out.
        if not tgt_base_url:
            raise ValueError(
                "target_base_url must be set in either download_target() or "
                "constructor"
            )

see the full diff here.

[jku]

this should also work -- the issue for mypy is two branches that seem unrelated (even if they aren't) so making the branches actually same should make the intent clear to computer...

        if target_base_url is None:
            if self._target_base_url is None:
                raise ValueError(
                    "target_base_url must be set in either download_target() or "
                    "constructor"
                )
            target_base_url = self._target_base_url
        else:
            target_base_url = _ensure_trailing_slash(target_base_url)

[jku]

The download related the changes should maybe only be done after the cleanup is done in #1448 (also the SlowRetrievalError change looks a bit fishy -- I would expect it's used in the legacy code as well?)

#1448 is now merged

[sechkova]

Rebased on latest changes in develop and fixed all issues but the Metadata.Signed typing ones.

[sechkova]

Actually bandit complains slightly about asserts:

>> Issue: [B101:assert_used] Use of assert detected. The enclosed code will be removed when compiling to optimised byte code.
   Severity: Low   Confidence: High
   Location: tuf/ngclient/updater.py:297
   More Info: https://bandit.readthedocs.io/en/latest/plugins/b101_assert_used.html
296	
297	            assert self._trusted_set.snapshot is not None
298	            metainfo = self._trusted_set.snapshot.signed.meta[f"{role}.json"]

[joshuagl]

Actually bandit complains slightly about asserts

Right. We could reasonably silence those warnings, as we are only suggesting use of asserts to help mypy determine the expected type. But I think this further reinforces the 'code smell' of using asserts to solve this problem.

[sechkova]

Actually bandit complains slightly about asserts

Right. We could reasonably silence those warnings, as we are only suggesting use of asserts to help mypy determine the expected type. But I think this further reinforces the 'code smell' of using asserts to solve this problem.

In this case we use assert as the recommended way to deal with None errors. So we either silence bandit or tweak the TrustedMetadataSet public properties to not return optional values.

[sechkova]

I used the part of #1457 that makes Metadata 'Generic' to resolve the type checking issues related to the 'signed' type.
Now there are no more mypy errors left and the PR should be ready for review.

[joshuagl]

Thanks Teodora. This is looking good. I have a minor comment on the change to the root property, and a slight pushback on the type hinting for IDE's that I would appreciate your thoughts on before we merge.

[sechkova]

Seems like the CI won't be triggered until I rebase and resolve the conflicts.

[jku]

• add urllib3 to ignored includes

If you researched this can you document why? looking in urllib3 git the annotations seem to be there but is that just for some future 2.0 release?

• download suggested stub packages when running mypy

Could we add the required stub packages to test requirements instead?

[jku]

I think it looks good (but obviously the last commits depend on a commit from generics PR #1457 so I'm not approving).

This PR isn't using the whole Generics branch though: if it did then it could just remove all of the checks of type if new_root.signed.type != "root" since the constructor would do that already

The last commit is IMO not great since it has so much code in the try-block: it's not clear that the KeyError should only happen in self.root... I think the original code is better. Maybe the real alternative would be to not call update_root() from init but instead do the required steps in init itself -- that seems ok to me as well as I think there aren't that many steps here.

[sechkova]

I think it looks good (but obviously the last commits depend on a commit from generics PR #1457 so I'm not approving).

Hmm ... I wouldn't say directly depending, I didn't cherry-pick the commit from the other PR but created a new one and added you as a co-author.

This PR isn't using the whole Generics branch though: if it did then it could just remove all of the checks of type if new_root.signed.type != "root" since the constructor would do that already

This was done intentionally but I guess I should've tried to explain my intentions somewhere in written form.

The "Generics" branch tries to achieve two things simultaneously: correct type annotations of Signed and a runtime check ensuring content and type are really what we expect. Since there seems to be an agreement already on the use of Generics and an ongoing discussion around the runtime check part, I decided to include only the first part here, the one directly related to type annotations and mypy. This way we can leave the rest of the discussion for #1457 which will also make it simpler because it will try to solve only one problem. I hope you agree :)

[sechkova]

The last commit is IMO not great since it has so much code in the try-block: it's not clear that the KeyError should only happen in self.root... I think the original code is better. Maybe the real alternative would be to not call update_root() from init but instead do the required steps in init itself -- that seems ok to me as well as I think there aren't that many steps here.

I tried this alternative since @joshuagl expressed some concerns about not using the 'root' property consistently inside the class. The three options, none of them perfect, would be:

• don't call update_root from init, repeat some of the code (not than much repetition so could be acceptable)
• don't use the property but "get" safely from self._trusted_set c205ac0 (concerns about not using consistently the root property)
• use the root property, enclose it in a try bock and catch the KeyError 8c172af (well you already described the disadvantages)

There is a forth one where we keep root optional as the rest of the properties and add asserts in the code but I tried and this means asserts at 4-5 places which seems to rule out this option.

[jku]

I think it looks good (but obviously the last commits depend on a commit from generics PR #1457 so I'm not approving).

Hmm ... I wouldn't say directly depending, I didn't cherry-pick the commit from the other PR but created a new one and added you as a co-author.

This PR isn't using the whole Generics branch though: if it did then it could just remove all of the checks of type if new_root.signed.type != "root" since the constructor would do that already

This was done intentionally but I guess I should've tried to explain my intentions somewhere in written form.

Yeah I think the generics change is major enough that we can't just hide it in a PR titled "enable mypy for ngclient" :) We can definitely split the PR if that makes it easier to swallow though.

[joshuagl]

Yeah I think the generics change is major enough that we can't just hide it in a PR titled "enable mypy for ngclient" :) We can definitely split the PR if that makes it easier to swallow though.

Ack, let's do it.

[sechkova]

• add urllib3 to ignored includes

If you researched this can you document why? looking in urllib3 git the annotations seem to be there but is that just for some future 2.0 release?

Yes, the latest release is 1.26 which does not include the type annotations. I updated the commit description.

• download suggested stub packages when running mypy

Could we add the required stub packages to test requirements instead?

Done: d306370

[jku]

Ok, I've made the #1457 PR into a "minimal generics" PR -- this PR could be based off of that one I believe.

[sechkova]

Rebased on #1457 and I believe I have addressed all comments so waiting for another review.

[jku]

Looks good, updater.py in VSCode is much nicer now. I think this is good to go... except this will definitely clash with #1514 which I was about to merge...

[jku]

this is unfortunate... I don't have any better ideas but it looks so strange in this code.

This seems to only be needed in Updater for getting meta-info -- which is also a sort of duplicate piece of code here and in _load_targets()... so there's probably some sort of refactoring possibility here but let's not do that in this PR.

[sechkova]

Rebased on latest changes in develop.