Queries and Filtering

[kenjiqq]

Hi.
There seems to be a requirement where queries either for find async methods or filtering for the filter sync methods require that you put params into a query object. Is this done because there are plans to add other possible params as well? If not it seems kind unnecessary, and it sets a few limitation, like many apis do not take a query param they just want the search params as normal url parameters.

Rolling your own queryTransform only allows you to modify the contents of the query object, not the query part it self so you can't remove it there. I guess the only option is to roll your own adapter, but that seems kind unnecessary when you can change most of the other behavior by just overriding the transform and filter methods.

[kentcdodds]

I didn't realize you had to use the term "query" on a findAll. Can you not just use whatever you want and angular-data will just send the request with the params you've specified? For the sync stuff it makes sense that angular-data would enforce a term, but we're not reliant on an api for that so I see no problem there. Is there a reason you could not simply do this:

DS.findAll('user', {
  search: 'Charles'
});

Actually, I've been wondering why there's documentation for the query property of that params object. I wouldn't expect angular-data to really do anything with those params but tack it onto the request... If it does need to be query then I'd like to know why because my backend guy wants me to change it...

[jmdobry]

This is the first time anyone has questioned the query/filtering part of angular-data, so I'm glad for the opportunity to discuss and improve it.

Ultimately my goal with findAll and filter is to have an asynchronous method that returns a subset of data from some data source and a synchronous method capable of pulling that same subset straight out of what has already been injected into the data store. Ideally, I should be able to pass the same criteria (where, limit, skip, etc.) to both findAll and filter and they should return the same subset of data. For example, I should be able to do this:

// Whether the params format should change is up for discussion
var params = {
  query: {
    where: {
      age: {
          '>': 30
      }
    }
  }
};

DS.findAll('user', params).then(function (users) {
  assert.deepEqual(users, DS.filter('user', params));
});

findAll covers three cases:

1. The query has never been made before

Delegate to an adapter and make the query

1. The query has been made and is still pending

Return the pending promise

1. The query has been made and has completed

Return (for example) DS.filter('user', params, options);

Case 3 is tricky one. If the query has been made and completed, then the subset returned by the server has already been injected into data store. How do I immediately pull that same subset (now cached) out of the data store? If filter takes the same criteria format as findAll then I can just pass then I can just call filter and pass it the same params argument that was passed into findAll.

If there is a better option for handling case 3, let's discuss it. The current solution facilitates the following:

app.controller('MyCtrl', function ($scope, DS) {
  var params = {
    query: {
      where: {
        age: {
            '>': 30
        }
      }
    }
  };

  DS.findAll('user', params);

  // more verbose (and more control)
  $scope.$watch(function () {
    return DS.lastModified('user');
  }, function () {
    $scope.myUsers = DS.filter('user', params);
  });

  // less verbose
  DS.bindAll($scope, 'myUsers', 'user', params);
});

Initially $scope.myUsers will be [ ], but once the findAll returns with data and the users are injected into the data store, the $watch will be triggered and the filter will pull the subset (now cached) out of the data store. Whenever the cached "users" collection changes, the filter will be triggered again, ensuring that $scope.myUsers always accurately represents the subset described by params. Of course, angular-data has now way of knowing when the subset (descibed by params as it would be returned by the server) changes, that's why this works best when paired with a messaging service like Pusher, GoInstant, custom thing, etc.

Technically, you can put anything you want into the params argument passed into findAll. If you want case 3 described above to work, then the params should be something that filter understands. If you have no control over the how the server handles the params sent by findAll, then angular-data provides the implementable queryTransform function. In addition, you can override the function that filter uses internally to figure out how to pull the correct subset out of the data store. Again, the format of params that DS.filter understands is up for change/discussion. And, I just realized there's no way to change the skip, limit orderBy fields that DS.filter expects. There is definitely room for improvement in this area.

Thoughts?

[kenjiqq]

The main reason i brought was that i was looking for a way to remove the query part of the url that is generated, so i started looking at the code. My api can't handle url?query={search: 'Charles'} i needs the format url?search='Charles'&limit=... and there did not seem to be any good way to do this. Even with custom queryTransform and custom filter, because these both handles the contents of the query object

var params = {
  query: {
  //passed to queryTransform
  {
    where: {
      //passed to custom filter
    }
  }
};

These should both probably be passed the same arguments, so that a custom implementation for the specific api can be made where both the server and filter uses the same logic and returns the same subset of elements.

And while you can do what @kentcdodds suggest to get the correct params sent to the server

DS.findAll('user', {
  search: 'Charles'
}); 

this will not work with filter, since it will never be called when there are no query object

[kenjiqq]

Also while we are on the topic. the documentation for bindAll references criteria as a possible query value:

query: {
  criteria: {
    ownerId: 5
  }
}

But i can't find any references to this in the code so is the documentation wrong? And if not it seems strange to have value that is supported just by bindAll when the rest of the methods uses where

[jmdobry]

@kenborge Yes, "criteria" is wrong. I meant to put "where".

I'm still thinking about the params format issue.

[kenjiqq]

For now i am doing something like this to work around the problem

.run(function (DSHttpAdapter, DSUtils) {
    DSHttpAdapter.GET = function GET(url, config) {
        config = config || {};
        if (config.params && config.params.query) {
            var tmp = config.params.query;
            delete config.params.query;
            DSUtils.deepMixIn(config.params, tmp);
        }
        return DSHttpAdapter.HTTP(DSUtils.deepMixIn(config, {
            url: url,
            method: 'GET'
        }));
    };
})

[jmdobry]

Perhaps flattening the params object a little by removing the need for nesting stuff under query field is a good way to proceed?

[kenjiqq]

It does seem a bit redundant to me, unless there are plans to add other stuff then query, but i do not know what that would be, since orderBy and limit also are under query now.

[jmdobry]

I think my original reason for adding it was to isolate the stuff angular-data needs from whatever else you might put in the params.

[kenjiqq]

I figured it was something like that. But does it not make sense that you would want access to whatever else you add to the params in the filter and queryTransformmethods as well?

I think another point could be that if you don't use other params added when checking if the request has been done before or not(if it should be fetched from cache), then you can't tell if it's the same request since other params outside of query could have been changed.

[jmdobry]

Yes, I'm realizing that now.

[jmdobry]

@kenborge @kentcdodds What do you think?

https://github.com/jmdobry/angular-data/blob/0.10.0/TRANSITION.md

I'm thinking I might want to make DS.filter treat top-level fields on the params argument as "where" statements (this could be optional), for example:

DS.filter('post', { author: 'John' });

Would be interpreted by angular-data as:

DS.filter('post' {
  where: {
    author: {
      '==': 'John'
    }
  }
});

skip, limit, offset, sort and orderBy would be reserved terms from angular-data's perspective.

Of course, if you wanted other types of "where" statements you'd have to use the more verbose syntax:

DS.filter('post' {
  where: {
    status: {
      'in': ['flagged', 'deleted']
    },
    upVotes: {
        '>': 100
    }
  }
});

[jmdobry]

The important changes are:

https://github.com/jmdobry/angular-data/blob/0.10.0/src/datastore/index.js#L11
https://github.com/jmdobry/angular-data/blob/0.10.0/src/datastore/sync_methods/filter.js#L70
https://github.com/jmdobry/angular-data/blob/0.10.0/src/adapters/http.js#L31
https://github.com/jmdobry/angular-data/blob/0.10.0/src/adapters/http.js#L372
https://github.com/jmdobry/angular-data/blob/0.10.0/src/adapters/http.js#L372
https://github.com/jmdobry/angular-data/blob/0.10.0/src/adapters/http.js#L372

[kenjiqq]

Looks pretty good. And i like what you are saying about offering a "shortcut" to adding params without the where clause. Would angular-data then add the where before sending the params to filter and queryTransform? Or just pass them as defined?

[jmdobry]

The only thing that would change the params object from how it's passed into findAll would be your queryTransform implementation.

[kentcdodds]

👍

[aikoven]

There is a problem with offset:
Suppose that resource has no injected items, then by calling

DS.findAll('resource', {offset: 10, limit: 10})

we will get requested items injected to data store and our query cached. But if we call the same code again, the DS.filter function kicks in and will offset these 10 items in data store, thus returning empty array.

[jmdobry]

Dang, I never ran into this issue because I always requested page 1 before requesting page 2. While you can get around this by passing the bypassCache: true option to findAll, I don't think you should have to do that. It should "just work". I'm not sure how to fix it though. I'm going to create another issue for this.