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

Make HTTP methods case sensitive, also allow non-standard HTTP methods #65

Merged
merged 7 commits into from
Feb 6, 2025

Conversation

arnu515
Copy link
Contributor

@arnu515 arnu515 commented Jan 16, 2025

This PR makes methods case sensitive (fixes #64), and allows non-standard methods. This makes it more in line with the HTTP RFCs.

An example:

http.parse_method("GET")  // -> Ok(http.Get)
http.parse_method("Get")  // -> Ok(http.Other("Get"))
http.parse_method("ГЄТ")  // -> Error(Nil)

Copy link
Member

@lpil lpil left a comment

Choose a reason for hiding this comment

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

Hello! Thank you for this.

This library was designed to be more permissive, but you raise a good point about methods of different cases not being equal.

This this is to be accepted the checking code will need to be moved into Gleam rather than replicating the logic in two external implementations.

@arnu515
Copy link
Contributor Author

arnu515 commented Jan 16, 2025

Okay, I'll port the implementation over to Gleam. Is it fine for the FFI code to call (compiled) gleam code again?

P.S.: Does gleam have syntax for getting the ASCII value of a character, like $a becomes 97 in erlang?

@lpil
Copy link
Member

lpil commented Jan 21, 2025

Okay, I'll port the implementation over to Gleam. Is it fine for the FFI code to call (compiled) gleam code again?

Ideally Gleam code should retain control, but it can be otherwise if there's a good performance reason.

P.S.: Does gleam have syntax for getting the ASCII value of a character, like $a becomes 97 in erlang?

It does not, no.

@arnu515
Copy link
Contributor Author

arnu515 commented Jan 23, 2025

It does not, no.

Ah that's unfortunate.

This this is to be accepted the checking code will need to be moved into Gleam rather than replicating the logic in two external implementations.

How do I structure the code? For example, http.method_from_dynamic calls a native function called decode_method, which decodes a method from a string in JS, and a charlist/atom/binary in erlang. This function has to call is_valid_tchar, which is the checking code you're talking about. If I have to move it to gleam, I assume I'll have to make it public, which is unnecessary since it will not be used by library consumers. Instead, I should put it in an internal folder.

There's another way I can do it -- I can make the decode_method function return a string representation of the value passed, and that can then be checked in gleam.

Please let me know what approach you'd prefer.

@lpil
Copy link
Member

lpil commented Feb 4, 2025

Sorry for the delay, I've been dealing with family matters.

Let's remove the method_from_dynamic function entirely. It doesn't have a valid use case.

@arnu515
Copy link
Contributor Author

arnu515 commented Feb 4, 2025

Sounds good!

@arnu515
Copy link
Contributor Author

arnu515 commented Feb 4, 2025

I've removed the erlang and js ffi modules entirely because they are only used for method_from_dynamic. Please take a look!

Copy link
Member

@lpil lpil left a comment

Choose a reason for hiding this comment

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

Thank you!

@lpil
Copy link
Member

lpil commented Feb 5, 2025

Ah! The tests are failing!

@arnu515
Copy link
Contributor Author

arnu515 commented Feb 5, 2025

The cached compiled beam file made me miss it lol

@lpil
Copy link
Member

lpil commented Feb 5, 2025

oops, sorry about that. Would be good if the build tool could handle that nicely

@arnu515
Copy link
Contributor Author

arnu515 commented Feb 5, 2025

Okay, now they should pass!

Copy link
Member

@lpil lpil left a comment

Choose a reason for hiding this comment

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

Thank you!!

@lpil lpil merged commit 0accb31 into gleam-lang:main Feb 6, 2025
1 check passed
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.

Standard HTTP methods must be in uppercase
2 participants