Page MenuHomeSoftware Heritage

Add functional metadata checks prior to actually update metadata
ClosedPublic

Authored by ardumont on Sep 25 2020, 1:31 PM.

Details

Summary

This adds the check D4013 did not add to avoid cluttering the diffs (as
quite some code needs moving).

Technically, this is just moving around existing checks (one done by the
checker in the standard deposit scenario) into a common module so it's
reusable.

Then adds the use case scenario checking that the failure case is indeed caught
by the api during the new metadata deposit update case. Only one failure case
is described there so the plumbing is checked.

The actual exhaustiveness in checks is done in the pre-existing
check_metadata implementation (so no need to duplicate it within the api
tests).

@moranegg ^

Depends on D4013

Test Plan

tox

Diff Detail

Repository
rDDEP Push deposit
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 15613
Build 24040: Phabricator diff pipeline on jenkinsJenkins console · Jenkins
Build 24039: arc lint + arc unit

Event Timeline

Build is green

Patch application report for D4047 (id=14269)

Could not rebase; Attempt merge onto 047588a03f...

Updating 047588a0..d8143f98
Fast-forward
 MANIFEST.in                                        |   1 +
 swh/deposit/__init__.py                            |  11 +
 swh/deposit/api/__init__.py                        |   7 -
 swh/deposit/api/checks.py                          |  54 +++++
 swh/deposit/api/common.py                          |  38 ++--
 swh/deposit/api/deposit_update.py                  | 193 +++++++++++++++++-
 swh/deposit/api/private/deposit_check.py           |  51 +----
 swh/deposit/api/private/deposit_read.py            |  10 -
 swh/deposit/config.py                              |   7 +
 swh/deposit/tests/api/test_checks.py               |  78 +++++++
 swh/deposit/tests/api/test_deposit_atom.py         |   2 +
 .../tests/api/test_deposit_private_check.py        |  77 +------
 .../api/test_deposit_private_read_metadata.py      |   2 +-
 swh/deposit/tests/api/test_deposit_update.py       | 225 ++++++++++++++++++++-
 swh/deposit/tests/conftest.py                      |  24 ++-
 .../tests/data/atom/entry-data-empty-body.xml      |   2 +-
 .../entry-data-fail-metadata-functional-checks.xml |   7 +
 swh/deposit/tests/test_init.py                     |   2 +-
 18 files changed, 613 insertions(+), 178 deletions(-)
 create mode 100644 swh/deposit/api/checks.py
 create mode 100644 swh/deposit/tests/api/test_checks.py
 create mode 100644 swh/deposit/tests/data/atom/entry-data-fail-metadata-functional-checks.xml
Changes applied before test
commit d8143f986577fb5a0f22e861c7783d00f9bfbb0a
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Sep 25 13:31:29 2020 +0200

    Check new metadata pass the functional checks prior to update them

commit f94c9effa2437ae5fdfd5311361dfff20d4f0a83
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Tue Sep 22 19:15:19 2020 +0200

    Allow deposit metadata update on deposit already completed
    
    This will allow to update the metadata of a deposit in "done" state. The
    metadata update occurs both in the deposit backend and in the metadata
    storage (provided some basic checks pass).

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

MANIFEST.in
6

Without this, somehow the actual new .xml file was not seen at all by tox.
I don't get why it worked before but meh.

moranegg added inline comments.
swh/deposit/tests/api/test_deposit_update.py
323

would change to:
test_put_update_metadata_done_deposit_failure_empty_xml

331

would add a test with a full xml, missing the mandatory fields with an assert for each field.

343

is it really :
# no title, nor author, nor name fields
or :
# empty xml

swh/deposit/api/private/deposit_check.py
177

here is our old check metadata code that i've moved around so it's reusable in our new scenario.

swh/deposit/tests/api/test_deposit_update.py
323

no, the comment is wrong this time, i've fixed it locally, the empty test is not related to this change (it's already in D4013).

331

The exhaustiveness in checks should not be here, it should be for the check_metadata (and it should already be covered by the tests i've moved around).
Here i'm just ensuring that we do catch the case of metadata failing.

343

for the specific case i chose, yes, it is.
The empty case was dealt with before in D4013.

Fix bad docstring (copy/paste ¯\_(ツ)_/¯)

Build is green

Patch application report for D4047 (id=14270)

Could not rebase; Attempt merge onto 047588a03f...

Updating 047588a0..fb2e8120
Fast-forward
 MANIFEST.in                                        |   1 +
 swh/deposit/__init__.py                            |  11 +
 swh/deposit/api/__init__.py                        |   7 -
 swh/deposit/api/checks.py                          |  54 +++++
 swh/deposit/api/common.py                          |  38 ++--
 swh/deposit/api/deposit_update.py                  | 193 +++++++++++++++++-
 swh/deposit/api/private/deposit_check.py           |  51 +----
 swh/deposit/api/private/deposit_read.py            |  10 -
 swh/deposit/config.py                              |   7 +
 swh/deposit/tests/api/test_checks.py               |  78 +++++++
 swh/deposit/tests/api/test_deposit_atom.py         |   2 +
 .../tests/api/test_deposit_private_check.py        |  77 +------
 .../api/test_deposit_private_read_metadata.py      |   2 +-
 swh/deposit/tests/api/test_deposit_update.py       | 225 ++++++++++++++++++++-
 swh/deposit/tests/conftest.py                      |  24 ++-
 .../tests/data/atom/entry-data-empty-body.xml      |   2 +-
 .../entry-data-fail-metadata-functional-checks.xml |   7 +
 swh/deposit/tests/test_init.py                     |   2 +-
 18 files changed, 613 insertions(+), 178 deletions(-)
 create mode 100644 swh/deposit/api/checks.py
 create mode 100644 swh/deposit/tests/api/test_checks.py
 create mode 100644 swh/deposit/tests/data/atom/entry-data-fail-metadata-functional-checks.xml
Changes applied before test
commit fb2e8120025a942c34fa59b9720b8dfc5e139840
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Sep 25 13:31:29 2020 +0200

    Check new metadata pass the functional checks prior to update them

commit f94c9effa2437ae5fdfd5311361dfff20d4f0a83
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Tue Sep 22 19:15:19 2020 +0200

    Allow deposit metadata update on deposit already completed
    
    This will allow to update the metadata of a deposit in "done" state. The
    metadata update occurs both in the deposit backend and in the metadata
    storage (provided some basic checks pass).

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

ardumont retitled this revision from Check new metadata pass the functional checks prior to update them to Add functional metadata checks prior to actually update metadata.Sep 25 2020, 2:37 PM
ardumont edited the summary of this revision. (Show Details)
ardumont marked an inline comment as done.
swh/deposit/api/private/deposit_check.py
164

here it seems that the software (title/name) are optional, because of the name optional.

I see below that the and makes it mandatory.
Could you check if that's the behavior?

I would suggest to change this to alternate_mandatory_result.

swh/deposit/tests/api/test_deposit_update.py
331

Nitpick, change:
incomplete metadata
To:
without required metadata fields

(to emphasis that deposit without them fails)

343

change to # no (title / name) of the software and no author

resumes the check that is done with the alternative fields

swh/deposit/api/private/deposit_check.py
164

I'm not modifying that code today, i'm just moving it around so we have consistent behavior everywhere we use it.

Could you check if that's the behavior?

It is.

The docstring indeed mentions that the checks are about "all" mandatory metadata.
So for the alternate fields, there must be at least one of those.

ardumont edited the summary of this revision. (Show Details)

Build is green

Patch application report for D4047 (id=14315)

Could not rebase; Attempt merge onto b820c61d67...

Merge made by the 'recursive' strategy.
 MANIFEST.in                                        |   1 +
 swh/deposit/__init__.py                            |  11 +
 swh/deposit/api/__init__.py                        |   7 -
 swh/deposit/api/checks.py                          |  54 +++
 swh/deposit/api/common.py                          |  38 +-
 swh/deposit/api/deposit_update.py                  | 231 ++++++++++--
 swh/deposit/api/private/deposit_check.py           |  51 +--
 swh/deposit/api/private/deposit_read.py            |  10 -
 swh/deposit/config.py                              |   7 +
 swh/deposit/tests/api/test_checks.py               |  78 ++++
 .../tests/api/test_deposit_private_check.py        |  77 +---
 .../api/test_deposit_private_read_metadata.py      |   2 +-
 swh/deposit/tests/api/test_deposit_update.py       | 396 ++++++++++++++++++++-
 swh/deposit/tests/conftest.py                      |  24 +-
 .../tests/data/atom/entry-data-empty-body.xml      |   2 +-
 .../entry-data-fail-metadata-functional-checks.xml |   7 +
 swh/deposit/tests/test_init.py                     |   2 +-
 17 files changed, 799 insertions(+), 199 deletions(-)
 create mode 100644 swh/deposit/api/checks.py
 create mode 100644 swh/deposit/tests/api/test_checks.py
 create mode 100644 swh/deposit/tests/data/atom/entry-data-fail-metadata-functional-checks.xml
Changes applied before test
commit 23c3cb76d0df661027768167a3082cc70879f075
Merge: b820c61d 8deb1911
Author: Jenkins user <jenkins@localhost>
Date:   Sat Sep 26 17:08:07 2020 +0000

    Merge branch 'diff-target' into HEAD

commit 8deb19116b78f72c7c63e7cbd6cad0ec6151df4a
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Sep 25 13:31:29 2020 +0200

    Check new metadata pass the functional checks prior to update them

commit 96ada86633e473064904bab7ed1ea7168a965fe2
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Tue Sep 22 19:15:19 2020 +0200

    Allow deposit metadata update on deposit already completed
    
    This will allow to update the metadata of a deposit in "done" state. The
    metadata update occurs both in the deposit backend and in the metadata
    storage (provided some basic checks pass).

commit 7271e750bca43692fb88b4fbd9126e88462460ac
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Sat Sep 26 18:37:50 2020 +0200

    Fix deposit atom test to actually check the response message
    
    Prior to this commit, the actual test was wrongfully ok because the slug was
    missing. This resulted in a 400 response which passed because we missed
    checking the actual response content. This now fixes it.

commit bb0167a8945c12a22e8d63e42b291dc414b59525
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Sep 25 15:15:37 2020 +0200

    Fix transition deposit to deposited state with empty post on SE IRI
    
    Prior to this the content-length could be None instead of 0, failing the
    transition.
    
    As the coverage was missing, that peculiar case was missed. This fixes it.

commit c6debbd53a1c055be30abbf738fb81aed382da49
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Sep 25 14:28:10 2020 +0200

    test: Update missing scenario on update metadata with multipart (post)

commit 8c40c1e093913734549590a5893d4d610e331de3
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Sep 25 14:14:20 2020 +0200

    test: Update missing scenario on update metadata with multipart (put)

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

Build is green

Patch application report for D4047 (id=14321)

Could not rebase; Attempt merge onto b820c61d67...

Updating b820c61d..7a2fdcd7
Fast-forward
 MANIFEST.in                                        |   1 +
 swh/deposit/__init__.py                            |  11 +
 swh/deposit/api/__init__.py                        |   7 -
 swh/deposit/api/checks.py                          |  54 +++
 swh/deposit/api/common.py                          |  38 +-
 swh/deposit/api/deposit_update.py                  | 231 ++++++++++--
 swh/deposit/api/private/deposit_check.py           |  51 +--
 swh/deposit/api/private/deposit_read.py            |  10 -
 swh/deposit/config.py                              |   7 +
 swh/deposit/tests/api/test_checks.py               |  78 ++++
 .../tests/api/test_deposit_private_check.py        |  77 +---
 .../api/test_deposit_private_read_metadata.py      |   2 +-
 swh/deposit/tests/api/test_deposit_update.py       | 396 ++++++++++++++++++++-
 swh/deposit/tests/conftest.py                      |  24 +-
 .../tests/data/atom/entry-data-empty-body.xml      |   2 +-
 .../entry-data-fail-metadata-functional-checks.xml |   7 +
 swh/deposit/tests/test_init.py                     |   2 +-
 17 files changed, 799 insertions(+), 199 deletions(-)
 create mode 100644 swh/deposit/api/checks.py
 create mode 100644 swh/deposit/tests/api/test_checks.py
 create mode 100644 swh/deposit/tests/data/atom/entry-data-fail-metadata-functional-checks.xml
Changes applied before test
commit 7a2fdcd7521f23fa4248ea77eb36b09a1efd74af
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Sep 25 13:31:29 2020 +0200

    Check new metadata pass the functional checks prior to update them

commit 221e415573d1356049103eff26152076d9f23d1a
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Tue Sep 22 19:15:19 2020 +0200

    Allow deposit metadata update on deposit already completed
    
    This will allow to update the metadata of a deposit in "done" state. The
    metadata update occurs both in the deposit backend and in the metadata
    storage (provided some basic checks pass).

commit 45c7c35e471ce28529bc495a076d10e2c67b9cf7
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Sat Sep 26 18:37:50 2020 +0200

    Fix deposit atom test to actually check the response message
    
    Prior to this commit, the actual test was wrongfully ok because the slug was
    missing. This resulted in a 400 response which passed because we missed
    checking the actual response content. This now fixes it.

commit b04c457d65689ae5e278edac7faee34bb3a2908f
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Sep 25 15:15:37 2020 +0200

    Fix transition deposit to deposited state with empty post on SE IRI
    
    Prior to this the content-length could be None instead of 0, failing the
    transition.
    
    As the coverage was missing, that peculiar case was missed. This fixes it.

commit f26ea4492695238164e8ac1bcec984feed68ed9d
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Sep 25 14:28:10 2020 +0200

    test: Update missing scenario on update metadata with multipart (post)

commit 251a0031eada21011d1683e6d755c1813830b97b
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Sep 25 14:14:20 2020 +0200

    test: Update missing scenario on update metadata with multipart (put)

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

ardumont added inline comments.
swh/deposit/tests/api/test_deposit_update.py
331

done


Also, i've never actually explicit that but i'm trying to summarize those docstrings into oneliner...
So when tests are executed in verbose mode, we actually see that oneline docstring instead of the test name... (and that without ellipsis).

I think i'm gonna relax this now.

Build is green

Patch application report for D4047 (id=14325)

Could not rebase; Attempt merge onto f26ea44926...

Updating f26ea449..6e29d812
Fast-forward
 MANIFEST.in                                        |   1 +
 swh/deposit/__init__.py                            |  11 +
 swh/deposit/api/__init__.py                        |   7 -
 swh/deposit/api/checks.py                          |  54 +++++
 swh/deposit/api/common.py                          |  38 +--
 swh/deposit/api/deposit_update.py                  | 233 ++++++++++++++++--
 swh/deposit/api/private/deposit_check.py           |  51 +---
 swh/deposit/api/private/deposit_read.py            |  23 +-
 swh/deposit/config.py                              |   6 +
 swh/deposit/tests/api/test_checks.py               |  78 ++++++
 .../tests/api/test_deposit_private_check.py        |  77 +-----
 .../api/test_deposit_private_read_metadata.py      |   2 +-
 swh/deposit/tests/api/test_deposit_update.py       | 270 ++++++++++++++++++++-
 swh/deposit/tests/conftest.py                      |  30 +--
 .../tests/data/atom/entry-data-empty-body.xml      |   2 +-
 .../entry-data-fail-metadata-functional-checks.xml |   7 +
 swh/deposit/tests/test_init.py                     |   2 +-
 17 files changed, 690 insertions(+), 202 deletions(-)
 create mode 100644 swh/deposit/api/checks.py
 create mode 100644 swh/deposit/tests/api/test_checks.py
 create mode 100644 swh/deposit/tests/data/atom/entry-data-fail-metadata-functional-checks.xml
Changes applied before test
commit 6e29d81201a00d206966c62bbca64e3177051c42
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Sep 25 13:31:29 2020 +0200

    Add functional metadata checks prior to updating them
    
    This refactor the common code executed by the checker so we functionally check
    everything the same way.

commit f23ad6cb2a550d94b0a81b1d718f24256acb94e7
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Tue Sep 22 19:15:19 2020 +0200

    Allow deposit metadata update on deposit already completed
    
    This will allow to update the metadata of a deposit in "done" state. The
    metadata update occurs both in the deposit backend and in the metadata
    storage (provided some basic checks pass).

commit 45c7c35e471ce28529bc495a076d10e2c67b9cf7
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Sat Sep 26 18:37:50 2020 +0200

    Fix deposit atom test to actually check the response message
    
    Prior to this commit, the actual test was wrongfully ok because the slug was
    missing. This resulted in a 400 response which passed because we missed
    checking the actual response content. This now fixes it.

commit b04c457d65689ae5e278edac7faee34bb3a2908f
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Sep 25 15:15:37 2020 +0200

    Fix transition deposit to deposited state with empty post on SE IRI
    
    Prior to this the content-length could be None instead of 0, failing the
    transition.
    
    As the coverage was missing, that peculiar case was missed. This fixes it.

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

Build is green

Patch application report for D4047 (id=14330)

Could not rebase; Attempt merge onto 45c7c35e47...

Updating 45c7c35e..92f99745
Fast-forward
 MANIFEST.in                                        |   1 +
 swh/deposit/__init__.py                            |  11 +
 swh/deposit/api/__init__.py                        |   7 -
 swh/deposit/api/checks.py                          |  54 +++++
 swh/deposit/api/common.py                          |  38 ++--
 swh/deposit/api/deposit_update.py                  | 205 ++++++++++++++++--
 swh/deposit/api/private/deposit_check.py           |  51 +----
 swh/deposit/api/private/deposit_read.py            |  23 +-
 swh/deposit/config.py                              |   6 +
 swh/deposit/tests/api/test_checks.py               |  78 +++++++
 swh/deposit/tests/api/test_deposit_atom.py         |   2 -
 .../tests/api/test_deposit_private_check.py        |  77 +------
 .../api/test_deposit_private_read_metadata.py      |   2 +-
 swh/deposit/tests/api/test_deposit_update.py       | 234 ++++++++++++++++++++-
 swh/deposit/tests/conftest.py                      |  30 +--
 .../entry-data-fail-metadata-functional-checks.xml |   7 +
 swh/deposit/tests/test_init.py                     |   2 +-
 17 files changed, 631 insertions(+), 197 deletions(-)
 create mode 100644 swh/deposit/api/checks.py
 create mode 100644 swh/deposit/tests/api/test_checks.py
 create mode 100644 swh/deposit/tests/data/atom/entry-data-fail-metadata-functional-checks.xml
Changes applied before test
commit 92f99745f35b95b1717d9344bd9b4e3f74b0cc20
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Fri Sep 25 13:31:29 2020 +0200

    Add functional metadata checks prior to updating them
    
    This refactor the common code executed by the checker so we functionally check
    everything the same way.

commit d1f3f69c9016ad6bb7d49515fc187dd1c85039eb
Author: Antoine R. Dumont (@ardumont) <ardumont@softwareheritage.org>
Date:   Tue Sep 22 19:15:19 2020 +0200

    Allow deposit metadata update on deposit already completed
    
    This will allow to update the metadata of a deposit in "done" state. The
    metadata update occurs both in the deposit backend and in the metadata
    storage (provided some basic checks pass).

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

vlorentz added a subscriber: vlorentz.

please add a module docstring to checks.py

This revision is now accepted and ready to land.Sep 28 2020, 1:59 PM