Page MenuHomeSoftware Heritage
Differential D4507

Remove return values of process_put and process_delete.
ClosedPublic
Actions

Authored by vlorentz on Nov 18 2020, 2:15 PM.

Details

Reviewers
ardumont
Group Reviewers
Reviewers
Commits
rDDEP1f0b39e2ed97: Remove return values of process_put and process_delete.
Summary

They were only used for the special "error" key, which no longer
exists, so they are useless now.

Depends on D4505.

Diff Detail

Repository
rDDEP Push deposit
Branch
reorganize-sword
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 17190
Build 26542: Phabricator diff pipeline on jenkinsJenkins console · Jenkins
Build 26541: arc lint + arc unit

Unit TestsFailed
View All

TimeTest
521 msJenkins > .tox.py3.lib.python3.7.site-packages.swh.deposit.tests.api.test_collection::test_delete_on_col_iri_not_supported
self = <rest_framework.test.ForceAuthClientHandler object at 0x7f7f193dfc18> request = <WSGIRequest: DELETE '/1/test/'>
402 msJenkins > .tox.py3.lib.python3.7.site-packages.swh.deposit.tests.api.test_checks::test_api_checks_check_metadata_ko[metadata_ko0-expected_summary0]
402 msJenkins > .tox.py3.lib.python3.7.site-packages.swh.deposit.tests.api.test_checks::test_api_checks_check_metadata_ko[metadata_ko1-expected_summary1]
402 msJenkins > .tox.py3.lib.python3.7.site-packages.swh.deposit.tests.api.test_checks::test_api_checks_check_metadata_ok[metadata_ok0]
402 msJenkins > .tox.py3.lib.python3.7.site-packages.swh.deposit.tests.api.test_checks::test_api_checks_check_metadata_ok[metadata_ok1]
View Full Test Results (1 Failed · 208 Passed)

Event Timeline

vlorentz created this revision.Nov 18 2020, 2:15 PM
Herald added a reviewer: Reviewers. · View Herald TranscriptNov 18 2020, 2:15 PM
vlorentz updated this revision to Diff 15987.Nov 18 2020, 2:18 PM

rebase

swh-public-ci added a comment.Nov 18 2020, 2:18 PM

Build has FAILED

Patch application report for D4507 (id=15985)

Could not rebase; Attempt merge onto a67ed6b06b...

Updating a67ed6b0..00b3af02
Fast-forward
 swh/deposit/api/{deposit.py => collection.py}      |  12 +-
 swh/deposit/api/common.py                          | 315 ++++++++++-----------
 swh/deposit/api/{deposit_content.py => content.py} |  16 +-
 swh/deposit/api/deposit_update.py                  | 277 ------------------
 swh/deposit/api/edit.py                            | 139 +++++++++
 swh/deposit/api/edit_media.py                      |  96 +++++++
 swh/deposit/api/private/__init__.py                |   8 +-
 swh/deposit/api/private/deposit_update_status.py   |  26 +-
 swh/deposit/api/service_document.py                |   2 +-
 swh/deposit/api/{deposit_status.py => state.py}    |  10 +-
 swh/deposit/api/sword_edit.py                      |  82 ++++++
 swh/deposit/api/urls.py                            |  44 +--
 swh/deposit/config.py                              |   3 +-
 swh/deposit/errors.py                              |  35 ++-
 swh/deposit/settings/common.py                     |   1 +
 swh/deposit/templates/deposit/deposit_receipt.xml  |   4 +-
 .../api/{test_deposit.py => test_collection.py}    |   4 +-
 ...eposit_atom.py => test_collection_post_atom.py} |  11 +-
 ...it_binary.py => test_collection_post_binary.py} |  17 +-
 ...etadata.py => test_collection_post_metadata.py} |   2 +
 ...tipart.py => test_collection_post_multipart.py} |   2 +
 .../api/{test_deposit_delete.py => test_delete.py} |   8 +-
 .../api/test_deposit_private_read_metadata.py      |   4 +-
 swh/deposit/tests/api/test_deposit_update.py       |  43 ++-
 .../{test_deposit_content.py => test_get_file.py}  |   2 +
 swh/deposit/tests/conftest.py                      |   4 +-
 26 files changed, 610 insertions(+), 557 deletions(-)
 rename swh/deposit/api/{deposit.py => collection.py} (91%)
 rename swh/deposit/api/{deposit_content.py => content.py} (81%)
 delete mode 100644 swh/deposit/api/deposit_update.py
 create mode 100644 swh/deposit/api/edit.py
 create mode 100644 swh/deposit/api/edit_media.py
 rename swh/deposit/api/{deposit_status.py => state.py} (83%)
 create mode 100644 swh/deposit/api/sword_edit.py
 rename swh/deposit/tests/api/{test_deposit.py => test_collection.py} (98%)
 rename swh/deposit/tests/api/{test_deposit_atom.py => test_collection_post_atom.py} (97%)
 rename swh/deposit/tests/api/{test_deposit_binary.py => test_collection_post_binary.py} (97%)
 rename swh/deposit/tests/api/{test_deposit_metadata.py => test_collection_post_metadata.py} (99%)
 rename swh/deposit/tests/api/{test_deposit_multipart.py => test_collection_post_multipart.py} (99%)
 rename swh/deposit/tests/api/{test_deposit_delete.py => test_delete.py} (94%)
 rename swh/deposit/tests/api/{test_deposit_content.py => test_get_file.py} (98%)
Changes applied before test
commit 00b3af02028e548bdbf52346a929671692abbdb9
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Wed Nov 18 14:15:34 2020 +0100

    Remove return values of process_put and process_delete.
    
    They were only used for the special "error" key, which no longer
    exists, so they are useless now.

commit b80d183f1efcdca414b2792bda1fd0b26d597300
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Wed Nov 18 14:07:35 2020 +0100

    Use exceptions instead of a special "error" key in returned dicts.

commit 7a8e6b0abe7a457cb87bd12f7fb469e811447fbe
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Wed Nov 18 13:36:32 2020 +0100

    Generalize BadRequestError to be a DepositError.
    
    First step toward using exceptions instead of dicts everywhere.

commit 66eb322d5c262157470cc4fcac7d696e3668ff9d
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Wed Nov 18 13:19:21 2020 +0100

    Move _compute_md5 to toplevel and remove its docstring.
    
    It doesn't need to be in a class, and the docstring is redundant.

commit 333f9847629bface8f8540d15d22be3a44c8240b
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Wed Nov 18 13:10:07 2020 +0100

    Move Deposit Receipt creation to its own function.
    
    And move _make_iri's code inside this function, it was almost a single statement.

commit 466a0f27d77c4601fc9933b6f67033de4df51cfe
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Wed Nov 18 12:47:00 2020 +0100

    Use an attr class instead of a dict to store parsed headers.
    
    Better typing.

commit 01cb24d3c44059839d0a1daa9c1947426171c019
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Wed Nov 18 11:48:40 2020 +0100

    Split SE-IRI and Edit-IRI.
    
    SWORD doesn't require them to be the same, and IMO it is clearer
    if we keep them separate.

commit 64371a3dc2133a977244e884a191ad7f0162557c
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Wed Nov 18 11:48:35 2020 +0100

    remove assumption that Edit-IRI and SE-IRI are the same from test_post_deposit_atom_entry_multiple_steps.
    
    Currently they are, but a future commit will make them different.

commit 329a2a3785ad8431fa7ec73fee31e86dbee36911
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Wed Nov 18 11:10:49 2020 +0100

    Rename files and classes in swh/deposit/api/deposit_* to be consistent with SWORD terminology.

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

Harbormaster failed remote builds in B17190: Diff 15985!Nov 18 2020, 2:18 PM
swh-public-ci added a comment.Nov 18 2020, 2:24 PM

Build is green

Patch application report for D4507 (id=15987)

Could not rebase; Attempt merge onto a67ed6b06b...

Updating a67ed6b0..1f0b39e2
Fast-forward
 swh/deposit/api/{deposit.py => collection.py}      |  12 +-
 swh/deposit/api/common.py                          | 316 ++++++++++-----------
 swh/deposit/api/{deposit_content.py => content.py} |  16 +-
 swh/deposit/api/deposit_update.py                  | 277 ------------------
 swh/deposit/api/edit.py                            | 139 +++++++++
 swh/deposit/api/edit_media.py                      |  96 +++++++
 swh/deposit/api/private/__init__.py                |   8 +-
 swh/deposit/api/private/deposit_update_status.py   |  26 +-
 swh/deposit/api/service_document.py                |   2 +-
 swh/deposit/api/{deposit_status.py => state.py}    |  10 +-
 swh/deposit/api/sword_edit.py                      |  82 ++++++
 swh/deposit/api/urls.py                            |  44 +--
 swh/deposit/config.py                              |   3 +-
 swh/deposit/errors.py                              |  35 ++-
 swh/deposit/settings/common.py                     |   1 +
 swh/deposit/templates/deposit/deposit_receipt.xml  |   4 +-
 .../api/{test_deposit.py => test_collection.py}    |   4 +-
 ...eposit_atom.py => test_collection_post_atom.py} |  11 +-
 ...it_binary.py => test_collection_post_binary.py} |  17 +-
 ...etadata.py => test_collection_post_metadata.py} |   2 +
 ...tipart.py => test_collection_post_multipart.py} |   2 +
 .../api/{test_deposit_delete.py => test_delete.py} |   8 +-
 .../api/test_deposit_private_read_metadata.py      |   4 +-
 swh/deposit/tests/api/test_deposit_update.py       |  43 ++-
 .../{test_deposit_content.py => test_get_file.py}  |   2 +
 swh/deposit/tests/conftest.py                      |   4 +-
 26 files changed, 610 insertions(+), 558 deletions(-)
 rename swh/deposit/api/{deposit.py => collection.py} (91%)
 rename swh/deposit/api/{deposit_content.py => content.py} (81%)
 delete mode 100644 swh/deposit/api/deposit_update.py
 create mode 100644 swh/deposit/api/edit.py
 create mode 100644 swh/deposit/api/edit_media.py
 rename swh/deposit/api/{deposit_status.py => state.py} (83%)
 create mode 100644 swh/deposit/api/sword_edit.py
 rename swh/deposit/tests/api/{test_deposit.py => test_collection.py} (98%)
 rename swh/deposit/tests/api/{test_deposit_atom.py => test_collection_post_atom.py} (97%)
 rename swh/deposit/tests/api/{test_deposit_binary.py => test_collection_post_binary.py} (97%)
 rename swh/deposit/tests/api/{test_deposit_metadata.py => test_collection_post_metadata.py} (99%)
 rename swh/deposit/tests/api/{test_deposit_multipart.py => test_collection_post_multipart.py} (99%)
 rename swh/deposit/tests/api/{test_deposit_delete.py => test_delete.py} (94%)
 rename swh/deposit/tests/api/{test_deposit_content.py => test_get_file.py} (98%)
Changes applied before test
commit 1f0b39e2ed97d5d6661df7301dda1f0efecb3214
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Wed Nov 18 14:15:34 2020 +0100

    Remove return values of process_put and process_delete.
    
    They were only used for the special "error" key, which no longer
    exists, so they are useless now.

commit 229348f845ba99f05a7db1b969a95916e8093d85
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Wed Nov 18 14:07:35 2020 +0100

    Use exceptions instead of a special "error" key in returned dicts.

commit 7a8e6b0abe7a457cb87bd12f7fb469e811447fbe
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Wed Nov 18 13:36:32 2020 +0100

    Generalize BadRequestError to be a DepositError.
    
    First step toward using exceptions instead of dicts everywhere.

commit 66eb322d5c262157470cc4fcac7d696e3668ff9d
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Wed Nov 18 13:19:21 2020 +0100

    Move _compute_md5 to toplevel and remove its docstring.
    
    It doesn't need to be in a class, and the docstring is redundant.

commit 333f9847629bface8f8540d15d22be3a44c8240b
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Wed Nov 18 13:10:07 2020 +0100

    Move Deposit Receipt creation to its own function.
    
    And move _make_iri's code inside this function, it was almost a single statement.

commit 466a0f27d77c4601fc9933b6f67033de4df51cfe
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Wed Nov 18 12:47:00 2020 +0100

    Use an attr class instead of a dict to store parsed headers.
    
    Better typing.

commit 01cb24d3c44059839d0a1daa9c1947426171c019
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Wed Nov 18 11:48:40 2020 +0100

    Split SE-IRI and Edit-IRI.
    
    SWORD doesn't require them to be the same, and IMO it is clearer
    if we keep them separate.

commit 64371a3dc2133a977244e884a191ad7f0162557c
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Wed Nov 18 11:48:35 2020 +0100

    remove assumption that Edit-IRI and SE-IRI are the same from test_post_deposit_atom_entry_multiple_steps.
    
    Currently they are, but a future commit will make them different.

commit 329a2a3785ad8431fa7ec73fee31e86dbee36911
Author: Valentin Lorentz <vlorentz@softwareheritage.org>
Date:   Wed Nov 18 11:10:49 2020 +0100

    Rename files and classes in swh/deposit/api/deposit_* to be consistent with SWORD terminology.

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

Harbormaster completed remote builds in B17192: Diff 15987.Nov 18 2020, 2:24 PM
ardumont requested changes to this revision.Nov 18 2020, 3:03 PM
ardumont added a subscriber: ardumont.

I'm fine with the delete part.

Not with put part.

I recall we need to return something, don't we?

swh/deposit/api/edit.py
76

hmm?

What do you do of the multipart_upload result?

86

Is that a tab that got inserted?

This revision now requires changes to proceed.Nov 18 2020, 3:03 PM
ardumont added inline comments.Nov 18 2020, 3:06 PM
swh/deposit/api/edit.py
92

same question about the _atom_entry result.

ardumont added a comment.Nov 18 2020, 3:10 PM

Oh and i think that breaks the cli part (e.g. for the --replace flag, which does updates).

It's not caught by tests because the requests are mocked with the requests_mock_datadir fixture.

vlorentz added inline comments.Nov 18 2020, 3:13 PM
swh/deposit/api/edit.py
76

nothing. it's not used.

$ grep "process_put(" -r swh/**/*.py
swh/deposit/api/common.py:        self.process_put(request, headers, collection_name, deposit_id)
swh/deposit/api/common.py:    def process_put(
swh/deposit/api/edit.py:    def process_put(
swh/deposit/api/edit_media.py:    def process_put(
swh/deposit/api/private/deposit_update_status.py:    def process_put(
86

I added an else, because it shouldn't run if request.content_type.startswith("multipart/")

92

also not used

vlorentz added a comment.Nov 18 2020, 3:15 PM
In D4507#112580, @ardumont wrote:

Oh and i think that breaks the cli part (e.g. for the --replace flag, which does updates).

It's not caught by tests because the requests are mocked with the requests_mock_datadir fixture.

The server does not use the return value of process_put to build its response to the client. If it it, it would be caught in server tests.

vlorentz added a child revision: D4510: Remove return value of _check_preconditions_on..Nov 18 2020, 3:35 PM
ardumont added a comment.Nov 18 2020, 3:36 PM

fwiw, I was refering to this code:

https://forge.softwareheritage.org/source/swh-deposit/browse/master/swh/deposit/client.py$707-747

ardumont added a comment.Nov 18 2020, 3:37 PM

The server does not use the return value of process_put to build its response to the client. If it it, it would be caught in server tests.

yes, it seems i misremembered here.

ardumont accepted this revision.Nov 18 2020, 3:37 PM
This revision is now accepted and ready to land.Nov 18 2020, 3:37 PM
Closed by commit rDDEP1f0b39e2ed97: Remove return values of process_put and process_delete. (authored by vlorentz). · Explain WhyNov 18 2020, 4:09 PM
This revision was automatically updated to reflect the committed changes.
vlorentz added a commit: rDDEP1f0b39e2ed97: Remove return values of process_put and process_delete..

Revision Contents
Changeset List

PathSize
swh/
deposit/
api/
6 lines
40 lines
10 lines
private/
6 lines

Diff 15985

View Options

swh/deposit/api/common.py

Loading...
View Options

swh/deposit/api/edit.py

Loading...
View Options

swh/deposit/api/edit_media.py

Loading...
View Options

swh/deposit/api/private/deposit_update_status.py

Loading...
About · Developer information · Legal