Fix #2383, accepts mimeType

[ReneWerner87]

Description

Correct the invalid behavior of the ctx.Accepts method

Fixes #2383

Type of change

• Bug fix (non-breaking change which fixes an issue)

Checklist:

• For new functionalities I follow the inspiration of the express js framework and built them similar in usage
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes
• I tried to make my code as fast as possible with as few allocations as possible
• For new code I have written benchmarks so that they can be analyzed and improved

[ReneWerner87]

current master:

Benchmark_Ctx_Accepts/run-[]string{".xml"}-2            	 	 	         7432663	        162.4 ns/op	       0 B/op	       0 allocs/op
Benchmark_Ctx_Accepts/run-[]string{"json",_"xml"}-2     	                         5298415	        226.2 ns/op	       0 B/op	       0 allocs/op
Benchmark_Ctx_Accepts/run-[]string{"application/json",_"application/xml"}-2         	 8494714	        141.1 ns/op	       0 B/op	       0 allocs/op
Benchmark_Ctx_AcceptsCharsets-2                                                     	19419148	        61.88 ns/op	       0 B/op	       0 allocs/op
Benchmark_Ctx_AcceptsEncodings-2                                                    	15028786	        79.98 ns/op	       0 B/op	       0 allocs/op
Benchmark_Ctx_AcceptsLanguages-2                                                    	18556627	        62.99 ns/op	       0 B/op	       0 allocs/op

[ReneWerner87]

i have reversed the loop for all accepts methods, so that the arguments are passed first and then the header values are checked

@efectn @sixcolors @derkan
do you agree that this was the right thing to do ? the logic of express leading to the outputs implies this to me

[ReneWerner87]

after the fix:

Benchmark_Ctx_Accepts/run-[]string{".xml"}-2            	                         7367316	        162.8 ns/op	       0 B/op	       0 allocs/op
Benchmark_Ctx_Accepts/run-[]string{"json",_"xml"}-2     	                         6219842	        192.5 ns/op	       0 B/op	       0 allocs/op
Benchmark_Ctx_Accepts/run-[]string{"application/json",_"application/xml"}-2         	 8405518	        142.6 ns/op	       0 B/op	       0 allocs/op
Benchmark_Ctx_AcceptsCharsets-2                                                     	19289839	        62.33 ns/op	       0 B/op	       0 allocs/op
Benchmark_Ctx_AcceptsEncodings-2                                                    	14561808	        82.07 ns/op	       0 B/op	       0 allocs/op
Benchmark_Ctx_AcceptsLanguages-2                                                    	18876432	        63.73 ns/op	       0 B/op	       0 allocs/op

[derkan]

i have reversed the loop for all accepts methods, so that the arguments are passed first and then the header values are checked

@efectn @sixcolors @derkan do you agree that this was the right thing to do ? the logic of express leading to the outputs implies this to me

nice solution @ReneWerner87 thank you. 👍

[renanbastos93]

It's amazing, you made a good job, congrats dude.

[renanbastos93]

I believe that can we change that about one line. What do you think?

Suggested change

	if len(spec) >= 1 && spec[len(spec)-1] == '*' {
	return true
	} else if strings.HasPrefix(spec, offer) {
	if (len(spec) >= 1 && spec[len(spec)-1] == '*') || strings.HasPrefix(spec, offer) {
	return true

[ReneWerner87]

yes good point, had only split it up for readability and would keep it that way as it is more understandable

[sixcolors]

i have reversed the loop for all accepts methods, so that the arguments are passed first and then the header values are checked

@efectn @sixcolors @derkan do you agree that this was the right thing to do ? the logic of express leading to the outputs implies this to me

@ReneWerner87 good job, this make the existing test cases behave as expected.

Reading the ctx.Accepts docs my understanding is that:

The Accepts function in the GoFiber context is used to check if the specified extensions or content types are acceptable based on the request's Accept HTTP header.

It takes one or more strings as input arguments representing the content types or extensions and returns the most preferred one based on the Accept header.

Therefore, if my understanding of the intent of the Accept function is correct, the following should hold true:

c.Request().Header.Set(HeaderAccept, "text/plain, application/json;q=0.5, text/html, */*;q=0.1")
	utils.AssertEqual(t, "txt", c.Accepts("json", "txt"))

	c.Request().Header.Set(HeaderAccept, "text/plain;charset=UTF-8;q=0.8,text/plain;charset=US-ASCII;q=0.4,text/plain;q=0.2,text/html")
	utils.AssertEqual(t, "text/plain;charset=US-ASCII", c.Accepts("text/plain", "text/plain;charset=US-ASCII"))
	utils.AssertEqual(t, "text/plain;charset=UTF-8", c.Accepts("text/plain;charset=US-ASCII", "text/plain;charset=UTF-8", "text/plain"))
	utils.AssertEqual(t, "text/html", c.Accepts("text/plain", "text/html"))

For example: It is possible that a browser could use the Accept header with the value text/plain;charset=UTF-8;q=0.8,text/plain;charset=US-ASCII;q=0.4,text/html. While text/html is typically assumed to have a default quality value of 1 if not specified, the browser could still explicitly specify it with a quality value of 1, as it is allowed by the HTTP specifications.

Furthermore, the browser could be indicating to the server that it prefers text/plain with a charset of UTF-8 over text/plain with a charset of US-ASCII, but it is still willing to accept both with different quality values. This could be useful in scenarios where the server has content in both character sets and wants to serve the content that is preferred by the client.

However, it is worth noting that the Accept header sent by the browser may vary depending on the user agent and its configuration. Therefore, it is important for the server to properly parse and interpret the Accept header to determine the preferred content type for the client.

Background/Refs

The relevant RFCs for the HTTP/1.1 specification are RFC 7231 and RFC 7232.

Regarding the q parameter and its default value of 1, Section 5.3.1 of RFC 7231 states:

If no "*" is present in an Accept header field, then all
media types are acceptable, assuming that any media range
syntax is understood by the recipient.  If an Accept header
field is present, and if the server cannot send a response
which is acceptable according to the combined Accept field
value, then the server SHOULD send a 406 (not acceptable)
response.

[...]

Each media-range might be followed by one or more accept-
params, beginning with the "q" parameter for indicating a
relative quality factor.  The first "q" parameter (if any)
separates the media-range parameter(s) from the accept-
params.  Quality factors allow the user or user agent to
indicate the relative degree of preference for that media-
type, on a scale of 0 to 1.

Regarding the order of the MIME types in the Accept header, Section 5.3.2 of RFC 7231 states:

The order in which the content-codings are listed in the
header field is significant.  Typically, the first encoding
that is applied is the one listed as the first content-coding;
subsequent encodings are applied in the order listed.  The
order of charset values in the header field is significant,
whereas the order of language-tag values is not.  Specifically,
it does not mean that the first charset listed is preferred,
but rather that it represents the default charset if none is
otherwise specified.

An "Accept" header field can be present in a request to
indicate the formats that are acceptable for the response.
Field content negotiation is described in Section 5.3.  An
origin server that is capable of generating a response
message which is subject to content negotiation MAY vary the
response that is sent, based on the value of the Accept header
field supplied in the request.  For example, the server might
include a different Content-Type header field, or might
omit the content entirely.

The order in which the media type values are listed is
significant.  The first media type that is listed as acceptable
determines the format of the entity-body that is returned.  A
more precise or generally usable media type listed earlier in
the Accept header field SHOULD be given preference over those
that are listed later.

MDN on Quality Values https://developer.mozilla.org/en-US/docs/Glossary/Quality_values

The importance of a value is marked by the suffix ';q=' immediately followed by a value between 0 and 1 included, with up to three decimal digits, the highest value denoting the highest priority. When not present, the default value is 1.

[Examples](https://developer.mozilla.org/en-US/docs/Glossary/Quality_values#examples)
The following syntax

text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8

indicates the order of priority:

[...]

If there is no priority defined for the first two values, the order in the list is irrelevant. Nevertheless, with the same quality, more specific values have priority over less specific ones:

text/html;q=0.8,text/*;q=0.8,*/*;q=0.8

Value	Priority
text/html	0.8 (but totally specified)
text/*	0.8 (partially specified)
*/*	0.8 (not specified)

Some syntax, like the one of Accept, allow additional specifiers like text/html;level=1. These increase the specificity of the value. Their use is extremely rare.

[ReneWerner87]

are you ok with the current fix and you then provide a subsequent solution that takes into account the quantity parameter ? we should also test how expressjs behaves with these

new unittest etc.

[sixcolors]

A full solution would likely involve a specificity function and sorting of a struct that looks something like:

type acceptedType struct {
	mime string
	q float64
	charset string
}

A performance cost seems likely.

Finally; If my understanding of ctx.Accpets() func is correct: May I suggest updating the docs with a similar text to my previous comment, so that there is no ambiguity?

[sixcolors]

are you ok with the current fix and you then provide a subsequent solution that takes into account the quantity parameter ? we should also test how expressjs behaves with these

new unittest etc.

Yes, the current PR is an improvement, so I'll approve and then submit a PR to deal with quality value and specificity.

[sixcolors]

LGTM