Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[sharing-ng] status code for multiple mount and unmount share #8876

Closed
Tracked by #8428
S-Panta opened this issue Apr 17, 2024 · 11 comments · Fixed by #9197
Closed
Tracked by #8428

[sharing-ng] status code for multiple mount and unmount share #8876

S-Panta opened this issue Apr 17, 2024 · 11 comments · Fixed by #9197
Assignees
@2403905
Labels
Type:Bug

Comments

@S-Panta
Copy link
Contributor

S-Panta commented Apr 17, 2024 •
edited

Describe the bug

Mounting the share multiple times results in 400 status code. BUt this request isn't a bad request but conflict in request and thus should be 409. As RFC states in https://datatracker.ietf.org/doc/html/rfc7231#section-6.5.8

The 409 (Conflict) status code indicates that the request could not
   be completed due to a conflict with the current state of the target
   resource.

Same goes for unmounting share multiple times. At the moment, oCIS returns 424 failed dependency status code. MDN docs writes

The HTTP 424 Failed Dependency client error response code indicates that the method could not be performed on the resource because the requested action depended on another action, and that action failed.

What are the appropriate status code for these two request?

Steps to reproduce

1.Login to ocis as admin
2.create a folder test and share to einstein
3. click disable sync.
4. Send API request to mount resource.
5. send the request again to mount resource

curl -X POST 'https://host.docker.internal:9200/graph/v1beta1/drives/{share-Space-Id}/root/children'  
-d '{
    "remoteItem": {
        "id":"'item-id"
    }
}' -ueinstein:relativity
@S-Panta
Copy link
Contributor Author

S-Panta commented Apr 17, 2024

@rhafer @2403905

@S-Panta S-Panta mentioned this issue Apr 17, 2024
Open
30 tasks
@rhafer rhafer changed the title status code for multiple mount and unmount share [sharing-ng] status code for multiple mount and unmount share Apr 17, 2024
@S-Panta S-Panta mentioned this issue Apr 29, 2024
Closed
23 tasks
@S-Panta
Copy link
Contributor Author

S-Panta commented May 15, 2024

At the moment, multiple unmount request gives 400 status code.

@2403905 2403905 self-assigned this May 15, 2024
@rhafer rhafer mentioned this issue May 15, 2024
Closed
@2403905 2403905 mentioned this issue May 16, 2024
Merged
9 tasks
@2403905
Copy link
Contributor

2403905 commented May 17, 2024

@S-Panta I added the PR and some tests have been affected. Please validate the test result and clarify what is the suitable response code (400 or 404) in the given scenarios ?

runsh: Total unexpected failed scenarios throughout the test run:
apiSharingNg/enableDisableShareSync.feature:302
apiSharingNg/enableDisableShareSync.feature:378
apiSharingNg/enableDisableShareSync.feature:465
apiSharingNg/enableDisableShareSync.feature:547
apiSharingNg/enableDisableShareSync.feature:597

@saw-jan
Copy link
Member

saw-jan commented May 17, 2024

@S-Panta I added the PR and some tests have been affected. Please validate the test result and clarify what is the suitable response code (400 or 404) in the given scenarios ?

runsh: Total unexpected failed scenarios throughout the test run:
apiSharingNg/enableDisableShareSync.feature:302
apiSharingNg/enableDisableShareSync.feature:378
apiSharingNg/enableDisableShareSync.feature:465
apiSharingNg/enableDisableShareSync.feature:547
apiSharingNg/enableDisableShareSync.feature:597

404 for these scenarios looks valid

@S-Panta
Copy link
Contributor Author

S-Panta commented May 17, 2024

imo should be 404 as the shared resource gets deleted in those scenarios.

@2403905
Copy link
Contributor

2403905 commented May 17, 2024

But it collides with previous fix #8724

@S-Panta
Copy link
Contributor Author

S-Panta commented May 17, 2024

The bad request can only be the case if the value of resource-id doesn't match the format or is empty.

ocis/tests/acceptance/features/apiSharingNg/enableDisableShareSync.feature

Line 302 in 590a40d

Scenario: try to enable share sync of a non-existent resource

Here we enable sync where there is no resource shared. Thus, it should be 404.
Are there any tests to cover the scenario where the id is in a weird format? @saw-jan

@saw-jan
Copy link
Member

saw-jan commented May 20, 2024

But it collides with previous fix #8724

hmm, yeah @rhafer commented this for creating a drive item with random item id (#8724)

I think this should return a 400 (Bad Request)

Should there be differences in /non-existing-file-id and /random-string? If yes then we have to test accordingly

@rhafer
Copy link
Contributor

rhafer commented May 22, 2024

Should there be differences in /non-existing-file-id and /random-string? If yes then we have to test accordingly

No. Please don't. 400 was a bad idea. 404 in both cases is better I think.

@2403905
Copy link
Contributor

2403905 commented May 23, 2024 •
edited

Should there be differences in /non-existing-file-id and /random-string? If yes then we have to test accordingly

No. Please don't. 400 was a bad idea. 404 in both cases is better I think.

It would be great to keep the response codes in conformity.
If I understand correctly in case v1beta1/drives/{drive-id}/items/{item-id} if drive-id or item-id is wrong we should respond with 404 because it is a part of the resource path.
If the body payload or get parameter is wrong we should respond with 400.
@rhafer @micbar Correct?

UPD:
I discussed this with @butonic and he suggested if drive-id or item-id is a URL parameter

  • respond 400 if the parameter is empty or malformed.
  • respond 404 if the parameter has a valid format but the destination does not exist. upd: Also we should translate the gateway error.

@2403905
Copy link
Contributor

2403905 commented May 23, 2024

Could there be problems with the fact that the proxy can cache 404?

@S-Panta S-Panta mentioned this issue May 27, 2024
Merged
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@2403905 2403905

Labels
Type:Bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix the status code for multiple mount and unmount share 2403905/ocis
4 participants
@rhafer @2403905 @saw-jan @S-Panta