Page MenuHomeSoftware Heritage

common/utils: Ensure str type for query parameters in reverse function
ClosedPublic

Authored by anlambert on Jun 9 2022, 11:35 AM.

Details

Summary

It prevents possible issues in URL generation when providing a query parameter
that is not of string type.

See for instance bug fixed in rDWAPPS1c512a8365afb9485e9ac202046fd1ca4223a3c4
that could have been spotted by mypy if such rule was in place.

Depends on D7973

Diff Detail

Repository
rDWAPPS Web applications
Branch
reverse-ensure-str-query-params
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 29822
Build 46610: Phabricator diff pipeline on jenkinsJenkins console · Jenkins
Build 46609: arc lint + arc unit

Event Timeline

Build is green

Patch application report for D7974 (id=28728)

Could not rebase; Attempt merge onto 72b6022e09...

Updating 72b6022e..298d00b7
Fast-forward
 assets/src/bundles/origin/visits-reporting.js |   4 +-
 swh/web/api/views/add_forge_now.py            |   9 +-
 swh/web/api/views/origin.py                   |   2 +-
 swh/web/api/views/snapshot.py                 |   4 +-
 swh/web/browse/browseurls.py                  |  12 +-
 swh/web/browse/identifiers.py                 |   9 +-
 swh/web/browse/snapshot_context.py            | 109 ++++++++----
 swh/web/browse/urls.py                        |  11 +-
 swh/web/browse/utils.py                       | 238 ++++++++++++++++----------
 swh/web/browse/views/content.py               |  46 ++---
 swh/web/browse/views/directory.py             |  44 +++--
 swh/web/browse/views/origin.py                |  56 ++++--
 swh/web/browse/views/release.py               |  12 +-
 swh/web/browse/views/revision.py              |  39 +++--
 swh/web/browse/views/snapshot.py              |  34 ++--
 swh/web/common/identifiers.py                 |  23 ++-
 swh/web/common/typing.py                      |  20 +--
 swh/web/common/utils.py                       |  14 +-
 swh/web/misc/iframe.py                        |   4 +-
 19 files changed, 421 insertions(+), 269 deletions(-)
Changes applied before test
commit 298d00b7180462161ecda73ad681207b4e3b1234
Author: Antoine Lambert <anlambert@softwareheritage.org>
Date:   Fri Jun 3 13:48:45 2022 +0200

    common/utils: Ensure str type for query parameters in reverse function
    
    It prevents possible issues in URL generation when providing a query parameter
    that is not of string type.
    
    See for instance bug fixed in rDWAPPS1c512a8365afb9485e9ac202046fd1ca4223a3c4
    that could have been spotted by mypy if such rule was in place.

commit efa8ab0f9117a86923d67119d2b1928a977e1e53
Author: Antoine Lambert <anlambert@softwareheritage.org>
Date:   Wed Jun 8 14:47:52 2022 +0200

    browse: Add typing to view function signatures
    
    Because parameters of view functions for browse web application were
    not typed, mypy was not processing the body of those functions and
    thus typing errors could be missed.
    
    So add typing to these function signatures and fix new mypy errors now
    new code is processed.

See https://jenkins.softwareheritage.org/job/DWAPPS/job/tests-on-diff/1893/ for more details.

vlorentz added inline comments.
swh/web/common/identifiers.py
111–112

is this still relevant?

swh/web/common/utils.py
103–105

ditto

swh/web/common/identifiers.py
111–112

Yes because query_params can be None and is defined as Mapping[str, str] (to fix mypy errors about Dict variance).

We need to work with a Dict[str, str] in that function or mypy is not happy.

However the call to sorted is redundant with what the reverse function does.

swh/web/common/utils.py
103–105

same as above plus here we ensure parameters are sorted by their names.

Rebase and remove redundant sorted call

Build is green

Patch application report for D7974 (id=28740)

Rebasing onto b54d930bed...

Current branch diff-target is up to date.
Changes applied before test
commit 9dc7b88e392dcd263664433b03efceecfd1063b6
Author: Antoine Lambert <anlambert@softwareheritage.org>
Date:   Fri Jun 3 13:48:45 2022 +0200

    common/utils: Ensure str type for query parameters in reverse function
    
    It prevents possible issues in URL generation when providing a query parameter
    that is not of string type.
    
    See for instance bug fixed in rDWAPPS1c512a8365afb9485e9ac202046fd1ca4223a3c4
    that could have been spotted by mypy if such rule was in place.

See https://jenkins.softwareheritage.org/job/DWAPPS/job/tests-on-diff/1895/ for more details.

swh/web/common/identifiers.py
111–112

then this:

diff
diff --git a/swh/web/common/identifiers.py b/swh/web/common/identifiers.py
index b6e6cd4a..295a7592 100644
--- a/swh/web/common/identifiers.py
+++ b/swh/web/common/identifiers.py
@@ -106,13 +106,10 @@ def resolve_swhid(
     object_id = swhid_parsed.object_id
     browse_url = None
     url_args = {}
-    query_dict = {}
     fragment = ""
     process_lines = object_type == ObjectType.CONTENT
 
-    if query_params and len(query_params) > 0:
-        for k in query_params.keys():
-            query_dict[k] = query_params[k]
+    query_dict: Dict[str, str] = dict(query_params or {})
 
     if swhid_parsed.origin:
         origin_url = unquote(swhid_parsed.origin)

Simplify some code

swh/web/common/identifiers.py
111–112

Nice, thanks !

Build is green

Patch application report for D7974 (id=28741)

Rebasing onto b54d930bed...

Current branch diff-target is up to date.
Changes applied before test
commit 70d9b5653f63b87664b1dfac190cb330000b730d
Author: Antoine Lambert <anlambert@softwareheritage.org>
Date:   Fri Jun 3 13:48:45 2022 +0200

    common/utils: Ensure str type for query parameters in reverse function
    
    It prevents possible issues in URL generation when providing a query parameter
    that is not of string type.
    
    See for instance bug fixed in rDWAPPS1c512a8365afb9485e9ac202046fd1ca4223a3c4
    that could have been spotted by mypy if such rule was in place.

See https://jenkins.softwareheritage.org/job/DWAPPS/job/tests-on-diff/1896/ for more details.

vlorentz added inline comments.
swh/web/common/utils.py
103–105

you can do the same here: query_dict.update(sorted(params.items())) or even query_dict = dict(sorted(params.items())

This revision is now accepted and ready to land.Jun 10 2022, 9:12 AM

Build is green

Patch application report for D7974 (id=28748)

Rebasing onto b54d930bed...

Current branch diff-target is up to date.
Changes applied before test
commit de50f0b4499620b7efdbc7797feec9439ec37148
Author: Antoine Lambert <anlambert@softwareheritage.org>
Date:   Fri Jun 3 13:48:45 2022 +0200

    common/utils: Ensure str type for query parameters in reverse function
    
    It prevents possible issues in URL generation when providing a query parameter
    that is not of string type.
    
    See for instance bug fixed in rDWAPPS1c512a8365afb9485e9ac202046fd1ca4223a3c4
    that could have been spotted by mypy if such rule was in place.

See https://jenkins.softwareheritage.org/job/DWAPPS/job/tests-on-diff/1897/ for more details.