replace redirect with case-insensitive routing #142
Conversation
|
Thrilled to hear Alex! It's so cool seeing Bass Loop using Bud. Bass also looks awesome. Looks like you're bringing the simplicity of functional programming to deploying infrastructure!
Ah, I can totally see the problem. Something like:
Thanks for opening this PR! I originally did this for SEO purposes, so I had a look at what Github does and I think I like their approach:
So I think your PR makes sense once the tests pass! If you feel like adding 2., you can adjust the default layout in this file. I'm not sure we're passing enough context to the views yet to provide a full URL like that, so this could also be a follow-up task once I revisit views for custom layouts, error pages, etc. |
vito
force-pushed
the
sensitive-paths
branch
2 times, most recently
from
5966737
to
3b7f622
Compare
|
Thanks. :)
Cool, that makes sense.
Great! I got the tests passing, but I'm not sure of my approach. There were a few places that iterated over bytes, not runes. The "伪" ( So I changed I'm interested in doing 2., but pushing just these changes for now; I'll try to find time to do it soon, so feel free to wait or merge as-is (if they actually pass in CI). |
package/router/lex/tokens.go
Outdated
| @@ -41,9 +42,10 @@ func (tokens Tokens) At(i int) string { | |||
| for _, token := range tokens { | |||
| switch token.Type { | |||
| case PathToken, SlashToken: | |||
| for j := 0; j < len(token.Value); j++ { | |||
| chars := []rune(token.Value) | |||
| for j := 0; j < len(chars); j++ { | |||
There was a problem hiding this comment.
Did you try ranging over token.Value? IIRC, you can do:
for char := range token.Value {
// do something
}And it'll range over the runes.
There was a problem hiding this comment.
There was a problem hiding this comment.
Nice, I'll switch to that - I thought about it but didn't recognize that it "did the right thing" with the index key, i.e. skipping numbers for multi-byte runes.
otherwise all utf8 characters have a "common prefix" which allowed one route to return a different one somehow. adds an explicit test for matching unicode, too. having this mixed in with the other test made things pretty difficult to debug
馃憢 Loving this project so far!
I'm switching an app over to it to see how it goes and I noticed that Bud normalizes paths to lowercase. This is a problem for my app because it uses base64-encoded digests to route to content-addressed objects. I could switch to a case-insensitive database lookup, but that introduces the admittedly low possibility of collisions.
I've seen other websites in the wild that use case sensitive routes, for example url shorteners, so I don't think this is too unusual.
This PR removes the redirect and instead changes the router to use a case-insensitive match, since I figure the goal is for URLs to be case-insensitive. I'm not too opinionated here, just wanted to get a PR out to spark discussion. :)
Another option could be to convert casing for only the static portions of the path based on the route, but that's a bigger lift.
Thanks for making Bud!