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

Redirects broken with latest Flask release 1.0.3 #96

Closed
RookieRick opened this issue May 23, 2019 · 3 comments
Closed

Redirects broken with latest Flask release 1.0.3 #96

RookieRick opened this issue May 23, 2019 · 3 comments

Comments

@RookieRick
Copy link
Contributor

Appears to be due to pallets/flask#2986

At first glance it looks to me like we're no longer getting an "error" on redirect (and we end up with default mimetype of text/html in the 301 response).

Could use another set of eyes (particularly people more intimately familiar with the internals of Flask than I ;) ) to help understand what's happening and how we can cleanly address. To reproduce, simply update Flask from 1.0.2 to 1.0.3 and run our unit tests.

@RookieRick RookieRick added help wanted Extra attention is needed and removed help wanted Extra attention is needed labels May 23, 2019
@RookieRick
Copy link
Contributor Author

OK after looking at that referenced Flask PR seems there's a suggested workaround there; will give that a shot (which I suspect means we'll have three separate conditions we test for when setting MOVED_PERMANENTLY_ERROR and PERMANENT_REDIRECT_ERROR)

@RookieRick
Copy link
Contributor Author

...and after still more inspection and reflection, I'm torn.. The suggested workaround that may be applicable is to override Flask's handle_http_exception -- while I can see a way that could work (we could insert a check for 301/308 specifically before passing through to default functionality there), it feels awfully dirty to me, and perhaps something better left to our users if they indeed require it.

Ultimately, the behavior of Flask has changed from raising an exception for redirects to simply returning them as a result. I'm not sure I agree with that (especially for a point-release), but it seems to be characterized as an unintentional behavior in 1.0, so it's possible our best (or at least most "correct") course of action here is to accept it as-is, and update our test to expect text/html in the case of a redirect.

I'll tinker with it some more and see if I can come up with any better ideas than that; other suggestions most definitely welcome.

@RookieRick
Copy link
Contributor Author

Addressing with PR #97 - changed only the test as the underlying Flask behavior has changed and I don't see a clean way to address it in Rebar.

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

1 participant