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

FindAll with object as a result. #113

Closed
kenjiqq opened this issue Aug 11, 2014 · 4 comments
Closed

FindAll with object as a result. #113

kenjiqq opened this issue Aug 11, 2014 · 4 comments
Assignees

Comments

@kenjiqq
Copy link

kenjiqq commented Aug 11, 2014

When using findAll and the resource returns a single object as a result, not an array. Some APIs could work like that if you have filtered or something causing only one answer to be returned from the server instead of returning an array with one element. But this code could fail when that happens because the angular.forEach starts to iterate over the values in the object. And if a value is null the code crashes.

function processResults(utils, data, resourceName, queryHash) {
  var resource = this.store[resourceName],
    idAttribute = this.definitions[resourceName].idAttribute,
    date = new Date().getTime();

  data = data || [];

  // Query is no longer pending
  delete resource.pendingQueries[queryHash];
  resource.completedQueries[queryHash] = date;

  // Update modified timestamp of collection
  resource.collectionModified = utils.updateTimestamp(resource.collectionModified);

  // Merge the new values into the cache
  var injected = this.inject(resourceName, data);

  // Make sure each object is added to completedQueries
  angular.forEach(injected, function (item) {
    resource.completedQueries[item[idAttribute]] = date;
  });

  return injected;
}

There should probably be a check to only run the loop if injected is an array, and just run the code on the one object if it is not.

@jmdobry
Copy link
Member

jmdobry commented Aug 11, 2014

I understand what you're talking about. In my opinion, no RESTful api should return a single object instead of an array when the endpoint is collection based, no matter how much filtering has been done.

GET /users -> returns array
GET /users/:id - > returns object

By its very nature, findAll is a collection based query. It expects the response to be a collection, even if it's empty or only has one object. Adding a check to angular-data's code to get around this feels very hacky to me, and doesn't belong in angular-data's codebase.

Luckily, angular-data provides plenty of opportunity for the developer to get around it:

Let's assume GET /users returns a single object instead of a collection for some reason.
Here's a way around it:

DS.findAll('user', {...}, { cacheResponse: false}).then(function (user) {
  DS.inject('user', user);
});

Regarding null items:

// definitely should add a null check here
resource.completedQueries[item[idAttribute]] = date;

@kenjiqq
Copy link
Author

kenjiqq commented Aug 11, 2014

I can see that, it's not a very restful behavior, but as far as i can tell this loop is the only thing that really requires an array to be the result from the query. And in my opinion the loop should have a check for isArray anyways to make sure it doesn't iterate over an object, if just to throw an error or something if it gets a result it doesn't expect.
And with that its just one line of code to actually support an object as result

 // Make sure each object is added to completedQueries
if(isArray(injected) {
  angular.forEach(injected, function (item) {
    resource.completedQueries[item[idAttribute]] = date;
  });
} else {
  resource.completedQueries[injected[idAttribute]] = date;
}

or throw an error at the begining

function processResults(utils, data, resourceName, queryHash) {
  if(!isArray(data)) {
    //throw error or log error
  }
}

@jmdobry jmdobry self-assigned this Aug 11, 2014
@jmdobry jmdobry added the bug label Aug 11, 2014
@Busata
Copy link

Busata commented Oct 20, 2014

Hello, regarding top level json, consider that some frameworks refuse it due to security issues. eg. http://flask.pocoo.org/docs/0.10/security/

@jmdobry
Copy link
Member

jmdobry commented Oct 20, 2014

@Busata See our responses to your mailing list question: https://groups.google.com/forum/?fromgroups=#!topic/angular-data/i1BPs7OqAlk

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants