Skip to content
This repository

PathPiece instance for Text leads to 404s #421

Closed
joeyh opened this Issue September 11, 2012 · 15 comments

3 participants

Joey Hess Michael Snoyman Felipe Lessa
Joey Hess

Replicating the bug:

Make a route with a #Text component. Then use T.empty when generating an url. This results in an url like "http://localhost/myroute/". When the web browser hits that, Yesod redirects it to "http://localhost/myroute", which is a 404.

Discussion:

It's iffy, I suppose, to use Text in a route. Nothing is communicated about the actual type of data. But sometimes you literally have a freeform peice of text, which could even be entered by a user and so be legitimately empty, and when this happened to me, I assumed it would be safe to just use Text, rather than wrapping it in a better data type.

I feel this is either a bug in the PathPiece instance for Text, or in the existence of that instance if it should not be used, or in the part of Yesod that does the redirect from "/" to "".

Michael Snoyman
Owner

I wouldn't really call it a bug in any of them. There is a user-configurable method called cleanPath which, by default, will strip/redirect empty path segments. If you change that to allow the empty segments, then you won't get 404s.

Joey Hess

Maybe the bug then is having cleanPath by default? It just seems wrong that doing this simple thing will break the nice type safe urls, in the default configuration.

Felipe Lessa
Owner

IMHO it's a documentation bug. It seems we can't have our cake and eat it too wrt. to this bug, so we'd have to choose either between an instance that works correctly for empty Texts or a cleanPath that does not normalize paths as it should. I currently prefer the latter.

BTW, I've been bitten by this bug as well and have some workarounds in place but I never thought about reporting it =).

Michael Snoyman
Owner

Here's my idea on the "correct" solution: path segments should be encoded at the type level to indicate that they are not allowed to be empty. This would be via some type like:

newtype NonEmptyText = NonEmptyText Text

Of course, that would massively affect user code... which is why I didn't do it. I consider the current situation a fair trade-off. The problem with changing the behavior of cleanPath is that the requests http://foo/bar/ and http://foo/bar would now be interpreted completely differently, which most users would not expect. (Currently, the former redirects to the latter.)

There's one other possibility I can think of: render empty path segments into some dummy value that we're sure will never be used in a real application, and have cleanPath turn that back into an empty text. It seems like an ugly hack to me, but it might be worth testing out.

Felipe Lessa
Owner

@snoyberg The other possibility is what I've done on my code right now but in a non-generic way. Here's a generic idea:

import qualified Data.Text as T

instance PathPiece T.Text where
  toPathPiece t =
    case T.uncons t of
      Nothing       -> "-"
      Just ('-', r) -> "--" <> r
      Just _        -> t
  fromPathPiece p = 
    case T.uncons p of
      Just ('-', r) -> Right r
      _             -> Right p

So an empty Text is encoded as a dash. Everything that starts with a dash gets a new dash at the front of the string. Everything that is non-empty and doesn't start with a dash stays the same. Since the number of useful texts that begin with a dash should be small, I guess this is a good compromise.

Felipe Lessa
Owner

BTW, just for fun I've proved that fromPathPiece . toPathPiece = Right for the definitions above using Agda. I'll leave the proof here in case anyone is interested in seeing it =).

data Char : Set where
  dash  : Char
  other : Nat  Char

toPathPiece : List Char  List Char
toPathPiece nil            = dash :: nil
toPathPiece (dash :: rest) = dash :: dash :: rest
toPathPiece x              = x

fromPathPiece : List Char  List Char
fromPathPiece (dash :: rest) = rest
fromPathPiece x              = x

toPathPiece/fromPathPiece : (xs : List Char)  fromPathPiece (toPathPiece xs) ≡ xs
toPathPiece/fromPathPiece nil            = refl
toPathPiece/fromPathPiece (dash    :: _) = refl
toPathPiece/fromPathPiece (other _ :: _) = refl

null/toPathPiece : (xs : List Char)  null (toPathPiece xs) ≡ false
null/toPathPiece nil            = refl
null/toPathPiece (dash    :: _) = refl
null/toPathPiece (other _ :: _) = refl
Michael Snoyman
Owner

I like the dash approach, though if we implement it in the Yesod typeclass instead, we can handle all PathPiece instances and let users control this behavior.

This kind of change would slightly break code (old URLs starting with a dash would be broken), but I'm still tempted to push the change in mid-release-cycle. What do you think about that? I'd probably start off with an email to the mailing list.

Felipe Lessa
Owner

Sounds nice. I can't imagine any legitimate use for putting a dash as the first character on purpose (besides coming as user input).

Michael Snoyman
Owner

Actually, how about this approach, which should cause less breakage:

import qualified Data.Text as T

instance PathPiece T.Text where
  toPathPiece t
    | T.all (== '-') t = T.cons '-' t
    | otherwise = t
  fromPathPiece t
    | T.all (== '-') t = Right $ T.drop 1 t
    | otherwise = Right t
Felipe Lessa
Owner

Nice idea, I like it! BTW, it took me a lot more effort to prove its correctness, I wonder how a seasoned Agda-er would write it...

allDashes : List Char  Bool
allDashes nil          = true
allDashes (dash :: xs) = allDashes xs
allDashes _            = false

toPathPiece : List Char  List Char
toPathPiece xs with allDashes xs
... | true  = dash :: xs
... | false = xs

fromPathPiece : List Char  List Char
fromPathPiece nil = nil
fromPathPiece (x :: xs) with allDashes (x :: xs)
... | true  = xs
... | false = x :: xs


fromPathPiece/false : (xs : List Char)  allDashes xs ≡ false  fromPathPiece xs ≡ xs
fromPathPiece/false nil      _               = refl
fromPathPiece/false (_ :: _) prf rewrite prf = refl

fromPathPiece/true : (xs : List Char)  allDashes xs ≡ true  fromPathPiece (dash :: xs) ≡ xs
fromPathPiece/true nil      _               = refl
fromPathPiece/true (_ :: _) prf rewrite prf = refl

toPathPiece/fromPathPiece : (xs : List Char)  fromPathPiece (toPathPiece xs) ≡ xs
toPathPiece/fromPathPiece xs with inspect (allDashes xs)
... | it false prf rewrite prf | fromPathPiece/false xs prf = refl
... | it true  prf rewrite prf | fromPathPiece/true  xs prf = refl


null/toPathPiece : (xs : List Char)  null (toPathPiece xs) ≡ false
null/toPathPiece nil = refl
null/toPathPiece (x :: xs) with allDashes (x :: xs)
... | true  = refl
... | false = refl
Michael Snoyman snoyberg referenced this issue from a commit in yesodweb/path-pieces September 21, 2012
Michael Snoyman No null checking (yesodweb/yesod#421) 658f725
Michael Snoyman
Owner

Changes pushed, can you both have a look before release?

Felipe Lessa
Owner

Looks great! Thanks!

Michael Snoyman
Owner

OK, everything should now be working and live on Hackage, closing. Please reopen if any problems persist.

Michael Snoyman snoyberg closed this September 24, 2012
Yann Esposito yogsototh referenced this issue from a commit in yogsototh/yesod September 24, 2012
Yann Esposito Merge branch 'master' of https://github.com/yesodweb/yesod
* 'master' of https://github.com/yesodweb/yesod:
  Removed unnecessary conditionals
  warpEnv
  Use shouldLog in scaffold
  Export log*S (#405)
  Use haddock for Yesod.Form.Types.Field
  Add deprecation for messageLogger (#405)
  LogSource (#405)
  Precede null path segments with dashes (#421)
  MonadLift instance for AForm
  network 2.4
  Fix documentation.
b04063f
Joey Hess

Wow. While the first several people I mentioned this problem to seemed content with the status quo, I'm seriously impressed with the speed and attention to detail with which this bug report was addressed in the end. The multiple agda proofs push it over the line from good job to you're-making-the-rest-of-us-look-bad territory. :) Astounding work. Would file bug again.

Felipe Lessa
Owner

@joeyh Glad you're enjoying your stay here =). We'll be happy to receive other bug reports in the future, and especially so if they came on pull requests ;-).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.