http-client icon indicating copy to clipboard operation
http-client copied to clipboard

Cookie Ord instance is not a reasonable ordering

Open tysonzero opened this issue 4 years ago • 3 comments

For any cookie c, c == c and c < c are True, and c <= c is False.

If the above functionality is useful then I propose it be moved to a separate function outside of Ord. Either way I propose a more normal Ord instance be defined that aligns with Eq and reasonable expectations of what an ordering entails.

tysonzero avatar Aug 15 '19 01:08 tysonzero

There's not really enough information for me to understand what you're proposing here.

snoyberg avatar Aug 15 '19 16:08 snoyberg

I suppose more formally, currently:

∀ (c :: Cookie). c == c && c < c && not (c <= c)

Whereas what we pretty much always want is:

∀ c. c == c && not (c < c) && c <= c

The specific questionable code is:

instance Ord Cookie where
  compare c1 c2
    | S.length (cookie_path c1) > S.length (cookie_path c2) = LT
    | S.length (cookie_path c1) < S.length (cookie_path c2) = GT
    | cookie_creation_time c1 > cookie_creation_time c2 = GT
    | otherwise = LT

Which should probably be something more like (open to other suggestions, just going off of the Eq instance):

instance Ord Cookie where
  compare a b = name_comparison <> domain_comparison <> path_comparison
    where name_comparison = compare (cookie_name a) (cookie_name b)
          domain_comparison = compare (CI.foldCase (cookie_domain a)) (CI.foldCase (cookie_domain b))
          path_comparison = compare (cookie_path a) (cookie_path b)

Currently putting Cookies in things like Map or Set has relatively catastrophic results.

tysonzero avatar Aug 15 '19 22:08 tysonzero

Seems reasonable, PR welcome.

snoyberg avatar Aug 16 '19 02:08 snoyberg