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

Fix go-ipfs#7242: Remove "HEAD" from Allow methods #195

Merged
merged 4 commits into from Apr 29, 2020

Conversation

hsanjuan
Copy link
Contributor

(when GET is not allowed).

Fixes ipfs/kubo#7242

@hsanjuan hsanjuan requested a review from Stebalien April 28, 2020 22:11
@hsanjuan hsanjuan self-assigned this Apr 28, 2020
@ibnesayeed
Copy link
Contributor

I think it will be less data transferred in each response if vales of Allow header we reported in a single line.

That is:

Allow: POST, OPTIONS

Instead of

Allow: POST
Allow: OPTIONS

These repeated bytes will add up if the list of allowed methods is long.

@hsanjuan
Copy link
Contributor Author

I think it will be less data transferred in each response if vales of Allow header we reported in a single line.

That is:

Allow: POST, OPTIONS

Instead of

Allow: POST
Allow: OPTIONS

These repeated bytes will add up if the list of allowed methods is long.

I don't know. The list of allowed methods is short (for 4 headers, 21 bytes overhead in the worst case?). But then I have to allocate a slice and an additional string for the Join operation the code becomes a bit less readable so I'm not sure it's super worth it either way.

@ibnesayeed
Copy link
Contributor

I don't know. The list of allowed methods is short (for 4 headers, 21 bytes overhead in the worst case?).

Another motivation to join the values would be better client support.As per the specification both styles are correct, but in most of the examples of the HTTP RFC they show the comma separated style. It has a consequence in poorly written clients or scripts to overlook the possibility of repeated headers and only consume the first or the last entry.

But then I have to allocate a slice and an additional string for the Join operation the code becomes a bit less readable so I'm not sure it's super worth it either way.

If you are concerned about programmers maintaining yet another slice and string, then you can do something like the following (one line inclusion at the end) to leverage the internal data structure that is maintained by the standard library to support Headers.

	w.Header().Add("Allow", http.MethodOptions)
	w.Header().Add("Allow", http.MethodPost)
	if allowGet {
		w.Header().Add("Allow", http.MethodHead)
		w.Header().Add("Allow", http.MethodGet)
	}
	w.Header().Set("Allow", strings.Join(w.Header()["Allow"], ", "))

However, if you are concerned about memory and execution micro efficiency then allocating a slice with the two default values, concatenating an anonymous slice with a couple more values to it conditionally, and then anonymously joining them to set the header at once should be more efficient. The memory efficiency comes from the fact that the lifecycle of our slice will be shorter and it can be garbage collected immediately after values are joined and the header is set. While, if we had atomic header values stored inside of the Header (which is stored as a slice per key), we will be storing four small strings in one slice vs. one long string in one slice. Time efficiency comes from minimized function call stack and reduced slice mutations. Each time Header().Add() is called, it performs header key canonicalization (so that keys like ab-cd, AB-CD, Ab-cd, ab-Cd, and every other case variation are converted to Ab-Cd) and then it mutates the slice holding the value by appending one element to it.

@hsanjuan
Copy link
Contributor Author

@ibnesayeed I have given you write permissions, can you update the PR with your proposal?

@ibnesayeed
Copy link
Contributor

@hsanjuan which approach do you want me to push? The example with one line addition I illustrated above or the optimized one that I discussed?

@hsanjuan
Copy link
Contributor Author

The optimized

@ibnesayeed
Copy link
Contributor

@hsanjuan I have updated the code in the first commit to reflect what I was suggesting. However, I have also pushed a second commit to change the function name (from setAllowedHeaders to setAllowHeader) to better reflect the purpose of it. The are two changes in the function name: 1) It is Allow header, not Allowed (i.e., it suggests what methods are allowed, not what headers are allowed), and 2) now it is bettwe to use the singular form of Header as we are only including one Allow header.

@hsanjuan
Copy link
Contributor Author

Thanks @ibnesayeed for jumping in to improve things. Very much appreciated.

I'll let @Stebalien merge at will.

@Stebalien
Copy link
Member

Rebased to remove merge commit.

Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@Stebalien Stebalien merged commit 2d17fdf into master Apr 29, 2020
@hsanjuan hsanjuan deleted the fix/go-ipfs-7242 branch April 29, 2020 20:59
@Stebalien Stebalien mentioned this pull request May 26, 2020
77 tasks
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.

HEAD method is advertised in the API but not allowed
4 participants