Details

Reviewers

vlorentz

Group Reviewers

Reviewers

Commits

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

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

Branch

master

Lint

Lint Skipped

Unit

Unit Tests Skipped

Build Status

Buildable 15609
Build 24032: Phabricator diff pipeline on jenkins	Jenkins console · Jenkins
Build 24031: arc lint + arc unit

Event Timeline

ardumont created this revision.Sep 25 2020, 3:17 PM

Herald added a reviewer: Reviewers. · View Herald TranscriptSep 25 2020, 3:17 PM

ardumont edited the summary of this revision. (Show Details)Sep 25 2020, 3:18 PM

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

Harbormaster failed remote builds in B15569: Diff 14278!Sep 25 2020, 3:19 PM

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.

Harbormaster completed remote builds in B15575: Diff 14284.Sep 25 2020, 3:33 PM

Rework commit message

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.

Harbormaster completed remote builds in B15576: Diff 14285.Sep 25 2020, 3:39 PM

ardumont mentioned this in D4049: test: Update missing scenario on update metadata with multipart (post).Sep 25 2020, 3:44 PM

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.

Harbormaster completed remote builds in B15578: Diff 14287.Sep 25 2020, 3:49 PM

ardumont added inline comments.Sep 25 2020, 4:06 PM

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

Rebase on D4049

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.

Harbormaster completed remote builds in B15609: Diff 14311.Sep 26 2020, 6:52 PM

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'

ardumont added a child revision: D4057: Fix deposit atom test to actually check the response message.Sep 26 2020, 7:02 PM

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

ardumont added a parent revision: D4049: test: Update missing scenario on update metadata with multipart (post).

Rebase

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.

Harbormaster completed remote builds in B15616: Diff 14318.Sep 26 2020, 7:17 PM

vlorentz accepted this revision.Sep 28 2020, 12:10 PM

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

Closed by commit rDDEPb04c457d6568: Fix transition deposit to deposited state with empty post on SE IRI (authored by ardumont). · Explain WhySep 28 2020, 12:14 PM

This revision was automatically updated to reflect the committed changes.

ardumont added a commit: rDDEPb04c457d6568: Fix transition deposit to deposited state with empty post on SE IRI.

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

Details

Diff Detail

Event Timeline

Patch application report for D4052 (id=14278)

Changes applied before test

Patch application report for D4052 (id=14284)

Changes applied before test

Patch application report for D4052 (id=14285)

Changes applied before test

Patch application report for D4052 (id=14287)

Changes applied before test

Patch application report for D4052 (id=14311)

Changes applied before test

Patch application report for D4052 (id=14318)

Changes applied before test

Revision Contents
Changeset List

Diff 14311

swh/deposit/api/deposit_update.py

swh/deposit/tests/api/test_deposit_update.py

Fix transition deposit to deposited state with empty post on SE IRIClosedPublicActions

Details

Diff Detail

Event Timeline

Patch application report for D4052 (id=14278)

Changes applied before test

Patch application report for D4052 (id=14284)

Changes applied before test

Patch application report for D4052 (id=14285)

Changes applied before test

Patch application report for D4052 (id=14287)

Changes applied before test

Patch application report for D4052 (id=14311)

Changes applied before test

Patch application report for D4052 (id=14318)

Changes applied before test

Revision ContentsChangeset List

Diff 14311

swh/deposit/api/deposit_update.py

swh/deposit/tests/api/test_deposit_update.py

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

Revision Contents
Changeset List