Page MenuHomeSoftware Heritage

Fix transition deposit to deposited state with empty post on SE IRI
ClosedPublic

Authored by ardumont on Sep 25 2020, 3:17 PM.

Details

Summary

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.

Depends on D4049

Test Plan

tox

Diff Detail

Repository
rDDEP Push deposit
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Build has FAILED

Patch application report for D4052 (id=14278)

Could not rebase; Attempt merge onto 7849f40a8a...

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                  | 240 ++++++++++--
 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       | 427 ++++++++++++++++++++-
 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, 840 insertions(+), 200 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 408ca25c8e4c1f111b64dcb3d2ef3ba1b2ff7305
Merge: 7849f40a b925c729
Author: Jenkins user <jenkins@localhost>
Date:   Fri Sep 25 13:17:14 2020 +0000

    Merge branch 'diff-target' into HEAD

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

    Add and fix missing scenarios around transitioning deposit to deposited
    
    Prior to this, the coverage was missing and it could happen that without the
    right IN-PROGRESS header, the api call would fail.
    
    This fixes that behavior to actually accept or fail the request. And adds the
    proper tests around those scenarios.

commit d909bb8628f212faa3d200f1bc62042bc711c879
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 c217dfdddb0f955a68c9943a26506c43b8073682
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)

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).

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

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

Simplify the check, IN-PROGRESS if not specified, is False so no need for the elif condition

swh/deposit/api/deposit_update.py
176

No need for that elif, headers['in-progress'] is False if not provided.

ardumont retitled this revision from Add and fix missing scenarios around transitioning deposit to deposited to Fix transition deposit to deposited state with empty post.Sep 25 2020, 3:33 PM

Build is green

Patch application report for D4052 (id=14284)

Could not rebase; Attempt merge onto 7849f40a8a...

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                  | 233 ++++++++++--
 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       | 400 ++++++++++++++++++++-
 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, 806 insertions(+), 200 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 ad0ea5e869e7e12f96b269ad31103b8e06567eca
Merge: 7849f40a a13338d9
Author: Jenkins user <jenkins@localhost>
Date:   Fri Sep 25 13:31:06 2020 +0000

    Merge branch 'diff-target' into HEAD

commit a13338d9ee404e5f77b7031acb51efb14228e03b
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
    
    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.

commit d909bb8628f212faa3d200f1bc62042bc711c879
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 c217dfdddb0f955a68c9943a26506c43b8073682
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)

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/125/ for more details.

ardumont retitled this revision from Fix transition deposit to deposited state with empty post to Fix transition deposit to deposited state with empty post on SE IRI.Sep 25 2020, 3:36 PM
ardumont edited the summary of this revision. (Show Details)

Build is green

Patch application report for D4052 (id=14285)

Could not rebase; Attempt merge onto 7849f40a8a...

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                  | 233 ++++++++++--
 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       | 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 +-
 18 files changed, 802 insertions(+), 200 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 d55e233bef38d4bfaa534a2f78bce24145fe6129
Merge: 7849f40a daa84fb3
Author: Jenkins user <jenkins@localhost>
Date:   Fri Sep 25 13:36:42 2020 +0000

    Merge branch 'diff-target' into HEAD

commit daa84fb306919f7cb6ee066f6b62093050f444a3
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 d909bb8628f212faa3d200f1bc62042bc711c879
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 c217dfdddb0f955a68c9943a26506c43b8073682
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)

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/126/ for more details.

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

Rebase

I'm uncertain about which scenario is tested.

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

I actually don't get which scenario is tested.
Let's see:
empty atom is the xml -> no xml file?
it's a POST (so not an update, so for now it can't be just metadata).
header is in-progress

Clarify in docstring

314

checking that the deposit is still in-progress

327

here it's deposited.
So there was an update for a partial deposit with only metadata. right?

Build is green

Patch application report for D4052 (id=14287)

Could not rebase; Attempt merge onto 7849f40a8a...

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 ++++
 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       | 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 +-
 18 files changed, 801 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 6225d661f707ceea08cc10ce493fefc1d8e4a600
Merge: 7849f40a c6c8fa33
Author: Jenkins user <jenkins@localhost>
Date:   Fri Sep 25 13:47:19 2020 +0000

    Merge branch 'diff-target' into HEAD

commit c6c8fa336efc1c12580f5fabfd6da44efa9f35d1
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 8ae38c1cdc10dacb49228dcf4788624d12aa1f6c
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 c217dfdddb0f955a68c9943a26506c43b8073682
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)

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/128/ for more details.

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

I'm testing the conditional in the deposit_update.py code above (which is orange in
another diff, that means untested [1]

You can check that the blue near the scrollbar which says 'alright, we passed
there now` (when the build is actually finished being run that is :)

[1] not covered in the previous diffs i opened, which adds coverage to some
other uncovered part (which ... same players plays again ...). I'm playing
whackamole with our current deposit_update module here ¯\_(ツ)_/¯

314

yes, but the assert is enough to say as much.

327

It did not do anything but transitions the status from partial to deposited (the deposit already has the necessary archive and metadata).

Build is green

Patch application report for D4052 (id=14311)

Could not rebase; Attempt merge onto b820c61d67...

Merge made by the 'recursive' strategy.
 swh/deposit/api/deposit_update.py            |  38 +++---
 swh/deposit/tests/api/test_deposit_update.py | 171 ++++++++++++++++++++++++++-
 2 files changed, 188 insertions(+), 21 deletions(-)
Changes applied before test
commit 0cf79fc09d255b7ea62fbe6940b39e5db53db690
Merge: b820c61d bb0167a8
Author: Jenkins user <jenkins@localhost>
Date:   Sat Sep 26 16:49:47 2020 +0000

    Merge branch 'diff-target' into HEAD

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/132/ for more details.

I'm uncertain about which scenario is tested.

This has nothing to do with new metadata update scenario, that's an existing scenario which was not tested.
This was unveiled by jenkins running that code in the current new scenario i'm adding.

There is actual documentation linked in the code i'm testing (deposit_update.py file, check the blue, meaning "covered", in the conditional now[1]) which explains it, quoting below:

The client can assert that a deposit process has completed by issuing an HTTP POST to the SE-IRI with a blank message body and with the In-Progress header set to false (it may simply omit the header altogether too, as this is treated as In-Progress: false by the server). The client SHOULD specify a Content-Length: 0 HTTP header.

I'm just being "pedantically correct" regarding tests because in my experience, it's often the case that if one test is missing, the bug or a regression slide in there...

[1] it was orange in another diff which means 'not covered'

Build is green

Patch application report for D4052 (id=14318)

Could not rebase; Attempt merge onto b820c61d67...

Updating b820c61d..b04c457d
Fast-forward
 swh/deposit/api/deposit_update.py            |  38 +++---
 swh/deposit/tests/api/test_deposit_update.py | 171 ++++++++++++++++++++++++++-
 2 files changed, 188 insertions(+), 21 deletions(-)
Changes applied before test
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/139/ for more details.

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