Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Support default (session) headers for httpclient #325

merged 3 commits into from Nov 4, 2013


None yet
2 participants

miguelff commented Oct 31, 2013

This PR fixes #322

I have also added support for all the headers stored in the SessionManager, which are set in the request by HTTPClient::Session[1]

[1] https://github.com/nahi/httpclient/blob/v2.3.4.1/lib/httpclient/session.rb#L723


bblimke commented Oct 31, 2013

@miguelff Thank you for submitting the pull request so quickly!

I haven't had a chance to review the correctness of functionality of the adapter yet,
but there are some things that need to be improved before this pull request could be merged.

First, I suggest dedicating separate specs to cover the behaviours you implemented,
instead of modifying existing specs, which are already dedicated to test specific cases.

The specs dedicated to httpclient only, should reside in http_client_spec.rb only and not affect common specs.
The consequence of modifying common specs, was that you had to modify Excon adapter helper, which has nothing to do with httpclient.

In addition, the specs you modified don't actually test any of the new behaviour you implemented,
because you are passing omit_session_headers flag, which effectively disables the new behaviour.

I also don't understand the need for omit_session_headers flag. I don't see it being part of a normal HTTPClient interface. Do you really need it or can it be removed?



miguelff commented Oct 31, 2013

Hey @bblimke, thanks for you review!

I added specific specs to test the new functionality. [1] Do you mean in a different file, instead of in the acceptance spec dedicated to HTTPClient? There are more specs there then those in the shared example that test actual behavior of HTTPClient, that's why I also put them there. I don't have any problem to put it in another file if you prefer.

With regards to the omit_session_headesr flag, If it didn't existed, it couldn't be possible to add an expectation to .with(:headers => ...) testing only those headers you are explicitly providing. This happens because the default ones would always be added by HTTPClient. To avoid this, I added an optional parameter to the HTTPClientSpecHelper#http_request in order to be able to avoid session headers.

This is illustrated in shared examples [2] and [3], which expect a set of headers specified explicitly on the request. In absence of the flag, HTTPClient will also add the defaults headers sent by the session (and this is the actual behavior of HTTPClient!) so the spec will fail. Being these specs shared among all the adapter specs, I had to modify them to include the flag, letting HTTPClient to avoid the inclussion of the session headers.

Why I touched Excon? Because ExconSpecHelper forwards the options to the adapter, who doesn't allow custom options like (:adapter_flags). 😕

Notice that in any case:

  • No adapters have been modified besides HTTPClient (only their spec helpers with non-intrusive options)
  • Changes to the shared examples affect the exercise of other adapter specs. As the :adapter_flags is an option provided to an spec helper, and no to the adapter itself.

Alternative approaches will be:

  • A) Omit session headers by default in the specs, and provide a flag to enable them when wanted to change Headers behavior (ex. :enable_session_headers). The trede-off is that this will no exercise actual HTTPClient behavior, as HTTPClient always sends session headers in its real implementation.
  • B) Remove flags, and let HTTPClient behave like it does. Shared examples [2] and [3] will not work, because the expected behavior is not shared by HTTPCLient, which behaves differently then the othet adapters (sending a set of headers always). [2] and [3] would need to be refactored and instead of shared-examples would need to be repeated for each adapter, and modified by HTTPClient
  • C) Equal to B), but in [2] and [3], modify SAMPLE_HEADERS to include those headers that are also sent always by HTTPClient (Accept, User-Agent and Date).
  • D) Leave it as is, as no adapter behavior has been really modified besides

After writing this, I would definitely go for C)

What do you think?



[1] https://github.com/miguelff/webmock/commit/7f3d583aebaf9ff03f0f0599826d3cea1ad33bec#diff-670f66dbf3973e606166124aa08937e2R130
[2] https://github.com/miguelff/webmock/blob/master/spec/acceptance/shared/request_expectations.rb#L324
[3] https://github.com/miguelff/webmock/blob/master/spec/acceptance/shared/stubbing_requests.rb#L246


miguelff commented Oct 31, 2013

I have implemented C, I leave it in the PR for your review

@miguelff miguelff Remove flags, and let HTTPClient behave like it does:
- modify `SAMPLE_HEADERS` to include those headers that are also sent always by HTTPClient (Accept, User-Agent and Date).

bblimke commented Nov 1, 2013

Thank you for these changes and explaining your reasons.
I'm not very convinced about these changes, since the only change is really in building the request signature, used internally by WebMock.
This change doesn't depend on httpclient current behaviour. I.e if HTTPClient stops sending these headers in the future, WebMock will still pretend it does send them. In that case, relying on WebMock, is not very reliable.

As I understand, this change is only useful, if someone wants to test that httpclient sends these session header. It's an edge case right?
Can you please share more insight on why is it important to you, to test these headers presence?
Does the functionality in your application depend on them?

Anyway, the current version of pull request looks fine, except the SAMPLE_HEADERS change. Why did you have to modify them?
I don't really like the idea of modifying shared code (shared tests) to adjust for just one adapter custom functionality.
Second, you don't have to modify SAMPLE_HEADERS at all. WebMock only requires headers declared in a stub,
to be a subset of request headers, to match the request.


miguelff commented Nov 3, 2013

Thank you again for your review, Bartosz.

Answering to your questions in reverse order:

With regards to modify SAMPLE_HEADERS. You are right! there's no need of modifying shared code and I have reverted that in my last commit.

Regarding wether it is an edge case or not for httpclient: For me it's not an edge case at all, I'm modifying an API client (https://github.com/bebanjo/almodovar) that depends on patron, to depend on http-client when used in Jruby. Some of its functionality relies on digest authentication and content negotiation based on the user-agent. In order to safely swap implementations I need to test this functionality, and I eventually realized that the mock of HTTPClient didn't behave like the real HTTPClient.

I understand that future implementations of http-client can vary with regards to headers sending, but IMO, this would not be likely to happen given it's maturity and the fact it hasn't changed the way it sends headers since there are records in Github.

To sum up, sending headers the way suggested is how httpclient actually behaves. I try to imagine which will be the downside of letting the adapter behave like the original implementation, but I can't see it, honestly.

Again, Thanks for your time.


bblimke commented Nov 4, 2013

@miguelff thanks for the info. Now I understand why you need these headers to be present.

Everything looks good. Thank you!

@bblimke bblimke added a commit that referenced this pull request Nov 4, 2013

@bblimke bblimke Merge pull request #325 from miguelff/fix-httpclient-adapter-headers
Support default (session) headers for httpclient

@bblimke bblimke merged commit acb998b into bblimke:master Nov 4, 2013

1 check passed

default The Travis CI build passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment