Skip to content
This repository has been archived by the owner on Mar 31, 2019. It is now read-only.

added redirect (307) Handling #45

Closed
wants to merge 1 commit into from

Conversation

cookingkode
Copy link

According to https://www.firebase.com/docs/rest/api/ we need client to handle redirect support.
Due to golang/go#7912 : the redirect is not supported for PUT and POST

// 307 handling.
// replay the previous request using the new url in the Location header
// Ref : https://code.google.com/p/go/issues/detail?id=7912
if strings.HasPrefix(resp.Status, "307 ") {
Copy link
Owner

Choose a reason for hiding this comment

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

Can you use the resp.StatusCode variable instead and compare against the http package constant?

if resp.StatusCode == http.StatusTemporaryRedirect {

}

@zabawaba99
Copy link
Owner

Can you point me to the documentation that says that redirects need to be supported for PUTs and POSTs? The only place I see a mention of a redirect is when streaming data which uses a GET to open the SSE connection.

@jgeiger
Copy link
Contributor

jgeiger commented Jun 26, 2016

I was just cleaning up my personal forks and saw this.

Might be worth referencing:#32

@cookingkode
Copy link
Author

Ref : https://www.firebase.com/docs/rest/api/#section-streaming
"Respect HTTP Redirects, in particular HTTP status code 307"
Will correct the MR as per suggestion and re-submit

@zabawaba99
Copy link
Owner

I understand that because of golang/go#7912 PUT and POST methods will not follow redirects, however I don't necessarily see where that is an issue for the current implementation. The firebase documentation says that redirects need to be respected only when doing http streaming. Currently the Watch function uses a GET method when opening the connection which follows the redirects for us.

Can you provide an example where the library is breaking because it's not following the redirects?

@pdakhane
Copy link

pdakhane commented Aug 18, 2016

golang/go#7912 is this issue fixed? not sure if fix discussed here made it to master branch??

@zabawaba99
Copy link
Owner

@pdakhane This change has not made it to the master branch yet. The reason for this is because redirects are only needed for the HTTP Streaming api. Since the HTTP Streaming API dials to firebase using a GET method, redirects are already respected.

I still cannot find any documentation that mentions the need to support redirects for the realtime database REST api. I'm going to close this PR until the need for redirect support comes back.

@zabawaba99 zabawaba99 closed this Aug 22, 2016
@pdakhane
Copy link

@zabawaba99 Thank you, could you please point me to library/package where redirects are already respected??

@zabawaba99
Copy link
Owner

@pdakhane The stdlib http package automatically respects redirects on get requests. Here is an example (you'll have to run it locally since you can't do http on the playground) https://play.golang.org/p/37F54Po7Lg

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants