api/server: Ensure to return a 404 status code for not found object
AbandonedPublicDraft
Actions

Authored by anlambert on Aug 9 2022, 7:00 PM.

Details

Reviewers

None

Group Reviewers

Reviewers

Summary

When an ObjNotFoundError is raised by an objstorage RPC endpoint
implementation, ensure that a 404 HTTP status code will be returned
for consistency.

Previously, a 400 HTTP status code was returned.

Depends on D8227

Diff Detail

Repository

rDOBJS Object storage

Branch

master

Lint

No Linters Available

Unit

No Unit Test Coverage

Build Status

Buildable 30735
Build 48055: Phabricator diff pipeline on jenkins	Jenkins console · Jenkins
Build 48054: arc lint + arc unit

Unit TestsFailed
View All

	Time	Test
	129 ms	Jenkins > .tox.py3.lib.python3.7.site-packages.swh.objstorage.tests.test_objstorage_api.TestRemoteObjStorage::test_check_missing self = <swh.objstorage.tests.test_objstorage_api.TestRemoteObjStorage testMethod=test_check_missing> def test_check_missing(self):
	43 ms	Jenkins > .tox.py3.lib.python3.7.site-packages.swh.objstorage.tests.test_objstorage_api.TestRemoteObjStorage::test_delete_missing self = <swh.objstorage.tests.test_objstorage_api.TestRemoteObjStorage testMethod=test_delete_missing> def test_delete_missing(self):
	57 ms	Jenkins > .tox.py3.lib.python3.7.site-packages.swh.objstorage.tests.test_objstorage_api.TestRemoteObjStorage::test_delete_present self = <swh.objstorage.tests.test_objstorage_api.TestRemoteObjStorage testMethod=test_delete_present> def test_delete_present(self):
	59 ms	Jenkins > .tox.py3.lib.python3.7.site-packages.swh.objstorage.tests.test_objstorage_api.TestRemoteObjStorage::test_get_missing self = <swh.objstorage.tests.test_objstorage_api.TestRemoteObjStorage testMethod=test_get_missing> def test_get_missing(self):
	163 ms	Jenkins > .tox.py3.lib.python3.7.site-packages.swh.objstorage.tests.test_objstorage_api.TestRemoteObjStorage::test_404_status_code_on_missing_object
		View Full Test Results (4 Failed · 600 Passed · 8 Skipped)

Event Timeline

anlambert created this revision.Aug 9 2022, 7:00 PM

Herald added a reviewer: Reviewers. · View Herald TranscriptAug 9 2022, 7:00 PM

anlambert mentioned this in D8227: api/RPCClient: Do not raise 404 remote exception if it must be reraised.Aug 9 2022, 7:01 PM

Build has FAILED

Patch application report for D8228 (id=29676)

Rebasing onto 2e642be3ec...

Current branch diff-target is up to date.

Changes applied before test

commit 444fdb8dd853c85c576093b4652da082fcbcd8a6
Author: Antoine Lambert <anlambert@softwareheritage.org>
Date:   Tue Aug 9 18:51:43 2022 +0200

    api/server: Ensure to return a 404 status code for not found object
    
    When an ObjNotFoundError is raised by an objstorage RPC endpoint
    implementation, ensure that a 404 HTTP status code will be returned
    for consistency.
    
    Previously, a 400 HTTP status code was returned.

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

Harbormaster returned this revision to the author for changes because remote builds failed.Aug 9 2022, 7:04 PM

Harbormaster failed remote builds in B30735: Diff 29676!

In D8228#213911, @swh-public-ci wrote:
Build has FAILED

Patch application report for D8228 (id=29676)

Rebasing onto 2e642be3ec...
Current branch diff-target is up to date.
Changes applied before test
commit 444fdb8dd853c85c576093b4652da082fcbcd8a6
Author: Antoine Lambert <anlambert@softwareheritage.org>
Date:   Tue Aug 9 18:51:43 2022 +0200

    api/server: Ensure to return a 404 status code for not found object
    
    When an ObjNotFoundError is raised by an objstorage RPC endpoint
    implementation, ensure that a 404 HTTP status code will be returned
    for consistency.
    
    Previously, a 400 HTTP status code was returned.
Link to build: https://jenkins.softwareheritage.org/job/DOBJS/job/tests-on-diff/174/
See console output for more information: https://jenkins.softwareheritage.org/job/DOBJS/job/tests-on-diff/174/console

Build will fail untill D8227 landed and tagged.

ensure that a 404 HTTP status code will be returned for consistency.

Consistency with what? None of the RPC servers uses 404 status codes to represent missing objects

In D8228#213935, @vlorentz wrote:

ensure that a 404 HTTP status code will be returned for consistency.

Consistency with what? None of the RPC servers uses 404 status codes to represent missing objects

With HTTP protocol, it makes sense to return a 404 status code in the objstorage context imho.

The RPC protocol does not use specific HTTP status codes just for the sake of following HTTP status codes; and it complicates handling on the server side.

For the same reason, we don't use 204 on methods returning None, 403 or 201 in _add endpoints, 409 on hash collisions, etc.

In D8228#213996, @vlorentz wrote:

The RPC protocol does not use specific HTTP status codes just for the sake of following HTTP status codes; and it complicates handling on the server side.

For the same reason, we don't use 204 on methods returning None, 403 or 201 in _add endpoints, 409 on hash collisions, etc.

Alright, abandoning this then.

anlambert abandoned this revision.Aug 10 2022, 5:10 PM