Page MenuHomeSoftware Heritage

Make API endpoints take Lists instead of Iterables as arguments
ClosedPublic

Authored by vlorentz on Wed, Jul 29, 10:27 AM.

Details

Reviewers
ardumont
Group Reviewers
Reviewers
Summary
  1. clients crash when they call the API with an iterator
  2. some backend implementations violate the contract by assuming the argument is a sequence (eg. by iterating twice on it, and assuming the elements are the same)
  3. no matter what we do, the endpoints will always get a list as argument in practice, because they go through the RPC framework
  4. there is no concrete (ha!) advantage in taking an Iterable rather than a List.

Diff Detail

Repository
rDSTO Storage manager
Branch
types
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 14053
Build 21575: Phabricator diff pipeline on jenkinsJenkins console · Jenkins
Build 21574: arc lint + arc unit

Event Timeline

vlorentz created this revision.Wed, Jul 29, 10:27 AM

Build has FAILED

Patch application report for D3636 (id=12796)

Rebasing onto 77960ca4b5...

Current branch diff-target is up to date.
Changes applied before test
commit c56616b4f39fb783a83516c01e3c744c7a0cfbd9
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Wed Jul 29 10:26:32 2020 +0200

    Make API endpoints take Lists instead of Iterables as arguments
    
    1. clients crash when they call the API with an iterator
    2. some backend implementations violate the contract by assuming
       the argument is a sequence (eg. by iterating twice on it, and
       assuming the elements are the same)
    3. no matter what we do, the endpoints will always get a list
       as argument in practice, because they go through the RPC framework
    4. there is no concrete (ha!) advantage in taking an Iterable rather
       than a List.

Link to build: https://jenkins.softwareheritage.org/job/DSTO/job/tests-on-diff/586/
See console output for more information: https://jenkins.softwareheritage.org/job/DSTO/job/tests-on-diff/586/console

ardumont requested changes to this revision.Wed, Jul 29, 2:18 PM
ardumont added a subscriber: ardumont.

Isn't there the filter, retry and buffer proxy storage to adapt accordingly as well?

This revision now requires changes to proceed.Wed, Jul 29, 2:18 PM
vlorentz updated this revision to Diff 12815.Wed, Jul 29, 2:55 PM
  • update storage proxies
  • remove irrelevant tests

Build is green

Patch application report for D3636 (id=12815)

Rebasing onto 643ebc6e7e...

First, rewinding head to replay your work on top of it...
Applying: Make API endpoints take Lists instead of Iterables as arguments
Changes applied before test
commit 29ce125987aae2b5f08419d979c66a95cc5e3512
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Wed Jul 29 10:26:32 2020 +0200

    Make API endpoints take Lists instead of Iterables as arguments
    
    1. clients crash when they call the API with an iterator
    2. some backend implementations violate the contract by assuming
       the argument is a sequence (eg. by iterating twice on it, and
       assuming the elements are the same)
    3. no matter what we do, the endpoints will always get a list
       as argument in practice, because they go through the RPC framework
    4. there is no concrete (ha!) advantage in taking an Iterable rather
       than a List.

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

no matter what we do, the endpoints will always get a list as argument in practice, because they go through the RPC framework

I was unsure about this but it seems it's the case (looking at the swh-site repository configuration, it seems we always pass by the remote storage at the end of the configuration).

ardumont accepted this revision.Wed, Jul 29, 3:13 PM
This revision is now accepted and ready to land.Wed, Jul 29, 3:13 PM

I was unsure about this but it seems it's the case

It has to be. If they didn't, we would have got errors

Build is green

Patch application report for D3636 (id=12822)

Rebasing onto 21b77304a0...

First, rewinding head to replay your work on top of it...
Applying: Make API endpoints take Lists instead of Iterables as arguments
Applying: Fix upcoming type warning with swh.core > v0.1.2.
Changes applied before test
commit 1764aa111ea6aaee54ce4cb427ce12833a998e2b
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Wed Jul 29 10:30:45 2020 +0200

    Fix upcoming type warning with swh.core > v0.1.2.
    
    origin_visit_status_get_latest expects an int, not Optional[int];
    but this error wasn't detected so far because mypy couldn't detect
    the type of the function.
    
    The next release of swh.core fixes that issue, so this fix is now
    needed to not trigger a mypy error.

commit 556d649e51837d92fceb5cd5a918dac63c40a226
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Wed Jul 29 10:26:32 2020 +0200

    Make API endpoints take Lists instead of Iterables as arguments
    
    1. clients crash when they call the API with an iterator
    2. some backend implementations violate the contract by assuming
       the argument is a sequence (eg. by iterating twice on it, and
       assuming the elements are the same)
    3. no matter what we do, the endpoints will always get a list
       as argument in practice, because they go through the RPC framework
    4. there is no concrete (ha!) advantage in taking an Iterable rather
       than a List.

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

ardumont accepted this revision.Wed, Jul 29, 4:20 PM