-
Notifications
You must be signed in to change notification settings - Fork 24
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.
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
Return 404 for not-found zarr chunks #6381
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there is a second method that needs your change, somewhere in VolumeTracingController. Otherwise, LGTM
@@ -212,7 +212,8 @@ class ZarrStreamingController @Inject()( | |||
), | |||
DataServiceRequestSettings(halfByte = false) | |||
) | |||
(data, _) <- binaryDataService.handleDataRequests(List(request)) | |||
(data, notFoundIndices) <- binaryDataService.handleDataRequests(List(request)) | |||
_ <- bool2Fox(notFoundIndices.size == 0) ~> "zarr.chunkNotFound" ~> 404 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_ <- bool2Fox(notFoundIndices.size == 0) ~> "zarr.chunkNotFound" ~> 404 | |
_ <- bool2Fox(notFoundIndices.isEmpty) ~> "zarr.chunkNotFound" ~> NOT_FOUND |
(possibly, NOT_FOUND needs to be be imported from play.api.http.Status.NOT_FOUND)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other lines above also use ~> 404
should I replace them all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if it’s no trouble, yes please :)
Seems to me |
Return 404 for not-found zarr chunks. Not sure if this plays well with the other currently in-review PRs for Zarr. Happy to close this one again, if you think that makes sense. I was just curious how a fix could be implemented.
Steps to test:
l4_sample
dataset and set it to "public":curl -I http://localhost:9000/data/zarr/sample_organization/l4_sample/color/1-1-1/0.112.112.32
-> 200 Okcurl -I http://localhost:9000/data/zarr/sample_organization/l4_sample/color/1-1-1/0.0.0.0
-> 404 Not foundIssues:
(Please delete unneeded items, merge only when none are left open)