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

[BUGFIX] fix jsonapi error handling when not using jquery #6941

Merged
merged 2 commits into from
Jan 30, 2020
Merged

[BUGFIX] fix jsonapi error handling when not using jquery #6941

merged 2 commits into from
Jan 30, 2020

Conversation

aclarembeau
Copy link
Contributor

Fixes #6936

Copy link
Contributor

@snewcomer snewcomer 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 comments. Also, linting errors are outputted inline. But otherwise will be looking forward to this fix! Thank you!!

packages/adapter/addon/rest.js Outdated Show resolved Hide resolved
packages/adapter/addon/rest.js Show resolved Hide resolved
@aclarembeau
Copy link
Contributor Author

@snewcomer , I've updated my code with:

  • better respect of linting rules
  • updated logic (not parsing JSON payload if payload is not JSON, but an error object)

@ghost
Copy link

ghost commented Jan 10, 2020

The asset size checks are failing by 28 bytes; not sure if that amount should hold up this PR.
@runspired thoughts?

@runspired
Copy link
Contributor

Asset size change is totally fine. I agree there is a bug and that the solution is to JSON.parse something.

What I am not convinced of is that this is the right place in the process to do this. To be convinced I need to spend a little time (or Igor needs to spend a little time) walking through the fetch flow and confirming where the point of divergence in behavior occurs so that we are confident we're fixing it at the source and not much later in the flow.

if (responseData.status === 200 && payload instanceof Error) {
responseData.errorThrown = payload;
payload = responseData.errorThrown.payload;
} else {
responseData.errorThrown = errorThrown;
payload = adapter.parseErrorResponse(payload);
Copy link
Contributor

Choose a reason for hiding this comment

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

A few comments after reading through the existing implementation:

I believe the necessary change is either this OR

  if (!response.ok) {
    return payload;
  }

^ this line in determineBodyPromise should possibly attempt JSON.parse. I lean heavily towards the former (current PR) solution

The primary difference is in whether we give the adapter the chance to do the parsing. Note the Ajax codepath calls this for all Ajax errors (I'm not sure if a 404 triggers an error though, as odd as that sounds at first glance, it doesn't trigger one in fetch for instance).

But if it does...

let payload = adapter.parseErrorResponse(jqXHR.responseText);

^ this is what would be doing the similar parsing for the Ajax branch.

cc @igorT

Copy link
Contributor

@runspired runspired left a comment

Choose a reason for hiding this comment

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

I'm approving this for the solution (see comment though for potential alternate solution). I have not reviewed the test changes. cc @igorT

Copy link
Contributor

@runspired runspired left a comment

Choose a reason for hiding this comment

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

Reviewed tests.

@runspired runspired merged commit d6bd5f1 into emberjs:master Jan 30, 2020
@runspired runspired added 🏷️ bug This PR primarily fixes a reported issue 🎯 beta PR should be backported to beta 🎯 release PR should be backported to release labels Jan 30, 2020
HeroicEric pushed a commit to HeroicEric/data that referenced this pull request Feb 27, 2020
@HeroicEric HeroicEric mentioned this pull request Feb 27, 2020
HeroicEric pushed a commit to HeroicEric/data that referenced this pull request Feb 27, 2020
@HeroicEric HeroicEric mentioned this pull request Feb 27, 2020
igorT pushed a commit that referenced this pull request Feb 28, 2020
igorT pushed a commit that referenced this pull request Feb 28, 2020
@HeroicEric HeroicEric removed the 🎯 release PR should be backported to release label Feb 28, 2020
@igorT igorT removed the 🎯 beta PR should be backported to beta label Jul 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bug This PR primarily fixes a reported issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adapter error while parsing JSONApi errors
5 participants