search

elixir-mint / mint

Functional HTTP client for Elixir with support for HTTP/1 and HTTP/2 🌱
Apache License 2.0
1.36k stars 106 forks source link

Clarify that header_value is invalid due to non-ASCII chars #302

Closed dsdshcym closed 3 years ago

dsdshcym commented 3 years ago

Also related: https://github.com/elixir-mint/mint/issues/272

Not sure if we should rename :invalid_header_value to something like non_ascii_char_in_header_value

ericmj commented 3 years ago

I think the old error was better because we reject all non-ASCII chars, all non-visible chars, spaces and tabs.

dsdshcym commented 3 years ago

The old error was really confusing to me because I didn't know why Mayagüez was invalid I could guess the reason was the utf-8 codepoint. But I had to dive into Mint (from Finch) to confirm this guess. I really wish the error message could help me confirm that (or even better, with a link to the RFC7230)

Maybe we can provide different error message for each case?

And spaces / tabs are valid, right? https://github.com/dsdshcym/mint/blob/d9c37fd1ed7b5c9a141912daa08d3cc0dc751366/lib/mint/http1/request.ex#L85

ericmj commented 3 years ago

And spaces / tabs are valid, right?

Yes, you are correct.

whatyouhide commented 3 years ago

What's the difference between "visible" and "printable" ASCII character?

ericmj commented 3 years ago

What's the difference between "visible" and "printable" ASCII character?

It's the same thing. The HTTP spec refers to them as VCHAR or visible characters, in Elixir we generally say printable characters. Should we change it to printable?

whatyouhide commented 3 years ago

Yes I think we should change to "printable" since we have String.printable? :)

chulkilee commented 3 years ago

I don't think the error message needs to specify what are valid or invalid in details. I prefer to have the full official spec of what's valid in the doc, not in the error message. See other "invalid" error cases in the same file.

dsdshcym commented 3 years ago

@chulkilee I think of these errors like Ecto.Changeset errors. When a validation failed for a changeset field, Ecto would return reasons like should be x character(s) This kind of message guides users (developers in mint's case) to fix errors faster. Having a full official spec is great, but having detailed error messages is even better. If all the other "invalid" error cases in the same file can include reasons, that would be the best.

chulkilee commented 3 years ago

I see the benefits of more detailed reasons in the error message for developers.

I don't know when this error message is shown. If this message is only used when inspecting - I think that's acceptable. However I don't think having the full error message (not error atom) in the struct alwaysis not necessary since this is for http interaction not for validation close to user input (like ecto).

Think of it - compiler should show friendly error message output, but not necessarily holding the error message internally.