Send CORS headers for 500 errors

[alison985]

#382 about CORS headers found that the problem wasn't the CORS headers but was actually that a 500 error was being returned because the dataset was too large. This ticket is to make sure that when a 500 error is sent CORS headers are as well. This will make sure the real error message isn't obscured in the future.

[jezdez]

Great idea, and most definitely something that upstream will want to have as well, right?

[alison985]

@jezdez I filed an issue upstream getredash#2594

I have a proposed fix for this ( StantonVentures@70001cc ) but I can't reproduce the issue locally, even with the same query from #382 in order to test and get confirmation. It seems that while STMO can see an events table on Mozilla Athena, my local can not for the same Mozilla Athena datasource which is putting me into a different error state for the results.json page (i.e. 404). Thoughts?

[jezdez]

@alison985 Hm, not being able to reproduce the triggering issue is unfortunate but looking at the code it seems obvious that the header addition only happens if there are results and not if there is an error to create the results. I believe this is a potential race condition or at least an inconsistency with how the API responds. A 500 error code is still a valid response, one without the headers isn't though, not what user are expecting IMO.

Long story short, let's send your change to the upstream PR and see where it gets us with review.

[arikfr]

When a 500 error returns due to large result set, it's usually because the handling process died. So I doubt that setting these headers in the application code will help.

[alison985]

@jezdez Given Arik's comment, do I still submit for review upstream?

[jezdez]

@arikfr @alison985 It's a tricky one, I think we need to consider any response not following the same CORS rules as successful responses to be a leaky situation. E.g. 3rd party sites like Observable will stumble over the CORS issue and not see the actual cause of the error to be displayed to their users.

As discussed in #382 a user of our Redash instance wasn't able to see that their resultset was so large it caused the 500 and assumed that the recent Redash update dropped CORS headers, effectively making this a red herring.

That said, I'm ready to let this go, if you think it's a non-issue since we definitively would get feedback from users if it occurs again.

[arikfr]

@jezdez I agree that the current situation is not the desired behavior. What I wanted to point out is that applying the CORS headers earlier in the code won't change this, because the 500 error probably comes from an upstream proxy after the Redash process dies.

To properly handle this, we need to address the underlying problem -- support for large query results.

[jezdez]

@arikfr Ah, understood, that makes sense to me. Let's close this for now since the work on improved query payloads is on track anyway.