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.

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

oxidecomputer/dropshot#348 support for HEAD method #755

Merged
merged 2 commits into from
Sep 15, 2023
Merged

Conversation

jclulow
Copy link
Collaborator

@jclulow jclulow commented Aug 25, 2023

No description provided.

@davepacheco
Copy link
Collaborator

This seems like a good step. How about a test?

I still wonder if it'd be useful to be able to opt into Dropshot implementing this for you if you have a GET implementation (or some other way to avoid people having to implement these two methods completely separately). Or maybe the answer to that is "just use functions to commonize the code between your GET and HEAD method". Anyway, just being able to implement HEAD as you've done here seems like a fine step.

@jclulow
Copy link
Collaborator Author

jclulow commented Aug 28, 2023

This seems like a good step. How about a test?

Can you point me at an existing test that I should modify to cover this case?

I still wonder if it'd be useful to be able to opt into Dropshot implementing this for you if you have a GET implementation (or some other way to avoid people having to implement these two methods completely separately). Or maybe the answer to that is "just use functions to commonize the code between your GET and HEAD method". Anyway, just being able to implement HEAD as you've done here seems like a fine step.

I think all of that is valuable, but obviously more work -- and I think even if we did do that, it would still be valuable to be able to escape and directly handle the request this way as well.

@davepacheco
Copy link
Collaborator

This seems like a good step. How about a test?

Can you point me at an existing test that I should modify to cover this case?

It's a little cheesy but this file has a bunch of endpoints and some tests that exercise them, which is at least a basic check:

// Test delete request
#[tokio::test]
async fn test_delete_request() {
let api = demo_api();
let testctx = common::test_setup("test_delete_request", api);
let client = &testctx.client_testctx;
object_delete(&client, "/testing/delete").await;
testctx.teardown().await;
}

I'd add a HEAD one analogous to the DELETE one.

I still wonder if it'd be useful to be able to opt into Dropshot implementing this for you if you have a GET implementation (or some other way to avoid people having to implement these two methods completely separately). Or maybe the answer to that is "just use functions to commonize the code between your GET and HEAD method". Anyway, just being able to implement HEAD as you've done here seems like a fine step.

I think all of that is valuable, but obviously more work -- and I think even if we did do that, it would still be valuable to be able to escape and directly handle the request this way as well.

💯

@jclulow
Copy link
Collaborator Author

jclulow commented Aug 28, 2023

I have added a test here. It's a bit more complex than the DELETE one, because I decided to verify that we are actually able to generate a zero-length response body with the appropriate non-zero Content-Length for a HEAD request, and that it all matches up with a GET request for the same URL in our mock server.

@davepacheco
Copy link
Collaborator

Offline, we discussed that it's a little easy to misuse this in that one could try to return a HEAD response with a body. We could have the macro check for this. But urgency is more of a priority right now. We can still add better checks later.

@davepacheco davepacheco merged commit 45047a2 into main Sep 15, 2023
10 checks passed
@davepacheco davepacheco deleted the get-a-head branch September 15, 2023 02:54
@jclulow jclulow mentioned this pull request Jul 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants