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

Bring back lost 'follow' (redirects) request option #324

Merged
merged 4 commits into from Oct 24, 2017

Conversation

Projects
None yet
3 participants
@robingustafsson
Member

robingustafsson commented Sep 25, 2017

I needed this functionality and then realized we seem to have lost it in the Otto -> Goja migration, as it was previously implemented, see #232.

Usage:

http.get("https://httpbin.org/redirect/1", {follow: false});
check(res, {
    "has status 302": (r) => r.status === 302,
    "has non-redirected URL": (r) => r.url === "https://httpbin.org/redirect/1"
})

@robingustafsson robingustafsson requested a review from liclac Sep 25, 2017

@liclac

This comment has been minimized.

Show comment
Hide comment
@liclac

liclac Oct 4, 2017

Collaborator

I think this option would do better as an override for the number of redirects.

Collaborator

liclac commented Oct 4, 2017

I think this option would do better as an override for the number of redirects.

@robingustafsson

This comment has been minimized.

Show comment
Hide comment
@robingustafsson

robingustafsson Oct 5, 2017

Member

@liclac Ok, sounds reasonable. And a maxRedirects: 0 would return last response (return http.ErrUseLastResponse from CheckRedirect in Go) rather than throwing the "max redirects..." error? Because that's a real use case, that you want to interfere with redirects.

Member

robingustafsson commented Oct 5, 2017

@liclac Ok, sounds reasonable. And a maxRedirects: 0 would return last response (return http.ErrUseLastResponse from CheckRedirect in Go) rather than throwing the "max redirects..." error? Because that's a real use case, that you want to interfere with redirects.

@liclac

This comment has been minimized.

Show comment
Hide comment
@liclac

liclac Oct 5, 2017

Collaborator

What if follow could be either a number or a bool, and if it's false that happens? But at that point, it may be better to just have two different options...

Collaborator

liclac commented Oct 5, 2017

What if follow could be either a number or a bool, and if it's false that happens? But at that point, it may be better to just have two different options...

@luxifer

This comment has been minimized.

Show comment
Hide comment
@luxifer

luxifer Oct 12, 2017

Hi guys, this is a serious BC break and I really need k6 to not fail when there is a redirect...

luxifer commented Oct 12, 2017

Hi guys, this is a serious BC break and I really need k6 to not fail when there is a redirect...

@luxifer

This comment has been minimized.

Show comment
Hide comment
@luxifer

luxifer Oct 12, 2017

With maxRedirects: 0 in the options, the current respons status is 0, instead of the real http status code (should be 302 in my case).

luxifer commented Oct 12, 2017

With maxRedirects: 0 in the options, the current respons status is 0, instead of the real http status code (should be 302 in my case).

@robingustafsson

This comment has been minimized.

Show comment
Hide comment
@robingustafsson

robingustafsson Oct 13, 2017

Member

@luxifer Do you mean removing follow: true|false would be a BC break, or are you saying that changing the behavior of maxRedirects: 0 is the BC break? In the latter case would you rather have the current behavior of an error being logged and status being 0, than having the first redirected response with 3xx status returned?

I'm thinking the best way forward might be to keep the maxRedirects behavior for now (keep it as a separate discussion whether to break that compatibility), and just reintroducing the follow: true|false as it's implemented in this PR right now. I'm also fine with implementing the follow: true|false|number version, although I'm a bit hesitant to having follow doubling as a per-request override for the global maxRedirects option under a different name.

@liclac Thoughts?

Member

robingustafsson commented Oct 13, 2017

@luxifer Do you mean removing follow: true|false would be a BC break, or are you saying that changing the behavior of maxRedirects: 0 is the BC break? In the latter case would you rather have the current behavior of an error being logged and status being 0, than having the first redirected response with 3xx status returned?

I'm thinking the best way forward might be to keep the maxRedirects behavior for now (keep it as a separate discussion whether to break that compatibility), and just reintroducing the follow: true|false as it's implemented in this PR right now. I'm also fine with implementing the follow: true|false|number version, although I'm a bit hesitant to having follow doubling as a per-request override for the global maxRedirects option under a different name.

@liclac Thoughts?

@luxifer

This comment has been minimized.

Show comment
Hide comment
@luxifer

luxifer Oct 13, 2017

That's exactly what I meant. The current behavior pour throwing an error when mawRedirects is 0 and the actual response code is 3xx is not good. I would like the follow option on http requests to be reintroduced thot that I can check that I am currently redirected.

Thanks!

luxifer commented Oct 13, 2017

That's exactly what I meant. The current behavior pour throwing an error when mawRedirects is 0 and the actual response code is 3xx is not good. I would like the follow option on http requests to be reintroduced thot that I can check that I am currently redirected.

Thanks!

@robingustafsson

This comment has been minimized.

Show comment
Hide comment
@robingustafsson
Member

robingustafsson commented Oct 18, 2017

Ping @liclac

@liclac

This comment has been minimized.

Show comment
Hide comment
@liclac

liclac Oct 18, 2017

Collaborator

What about this:

  • Default to following 10 redirects
  • Use { redirects: 5 }, etc. to change this
  • Return the last response, but log a warning if the redirects option isn't specified to prevent confusion ("why is my check failing??")
Collaborator

liclac commented Oct 18, 2017

What about this:

  • Default to following 10 redirects
  • Use { redirects: 5 }, etc. to change this
  • Return the last response, but log a warning if the redirects option isn't specified to prevent confusion ("why is my check failing??")
@robingustafsson

This comment has been minimized.

Show comment
Hide comment
@robingustafsson

robingustafsson Oct 19, 2017

Member

@liclac Ok, that sounds like a good idea to me. I'll change the implementation.

Member

robingustafsson commented Oct 19, 2017

@liclac Ok, that sounds like a good idea to me. I'll change the implementation.

@robingustafsson

This comment has been minimized.

Show comment
Hide comment
@robingustafsson

robingustafsson Oct 20, 2017

Member

@liclac Now updated.

Member

robingustafsson commented Oct 20, 2017

@liclac Now updated.

Show outdated Hide outdated js/modules/k6/http/http.go Outdated
Show outdated Hide outdated js/modules/k6/http/http.go Outdated
Show outdated Hide outdated js/modules/k6/http/http.go Outdated
Show outdated Hide outdated js/modules/k6/http/http.go Outdated
@robingustafsson

This comment has been minimized.

Show comment
Hide comment
@robingustafsson

robingustafsson Oct 23, 2017

Member

@liclac Requested changes have been made.

Member

robingustafsson commented Oct 23, 2017

@liclac Requested changes have been made.

@liclac

liclac approved these changes Oct 24, 2017

@liclac liclac merged commit e107dbe into master Oct 24, 2017

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@liclac liclac deleted the feature/follow-redirects branch Oct 24, 2017

@liclac liclac restored the feature/follow-redirects branch Nov 23, 2017

@na-- na-- deleted the feature/follow-redirects branch Mar 12, 2018

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