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

A collection of general API improvements #36

Merged
merged 2 commits into from
Jan 17, 2017

Conversation

bitwalker
Copy link
Collaborator

The following is a list of the changes made. To me, the ergonomics of this improved API are much nicer, as it's simpler to work with Conn, and the APIs tend to return what you'd expect if you think of Conn as any other data structure.

  • Working with Conn APIs requires calling new/0 or new/1 first
    old APIs are deprecated
  • new/1 parses the provided string with URI.parse/1 and sets the Conn
    struct fields accordingly (so you can pass full or partial url
    fragments and get what you would expect, including those containing
    query strings)
  • All headers are normalized to lower-case on put/get, and internally
    headers are a simple key/value map instead of the previous key =>
    key/value map, which was unnecessary.
  • Implemented get_req_headers/1, deprecated get_req_header/1
  • Implemented put_req_headers/2, deprecated put_req_header/2
  • get_req_header/2 returns String.t instead of {String.t, String.t}
  • get_req_headers/1 returns %{String.t => String.t} instead of
    %{String.t => %{String.t => String.t}}
  • Added a number of typespecs, and fixed invalid ones
  • Some small style fixes, documentation updates
  • Fixed a bug with the Header middleware which was not overriding
    headers in the connection if already set. This one may be up for
    discussion if what's really desired.
  • I recall encountering one or two other places where the tests were asserting
    the wrong thing, and so tests were passing even though the code was broken.

I'm opening this as a PR rather than just making the changes because it's a large changeset and you may not approve, but I think these are important changes to make and will make Maxwell much nicer to work with. I'll be using my fork in the meantime for that reason.

@secretworry
Copy link
Collaborator

secretworry commented Jan 16, 2017

@bitwalker the 3rd change needs more discussion.
The reason why we want to keep the key => key/value map internally is that we want to make sure the user can control the format of names of HTTP headers. Even though the standard says the names of headers are not case sensitive, but a faulty implemented server may put some restrictions on the format of the header names, so we internally make sure the middlewares can access headers without worrying about the case of characters, and the adapters still have the ability to transfer the headers as the user passed in.
And that's why we used the format, normalized_name -> {orignal_name, value}.

@bitwalker
Copy link
Collaborator Author

@secretworry There are a couple of reasons for why I disagree that this is necessary:

  1. Of the currently supported Erlang HTTP clients, neither hackney or httpc support case-sensitive headers at all, they are always converted to lowercase. ibrowse internally works with headers as lower case, but sets required headers which are not passed in with PascalCase. So to support this, someone must be using ibrowse, and they already have to do extra work to ensure the client does the right thing.
  2. It makes more sense for someone who needs this specific functionality to implement a middleware to transform all request headers to the proper case, as this is trivially done, and if it's a use case a user today has with Maxwell, we could even make it one of the default middlewares.
  3. Supporting this one, very specific, use case, enforces a very awkward API on all users of Maxwell. The vast majority of users don't care how Maxwell stores headers internally, when they call get_req_headers/1, they want a plain map of keys to values, when they call get_req_header/2 they want the value associated with the provided key, not a tuple containing the header key in potentially another case.
  4. We're forcing the rest of the code to work with this awkward structure to support a single use case which is best implemented one time in a middleware, leaving the rest of the code clean.

I don't think there is any good reason why it needs to remain, the same use case can be supported far more cleanly elsewhere.

@bitwalker
Copy link
Collaborator Author

@secretworry I've updated the PR to include a new HeaderCase middleware which enforces header casing in one of three styles, lower, upper, and title-case. This should allow anyone that was relying on the previous behaviour to easily migrate to the new version (and with a more explicit declarative intent in their code as well, which is another benefit).

- Working with Conn APIs requires calling new/0 or new/1 first
  old APIs are deprecated
- new/1 parses the provided string with URI.parse/1 and sets the Conn
  struct fields accordingly (so you can pass full or partial url
  fragments and get what you would expect, including those containing
  query strings)
- All headers are normalized to lower-case on put/get, and internally
  headers are a simple key/value map instead of the previous key =>
  key/value map, which was unnecessary.
- Implemented get_req_headers/1, deprecated get_req_header/1
- Implemented put_req_headers/2, deprecated put_req_header/2
- get_req_header/2 returns String.t instead of {String.t, String.t}
- get_req_headers/1 returns %{String.t => String.t} instead of
  %{String.t => %{String.t => String.t}}
- Added a number of typespecs, and fixed invalid ones
- Some small style fixes, documentation updates
- Fixed a bug with the Header middleware which was not overriding
  headers in the connection if already set. This one may be up for
  discussion if what's really desired.
- Added HeaderCase middleware for enforcing header casing
@bitwalker bitwalker force-pushed the api-improvements branch 2 times, most recently from 2a0ebd4 to 1c24b88 Compare January 16, 2017 20:41
@coveralls
Copy link

coveralls commented Jan 16, 2017

Coverage Status

Coverage increased (+0.9%) to 99.791% when pulling 1c24b88 on bitwalker:api-improvements into a60d98f on zhongwencool:master.

extra_headers
|> Enum.reduce(headers, fn {header_name, header_value}, acc ->
acc
|> Map.delete(header_name)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why need delete header_name first?
It's seems to have guaranteed that the head is all lowercase.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After thinking about it some more, I agree that this is unnecessary here, as it doesn't actually enforce what I intended which is to ensure there are never any duplicates. I'll update the PR


The following is a full implementation of a client showing various features of Maxwell.

```elixir
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete those, user can't find example:
how to send file/stream?
Maybe we need more example in adapter's document?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm actually planning on improving the docs around that, but I decided to hold off until this PR is merged. I think improving the adapter docs as well as having a more in depth description around streaming and files in the readme is preferable to a big example client which leaves out the details. I can make that part of the PR if you want, or after it's merged, your call.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:), I think docs can be updated after it's merged.
Thank you very much!

@coveralls
Copy link

coveralls commented Jan 17, 2017

Coverage Status

Coverage increased (+0.9%) to 99.791% when pulling 51dcd96 on bitwalker:api-improvements into a60d98f on zhongwencool:master.

@bitwalker bitwalker merged commit d7f85fb into zhongwencool:master Jan 17, 2017
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.

4 participants