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

[superset-client] use getClientErrorObject for client error handling #6163

Merged
merged 4 commits into from Oct 23, 2018

Conversation

williaster
Copy link
Contributor

@williaster williaster commented Oct 22, 2018

This PR fixes #6152 and is a more generic fix than #6158 for handling fetch-based errors.

It adds a method getClientErrorObject which takes a fetch Response as input, reads it as either json or text, and returns a Promise that resolves to an object with all keys from the Response and an error key which can be used for display in the UI.

Note this is more complex than previous ajax errors because we must read the response explicitly, and because of the mixture of response types we have no idea if it's text or json.

Added unit tests and tested locally for bug reported in #6152

image

@mistercrunch @graceguo-supercat @kristw @michellethomas

@williaster williaster changed the title [superset-client] use getClientErrorObject for client error handling [WIP][superset-client] use getClientErrorObject for client error handling Oct 22, 2018
Copy link
Contributor

@kristw kristw left a comment

Choose a reason for hiding this comment

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

a few nits. overall LGTM

});
});

it('Handles Repsonse that can be parsed as json', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

typo Response

});
});

it('Handles Repsonse that can be parsed as text', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

another typo

@williaster williaster changed the title [WIP][superset-client] use getClientErrorObject for client error handling [superset-client] use getClientErrorObject for client error handling Oct 22, 2018
@williaster williaster closed this Oct 22, 2018
@williaster williaster reopened this Oct 22, 2018
@williaster
Copy link
Contributor Author

🤮 build is stuck ... will give it a bit more time to sort itself out.

@williaster
Copy link
Contributor Author

Need to merge #6133 before this one

@williaster williaster merged commit d8d50a1 into apache:master Oct 23, 2018
@williaster williaster deleted the chris--final-ajax branch October 23, 2018 05:43
@mistercrunch
Copy link
Member

@williaster I wanted to add this into 0.28.2 as this is a pretty major issue with 0.28.1 but it was conflicting pretty hard. Any idea which series of cherries I should line up to avoid merge conflict? I tried lining up #6133 first but it conflicted as hard.

If it's not easy maybe I just need to cut 0.29.0 from master.

@williaster
Copy link
Contributor Author

Hmm, it would probably need all of SupersetClient PRs because it may have been introduced in one of the earlier ones. they were

#5875 chart ajax (introduced bug?)
#5869 explorer ajax
#5854 dashboard ajax
#5896 sqllab ajax
#6134 datasource editor ajax
#6133 logger ajax
#6135 misc ajax
#6148 csrf optimization

unsure, 0.29.0 might be easier?

@mistercrunch
Copy link
Member

I think 0.29.0 is a safer bet

michellethomas pushed a commit to michellethomas/panoramix that referenced this pull request Oct 24, 2018
…pache#6163)

* [superset-client] use getClientErrorObject for client error handling

* fix getClientErrorObject json parsing

* fix getClientErrorObject test typos

* kick build

(cherry picked from commit d8d50a1)
bipinsoniguavus pushed a commit to ThalesGroup/incubator-superset that referenced this pull request Dec 26, 2018
…pache#6163)

* [superset-client] use getClientErrorObject for client error handling

* fix getClientErrorObject json parsing

* fix getClientErrorObject test typos

* kick build
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.34.0 labels Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.34.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"No data" error message not getting displayed
3 participants