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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

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 16003

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