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

fix NPE in Populate the :trace-redirects response field #327 PR #341

Merged
merged 3 commits into from Dec 13, 2016

Conversation

Projects
None yet
2 participants
@jiacai2050
Contributor

jiacai2050 commented Nov 26, 2016

In order to fix NPE in #327 , I have to update two tests dealing with redirect , since I have removed wrap-redirects middleware, so it is impossible for clj-http to control redirect behavior

@jiacai2050

This comment has been minimized.

Show comment
Hide comment
@jiacai2050

jiacai2050 Nov 30, 2016

Contributor

Any question with this PR ?

Contributor

jiacai2050 commented Nov 30, 2016

Any question with this PR ?

@dakrone

I left a comment on this, but what I'm really concerned about is that there is now no way to disable redirects. There are some cases where a redirect needs to be handled by client code (think SAML SSO) instead of by the Apache HTTP library, so I think we should still have a way to disabling redirects.

Thoughts?

Show outdated Hide outdated src/clj_http/core.clj
@jiacai2050

This comment has been minimized.

Show comment
Hide comment
@jiacai2050

jiacai2050 Dec 7, 2016

Contributor

Redirect control is reasonable, We can use RequestConfig.setRedirectsEnabled() method to toggle it.

Contributor

jiacai2050 commented Dec 7, 2016

Redirect control is reasonable, We can use RequestConfig.setRedirectsEnabled() method to toggle it.

@dakrone

This comment has been minimized.

Show comment
Hide comment
@dakrone

dakrone Dec 9, 2016

Owner

Redirect control is reasonable, We can use RequestConfig.setRedirectsEnabled() method to toggle it.

Would you be willing to extend the PR to expose that as a boolean option?

Owner

dakrone commented Dec 9, 2016

Redirect control is reasonable, We can use RequestConfig.setRedirectsEnabled() method to toggle it.

Would you be willing to extend the PR to expose that as a boolean option?

@jiacai2050

This comment has been minimized.

Show comment
Hide comment
@jiacai2050

jiacai2050 Dec 10, 2016

Contributor

clj-http already has support for redirect control in here, I add one more test case for it

Contributor

jiacai2050 commented Dec 10, 2016

clj-http already has support for redirect control in here, I add one more test case for it

@dakrone dakrone merged commit 5bae92e into dakrone:master Dec 13, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@dakrone

This comment has been minimized.

Show comment
Hide comment
@dakrone

dakrone Dec 13, 2016

Owner

Merged, thanks!

Owner

dakrone commented Dec 13, 2016

Merged, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment