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

Use CaseInsensitiveString for Headers #138

Conversation

joneshf
Copy link
Contributor

@joneshf joneshf commented Apr 25, 2019

Re: #136

Following the discussion in the above issue, this converts from Object String to Map CaseInsensitiveString String. Let me know if anything should be done differently.

Also addresses part of #113.

cprussin and others added 3 commits February 26, 2019 22:18
There's currently a bug with `Headers`:
if a header is created with uppercase characters, it can never be found.
The problem is that we only look for lowercase characters in the
`Lookup` instance for `Headers`.
In an effort to be more true to HTTP,
we make the header keys case-insensitive.
This fixes the issue of looking up a header where the casing is different,
Since `CaseInsensitiveString`s compare in a way that ignore casing.

The API for consumers for `Headers` stays the same,
but we get more correct code.
A win for all!
@cprussin
Copy link
Collaborator

CI is broken right now, I need to fix it, so please ignore the failures... sorry about that!

@cprussin
Copy link
Collaborator

@joneshf Sweet! This is really great, thanks a bunch!

Just one question, I don't think there's any value any more in making Headers a newtype. The only reason it was originally a newtype was so that it could have an instance of Lookup that did case-insensitive lookup, but now that Headers is a Map CaseInsensitiveString String there's no reason to have a unique instance. Thoughts?

@joneshf
Copy link
Contributor Author

joneshf commented Apr 26, 2019

Sounds good to me.

@joneshf
Copy link
Contributor Author

joneshf commented Apr 26, 2019

Eh, most of the tests in Test.HTTPure.HeadersSpec will break:

1) HTTPure Headers show is a string representing the headers in HTTP format
  "(fromFoldable [(Tuple (CaseInsensitiveString \"Test1\") \"1\"),(Tuple (CaseInsensitiveString \"Test2\") \"2\")])" ≠ "Test1: 1\nTest2: 2\n\n"
2) HTTPure Headers append when there is a duplicated key uses the last appended value
  (fromFoldable [(Tuple (CaseInsensitiveString "Test") "foo")]) ≠ (fromFoldable [(Tuple (CaseInsensitiveString "Test") "bar")])
3) HTTPure Headers empty is an empty Map in an empty Headers
  "(fromFoldable [])" ≠ "\n"
4) HTTPure Headers header creates a singleton Headers
  "(fromFoldable [(Tuple (CaseInsensitiveString \"X-Test\") \"test\")])" ≠ "X-Test: test\n\n"
5) HTTPure Integration runs the middleware example
  "middleware" ≠ "router"

Some of this is the different behavior of Show and others are different behavior for Semigroup. Still worth it.

@cprussin
Copy link
Collaborator

Ah, right. I'd forgotten about those differences. Thank you for checking it out!

@joneshf
Copy link
Contributor Author

joneshf commented Apr 26, 2019

Still worth it.

Sorry, this was supposed to be, "Still worth it?"

@cprussin
Copy link
Collaborator

I think that we could clean up these APIs a bit and get rid of the newtype, but I think it's out of scope for this change. So let's go ahead and merge this and I'll add a separate ticket to remove the newtype.

@cprussin cprussin merged commit 5d7c2c8 into citizennet:master Apr 26, 2019
@joneshf joneshf deleted the joneshf/use-caseinsensitivestring-for-headers branch April 26, 2019 11:16
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