Skip to content

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.

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.

3 participants