Opened 4 years ago

Closed 4 years ago

#18769 closed defect (fixed)

request handleAs: 'json' try to parse non 2XX responses

Reported by: anders Owned by: dylan
Priority: high Milestone: 1.11
Component: IO Version: 1.10.4
Keywords: Cc:
Blocked By: Blocking:

Description

If the server does not respond with JSON formated content for response codes >= 300, this is not desirable.

This could be solved by checking the status before trying to parse.

In dojo-release-1.10.4 dojo/request/xhr.js line 67 - 83

if(!error){
  try{
    handlers(response);
  }catch(e){
    error = e;
  }
}

if(error){
  this.reject(error);
}else if(util.checkStatus(_xhr.status)){
  this.resolve(response);
}else{
  error = new RequestError('Unable to load ' + response.url + ' status: ' + _xhr.status, response);

  this.reject(error);
}

Could be changed to:








if(error){
  this.reject(error);
}else if(util.checkStatus(_xhr.status)){
  try{
    handlers(response);
    this.resolve(response);
  }catch(e){
    error = e;
    this.reject(error);
  }
}else{
  error = new RequestError('Unable to load ' + response.url + ' status: ' + _xhr.status, response);

  this.reject(error);
}

Change History (5)

comment:1 Changed 4 years ago by dylan

Component: CoreIO
Owner: set to dylan

I believe this is a duplicate of #16223 which I was in the process of reviewing recently.

The original decision was that a server could elect to wrap 300 and 400 errors in JSON also (which is what you're expected to do with handleAs: 'json'

I agree that's probably not what most people want, but is of course possible and is how this was originally specified.

The current workaround is to not set handleAs to json, and instead call json.parse on the response.

comment:2 Changed 4 years ago by anders

Thanks, Ok I understand. Then I only suggest an improved error message for the combined case with non 2XXX code and a parser error of the content. Perhaps something like this:

var handlerError
if(error){
  this.reject(error);
}else {
  try{
    handlers(response);
  }catch(e){
    handlerError = e;
  }
  if(util.checkStatus(_xhr.status) {
    if(!handlerError) {
      this.resolve(response);
    } else {
      this.reject(handlerError);
    }
  }else {
    if(!handlerError) {
      error = new RequestError('Unable to load ' + response.url + ' status: ' + _xhr.status, response);
      this.reject(error);
    } else {
      error = new RequestError('Unable to load ' + response.url + ' status: ' + _xhr.status
              + ' and error in handleAs: transformation of response', response);
      this.reject(error);
    }
  }
}


comment:3 Changed 4 years ago by dylan

Milestone: tbd1.11
Status: newassigned

comment:4 Changed 4 years ago by dylan

Priority: undecidedhigh

comment:5 Changed 4 years ago by dylan

Resolution: fixed
Status: assignedclosed

Hi @anders, thanks, I've landed your suggestion in https://github.com/dojo/dojo/commit/15175cb3d5afc19892304e6ee1cd196d008ce234 (with some minor rework).

If you don't already have a CLA on file, please fill one out at http://dojofoundation.org/about/claForm .

Note: See TracTickets for help on using tickets.