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 routing in graphql-cohttp #150

Merged
merged 1 commit into from
Mar 28, 2019
Merged

Fix routing in graphql-cohttp #150

merged 1 commit into from
Mar 28, 2019

Conversation

andreas
Copy link
Owner

@andreas andreas commented Mar 28, 2019

#146 inadvertently introduced a bug due to the different handling of empty parts when splitting strings:

(* Old implementation *)
Str.(split (regexp "/") "/graphql"
- : ["graphql"] : string list

(* New implementation *)
Astring.String.cuts ~sep:"/" "/graphql"
- : [""; "graphql"]

Astring.String.cuts ~sep:"/" "/graphql/"
- : [""; "graphql"; ""]

By adding ~empty:false, the old behavior can be restored:

Astring.String.cuts ~empty:false ~sep:"/" "/graphql"
- : ["graphql"]

Astring.String.cuts ~empty:false ~sep:"/" "/graphql/"
- : ["graphql"]

This is preferable over changing the pattern matching (#149) due to handling of the trailing slashes.

@phated
Copy link

phated commented Mar 28, 2019

@andreas and @anmonteiro thanks! I was smashing my head against this for so long before I reached out. Glad this is getting a fix 😄

@andreas
Copy link
Owner Author

andreas commented Mar 28, 2019

@phated Sorry about that 😢

@andreas andreas merged commit 9ec8c84 into master Mar 28, 2019
@andreas andreas deleted the graphql-cohttp/fix-routing branch March 28, 2019 20:49
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.

2 participants