Page MenuHomeSoftware Heritage

vault: Fix vault response schema
ClosedPublic

Authored by ardumont on Thu, Nov 19, 11:42 AM.

Details

Summary

Names got changed and, unfortunately, it was not caught neither by vault tests nor swh-web's ¯\_(ツ)_/¯.

wip and incomplete (needs to fix tests now)

In docker, i no longer reproduce with this.

Test Plan

tox

docker

$ cd swh-web && yarn build-dev
$ emacs docker-compose.override.yml  # and add the swh-web override mount points with the local swh-web repository
$ docker-compose up
$ # load some repository
$ # request some cooking
$ # check the listing now works

Diff Detail

Repository
rDWAPPS Web applications
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

ardumont created this revision.Thu, Nov 19, 11:42 AM
ardumont planned changes to this revision.Thu, Nov 19, 11:43 AM

Build has FAILED

Patch application report for D4521 (id=16022)

Rebasing onto 660ef62fd6...

First, rewinding head to replay your work on top of it...
Applying: vault-ui: Fix vault listing
Changes applied before test
commit a967e789bd61a4e5433bfde392a5e0d61e1c628a
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Thu Nov 19 11:12:48 2020 +0100

    vault-ui: Fix vault listing
    
    Names got changed

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

ardumont added a comment.EditedThu, Nov 19, 12:20 PM

Technically viable! with docker [1], it works!

[1]

$ cd swh-web && yarn build-dev
$ emacs docker-compose.override.yml  # and add the swh-web with the repository
$ docker-compose up

do some cooking and check the listing

Either that or modifying the vault so it keeps the same output as before or modifying the api in the webapp which makes a transformation of the data coming from the vault.

Thanks for fixing this.

I also think we could add a commit to print possible future errors by turning the catch handler to something like

.catch(error => {
  console.log('Error when fetching vault cooking tasks:', error);
});

We could also add an error message to display in place of the empty cooking list.

Thanks for fixing this.

I also think we could add a commit to print possible future errors by turning the catch handler to something like

.catch(error => {
  console.log('Error when fetching vault cooking tasks:', error);
});

We could also add an error message to display in place of the empty cooking list.

yep, good idea, it'd be nice indeed to have some feedback there.
I absolutely had no clue at first...

It hit me at some point but it was more like shot in the dark debugging ;)

ardumont edited the summary of this revision. (Show Details)Thu, Nov 19, 1:43 PM
ardumont edited the test plan for this revision. (Show Details)
ardumont updated this revision to Diff 16033.Thu, Nov 19, 1:56 PM
  • vault: Fix vault listing ui (tests maybe, i can't find the right stanza to run the tests locally)
  • vault-ui: Log caught error when listing
ardumont edited the test plan for this revision. (Show Details)Thu, Nov 19, 1:57 PM
ardumont edited the test plan for this revision. (Show Details)

Build has FAILED

Patch application report for D4521 (id=16033)

Rebasing onto 660ef62fd6...

Current branch diff-target is up to date.
Changes applied before test
commit 014e729daee4aa6d84bea1d8caf760b3abcf4e9f
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Thu Nov 19 13:56:37 2020 +0100

    vault-ui: Log caught error when listing

commit 9ae89e87cd181c73fa94a9f3e9a5131031198ab6
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Thu Nov 19 11:12:48 2020 +0100

    vault: Fix vault listing ui
    
    Names got changed in the api backend. Unfortunately, this passed undetected
    both by the vault and the webapp tests.
    
    As the try catch within the js is silent, this fails silently.
    
    This commit fixes to use the right record names.

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

ardumont planned changes to this revision.Thu, Nov 19, 2:19 PM

as discussed with @anlambert, it'd be best to have the change python side to avoid client side behavior change.
I'll adapt accordingly.

anlambert added a comment.EditedThu, Nov 19, 2:21 PM

as discussed with @anlambert, it'd be best to have the change python side to avoid client side behavior change.
I'll adapt accordingly.

Yes, after thinking back on the issue, we cannot introduce breaking changes in the public Web API responses so the changes of
schema in swh-vault should be handled in vault API endpoints implementation of swh-web.

ardumont updated this revision to Diff 16045.Thu, Nov 19, 3:07 PM

Fix issue python side
(i'm still fighting tests so they pass green but that escapes me for now)

Build has FAILED

Patch application report for D4521 (id=16045)

Rebasing onto 660ef62fd6...

Current branch diff-target is up to date.
Changes applied before test
commit 714d628f683f393288e7fdc672750463aee6123e
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Thu Nov 19 14:23:06 2020 +0100

    vault: Fix vault listing ui
    
    Names got changed in the api backend. Unfortunately, this passed undetected
    both by the vault and the webapp tests.
    
    As the try catch within the js is silent, this fails silently.
    
    This commit fixes to create a mapping from the new names to the old names to
    keep the api consistent with existing behavior.

commit ef24a558eb63f1e88a7d223d7b7cd7a77e54d5a7
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Thu Nov 19 13:56:37 2020 +0100

    vault-ui: Log caught error when listing

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

anlambert requested changes to this revision.Thu, Nov 19, 3:21 PM
anlambert added inline comments.
swh/web/api/views/vault.py
121

fetch_url is not returned by the vault, previous code adding it must be kept

This revision now requires changes to proceed.Thu, Nov 19, 3:21 PM
ardumont updated this revision to Diff 16056.Thu, Nov 19, 3:21 PM
ardumont removed a reviewer: anlambert.
  • vault: Fix tests, I forgot about the revision_gitfast implementation part
ardumont added inline comments.Thu, Nov 19, 3:24 PM
swh/web/api/views/vault.py
121

oh, right, i got mislead by the test input in there... (they add it)

Build is green

Patch application report for D4521 (id=16056)

Rebasing onto 660ef62fd6...

Current branch diff-target is up to date.
Changes applied before test
commit 0c12b5ca2864007b5b33eaff8370e0a4db13d211
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Thu Nov 19 14:23:06 2020 +0100

    vault: Fix vault listing ui
    
    Names got changed in the api backend. Unfortunately, this passed undetected
    both by the vault and the webapp tests.
    
    As the try catch within the js is silent, this fails silently.
    
    This commit fixes to create a mapping from the new names to the old names to
    keep the api consistent with existing behavior.

commit ef24a558eb63f1e88a7d223d7b7cd7a77e54d5a7
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Thu Nov 19 13:56:37 2020 +0100

    vault-ui: Log caught error when listing

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

ardumont updated this revision to Diff 16059.Thu, Nov 19, 3:33 PM

Adapt according to review

ardumont retitled this revision from wip: vault-ui: Fix vault listing to vault: Fix vault listing ui.Thu, Nov 19, 3:33 PM
ardumont retitled this revision from vault: Fix vault listing ui to vault: Fix vault response schema.Thu, Nov 19, 3:38 PM

Build is green

Patch application report for D4521 (id=16059)

Rebasing onto 660ef62fd6...

Current branch diff-target is up to date.
Changes applied before test
commit cd5a17950ad93ca8b7de6564c824d1768fb26a15
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Thu Nov 19 14:23:06 2020 +0100

    vault: Fix vault listing ui
    
    Names got changed in the api backend. Unfortunately, this passed undetected
    both by the vault and the webapp tests.
    
    As the try catch within the js is silent, this fails silently.
    
    This commit fixes to create a mapping from the new names to the old names to
    keep the api consistent with existing behavior.

commit ef24a558eb63f1e88a7d223d7b7cd7a77e54d5a7
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Thu Nov 19 13:56:37 2020 +0100

    vault-ui: Log caught error when listing

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

ardumont updated this revision to Diff 16060.Thu, Nov 19, 3:44 PM

Drop the console.log commit from the diff (it will have its own dedicated diff)

anlambert requested changes to this revision.Thu, Nov 19, 3:46 PM

Just some improvements to bring to the fetch_url value and we should be good to land this.

swh/web/api/views/vault.py
46

You could simply name the function to _vault_response IMHO.

116

I just noticed the fetch_url is not a complete one (missing protocol and domain).
Can you add the request=request parameter to the reverse function to get a complete URL ?

223

same here

This revision now requires changes to proceed.Thu, Nov 19, 3:46 PM

Build is green

Patch application report for D4521 (id=16060)

Rebasing onto 660ef62fd6...

Current branch diff-target is up to date.
Changes applied before test
commit 6eb2ceb4689410d0ea274e0bdb62154bf8622b33
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Thu Nov 19 14:23:06 2020 +0100

    vault: Fix vault listing ui
    
    Names got changed in the api backend. Unfortunately, this passed undetected
    both by the vault and the webapp tests.
    
    As the try catch within the js is silent, this fails silently.
    
    This commit fixes to create a mapping from the new names to the old names to
    keep the api consistent with existing behavior.

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

ardumont updated this revision to Diff 16062.Thu, Nov 19, 3:52 PM
  • Adapt according to review
  • Rework commit message to align with the diff title
anlambert accepted this revision.Thu, Nov 19, 3:59 PM

Great, thanks !

This revision is now accepted and ready to land.Thu, Nov 19, 3:59 PM

Build is green

Patch application report for D4521 (id=16062)

Rebasing onto 660ef62fd6...

Current branch diff-target is up to date.
Changes applied before test
commit 4d35a4435a2c0dd6fac69562985f8e3082ea1821
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Thu Nov 19 14:23:06 2020 +0100

    vault: Fix vault response schema
    
    Names got changed in the vault api backend. Unfortunately, this passed
    undetected both by the vault and the webapp tests.
    
    Furthermore, as the try catch within the js layer is silent, this fails
    silently.
    
    This commit fixes the situation by mapping from the new names to the old names.
    This keeps the the api behavior consistent.

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

This revision was automatically updated to reflect the committed changes.