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

Redirect Error Reporting #987

Closed
jwise-sncr opened this issue Apr 5, 2019 · 8 comments
Closed

Redirect Error Reporting #987

jwise-sncr opened this issue Apr 5, 2019 · 8 comments

Comments

@jwise-sncr
Copy link

In v0.23.0 a redirect to a down server provided an error including the failing URL like:

error="Post http://127.0.0.1?session_state=29de8415-ea47-4919-a640-4db59cbb953e&code=02e7e7c9-56e7-4c43-a709-af145d4fe5d8.29de8415-ea47-4919-a640-4db59cbb953e.2d78ebc3-9903-4753-a00d-28c051498a49: dial tcp 127.0.0.1:80: connect: connection refused"

In v0.24.0 while the redirect URL still shows on the console output in a WARN entry, the failing redirect URL is no longer available in the error attribute of the response. The error looks like:

error="dial tcp 127.0.0.1:80: connect: connection refused"

In v0.24.0 how can I programmatically determine which URL had the failure?

@mstoykov
Copy link
Contributor

mstoykov commented Apr 5, 2019

This was changed with #907 , and the error message should now be dial tcp: connection refused.
You should be able to get the url through response.request.url . I would say this is a lot more likely to work

@jwise-sncr
Copy link
Author

Unfortunately, response.request.url shows the original request url and not the redirect url. How can I get to the redirect URL?

@mstoykov
Copy link
Contributor

mstoykov commented Apr 8, 2019

Hm ... I did some tests. Currently this seems like a thing that is not supported. I would argue that it was previously not supported as well ... but I suppose your workaround was working.

My current suggestion for workaround is either rollback to the previous stable version, especially if you don't need some of the new features or (more involved) you write your own get/post/put methods which use http.* to make request with "redirects":0 and than follow the redirect.

Because I think that this while rare case for most situation still worth supporting in some ways I have a couple of suggestion on our side:

  1. we can just add a field to the response that is actual_last_url (name to be discussied )
  2. we can change the current response.url to be last url for which this response is - I don't like this
  3. we can change the current response.request.url to be the last url that was request - I don't like this even more.
  4. Having an array of previous request/responses as a field. This seems like the best solution but has some problems.
    4.1. It will take memory, especially in cases with a lot of redirects
    4.2. body's can be big and we probably should save them ... again memory
    4.3. I am pretty sure the current code will need a lot of work to get it done.
    4.4. This probably should be behind of flag as I doubt that it will be useful in the majority of cases
  5. Your suggestion @jwise-sncr :)

cc @na-- @robingustafsson

@robingustafsson
Copy link
Member

Regarding 2), response.url is supposed to be the URL of the last request that resulted in the returned response object, is that not the case @mstoykov? That's also what we say in the docs:

The URL that was ultimately fetched (i.e. after any potential redirects).

I think that 4) sounds like a good solution to being able to easily check the chain of redirects. I'd argue that we should keep this simple though and only store an array of URLs. Anything more advanced can be solved by wrapping the request APIs and manually handling redirects (we could perhaps even provide some helper JS functions to handle this).

@mstoykov
Copy link
Contributor

mstoykov commented Apr 8, 2019

@robingustafsson hm ... yeah actually this given that response.request.url makes sense and if we have documented it that way and we also have a test for it is a bug .

@jwise-sncr
Copy link
Author

jwise-sncr commented Apr 8, 2019

@mstoykov,

I agree with your assessment of the solution space. For my use case, any of the solutions would suffice. I'm testing an OAuth2 authorization code flow from an IDP perspective. I'm using the redirect URL in the response to pull the code value so I don't have to standup a separate client service at the redirect URL to capture it and coordinate with.

@mstoykov
Copy link
Contributor

mstoykov commented Apr 8, 2019

@jwise-sncr we actually decided to fix it by 2) because this was actually a bug see #990 . I am probably going to merge it tomorrow into master but if you can test it I would be glad :)

@jwise-sncr
Copy link
Author

I attempted to test it. It's my first time with golang. I probably did something wrong as I got unrelated errors in some custom modules that worked fine under both 0.23.0 and 0.24.0.

ankur22 added a commit that referenced this issue Dec 15, 2023
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

No branches or pull requests

3 participants