Improve error handling for poorly formed relationships

[carlbennettnz]

Tells the user what they did wrong when they forget to wrap their relationship changes in a { data: ... } object

[ethanresnick]

This basically looks good, but I need to think about it a bit too, so I'll take a closer look within the next couple days and let you know.

[carlbennettnz]

Sure, no rush

[ethanresnick]

Overall, this PR looks great. I'd make one small tweak, though, namely: I'd throw a more generic error from relationshipObjectFromJSON and then augment and rethrow that error in resourceFromJSON. Like so:

function relationshipObjectFromJSON(json) {
  if (typeof json.data === "undefined") {
    throw new APIError(400, undefined, "Missing relationship linkage.");
  }

  //  blah blah
}

// then...
function resourceObjectFromJSON(json) {
  // blah blah

  // build RelationshipObjects
  let key;
  try {
    for(key in relationships) {
      relationships[key] = relationshipObjectFromJSON(relationships[key]);
    }
  }
  catch(e) {
    e.details =  `No data key was found for the '${key}' relationship.`;
    throw e;
  }

  // blah blah
}

I prefer this because adding the relationship argument to relationshipObjectFromJSON just for use in an error message bothered me a bit. It would suggest that the creation of the relationship object is dependent on knowing the relationship's name, when that's not really true or is only true in a trivial sense. Also, this has the side benefit of making the APIError's title into something constant (the "Missing Relationship Linkage" message) while making details vary per instance of the problem (the "No data key was found..." message), which is how title and details are specced to work.

[carlbennettnz]

👍

Updated.

[ethanresnick]

Perfect. Can you add an integration test too? Then I'll give this a merge.

[carlbennettnz]

Sure, although I have tried writing integration tests for this library a few times now but something about the way it's set up means I keep getting silent errors or a failures of the first test without any pointers to what I'm doing wrong.

The main thing that you're doing that's different from what I'm used to is structuring your tests like this

describe('' => {
  makeRequest().then((res) => {
    describe('Stuff', () => {
      it('should do something', () => {
        expect(true).to.be.ok;
      });
    });
  });
});

not like this

describe('Stuff', () => {
  var res;

  before((done) => {
    makeRequest('/users', (err, result) => {
      res = result;
      done(err);
    });
  });

  it('should do something', () => {
    expect(res).to.be.ok;
  });
});

The main problem with the first method is that if makeRequest() rejects then the tests aren't run. The test suite exits saying slightly fewer than normal tests have passed, but none have failed.

I might be misunderstanding something here, but this was was the main problem I was having when trying to write tests for #70. Are you able to explain some of the reasoning for that structure?

[ethanresnick]

Are you able to explain some of the reasoning for that structure?

Short answer: the current structure is a mistake (for the reasons you cite) and I've already started to phase it out. Take a look at the http compliance tests, which use before and call .done on Agent.request promises to make sure failures aren't silent.

Feel free to write the new tests modeled after the http compliance ones, or, if that feels too inconsistent in a given file, to use the current structure but add .done()/.catch() calls on the promises to make sure errors get reported. Here's how that second approach would work (amending your first code sample):

describe((done) => {
  makeRequest().then((res) => {
    describe('Stuff', () => {
      it('should do something', () => {
        expect(true).to.be.ok;
      });
    });
  }).catch(done);
});

[ethanresnick]

Merged! Sorry this has been sitting for a few days. Github doesn't notify me when the commits on a PR change. So, in the future, if you lease a comment after you've updated things, I'lll see it faster!

[carlbennettnz]

Oh ok, sorry, didn't realise. Will do :)