Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

DS.RESTAdapter can findMany in size-limited batches #531

Closed
wants to merge 2 commits into from

9 participants

@seancribbs

Especially in the case where hasMany associations have high-cardinality, sometimes the Ajax request to fetch many records at once will generate a URL that is longer than supported by the browser.

This pull-request adds a 'fetchBatchSize' property to DS.RESTAdapter that allows large collections to be fetched in fixed-size chunks. If left unset, the default behavior is essentially unchanged.

@wagenet
Owner

@seancribbs This seems pretty useful. Can you get it merging cleanly again?

Sean Cribbs added some commits
Sean Cribbs Support fetching records in batches in DS.RESTAdapter.
This helps with high cardinality hasMany associations where the
generated URL may be longer than the browser supports.
e5ca276
Sean Cribbs Accumulate Ajax requests across the batched findMany and verify all o…
…f them.
256d159
@seancribbs

@wagenet Rebased, cleaned-up and force-pushed.

@cmeiklejohn

@wagenet thoughts on the rebased edition?

@sly7-7
Collaborator

@seancribbs definitely usefull... I just encountered this type of problem... we were simulating the potential objects a user will manipulate. about 1000 items in one project... when entering /projects/42/items.... boom...
Perhaps there is an other workaround, but your PR seems fine. @wycats @tomdale @tchak any thoughts ?

Related to the old #241

@darthdeus
Collaborator

ping

@Goltergaul

why do you not load the associated objects by their foreign key? this way you would have to pass only one id

@bobbus

@Goltergaul , I thought about this but there is no easy way to do it for now : when we access an hasMany relationship which was loaded by an array of ids, ember data automatically fire the requests to load all of them.

@tomdale
Owner

I'm going to opt to close this for now. We would like to encapsulate sparse array semantics than at a higher-level than the REST adapter.

@tomdale tomdale closed this
@tomdale
Owner

We should continue the discussion on pagination in general on the JSON API spec.

@seancribbs

@tomdale Maybe I misunderstand, but isn't this a transport-layer concern? The layers above should not care about how the requests are made to the backend, just that they are requesting an entire collection of records. The fact that browsers have a (sensible) limit on URL length seems to be something that is only the concern of the adapter.

@tchak

@tomdale I kinda agree with @seancribbs. It seems this and pagination are different concerns.

@seancribbs

FWIW I'm not arguing for this particular pull-request, I realize it's out of date. I just think "sparse array semantics at a higher level than the REST adapter" fails to cover the problem it solves, unless you have something else in mind entirely.

@Goltergaul

@tomdale what do you mean by "sparse array semantics than at a higher-level than the REST adapter"?

@wagenet
Owner

#651 has been left open. Any further discussion should probably be there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jan 10, 2013
  1. Support fetching records in batches in DS.RESTAdapter.

    Sean Cribbs authored
    This helps with high cardinality hasMany associations where the
    generated URL may be longer than the browser supports.
This page is out of date. Refresh to see the latest.
View
34 packages/ember-data/lib/adapters/rest_adapter.js
@@ -3,7 +3,7 @@ require('ember-data/system/adapter');
require('ember-data/serializers/rest_serializer');
/*global jQuery*/
-var get = Ember.get, set = Ember.set, merge = Ember.merge;
+var get = Ember.get, set = Ember.set, merge = Ember.merge, getWithDefault = Ember.getWithDefault;
/**
The REST adapter allows your store to communicate with an HTTP server by
@@ -63,6 +63,7 @@ var get = Ember.get, set = Ember.set, merge = Ember.merge;
DS.RESTAdapter = DS.Adapter.extend({
bulkCommit: false,
since: 'since',
+ fetchBatchSize: undefined,
serializer: DS.RESTSerializer,
@@ -266,17 +267,26 @@ DS.RESTAdapter = DS.Adapter.extend({
},
findMany: function(store, type, ids) {
- var root = this.rootForType(type);
- ids = this.serializeIds(ids);
-
- this.ajax(this.buildURL(root), "GET", {
- data: {ids: ids},
- success: function(json) {
- Ember.run(this, function(){
- this.didFindMany(store, type, json);
- });
- }
- });
+ var root = this.rootForType(type),
+ batchSize = getWithDefault(this, 'fetchBatchSize', ids.length),
+ batch, rest = ids, success;
+
+ success = function(json) {
+ Ember.run(this, function(){
+ this.didFindMany(store, type, json);
+ });
+ };
+
+ while(rest.length > 0) {
+ batch = rest.slice(0, batchSize);
+ rest = rest.slice(batchSize);
+ ids = this.serializeIds(batch);
+
+ this.ajax(this.buildURL(root), "GET", {
+ data: {ids: ids},
+ success: success
+ });
+ }
},
/**
View
104 packages/ember-data/tests/unit/rest_adapter_test.js
@@ -20,9 +20,9 @@ module("the REST adapter", {
ajax: function(url, type, hash) {
var success = hash.success, self = this;
- ajaxUrl = url;
- ajaxType = type;
- ajaxHash = hash;
+ ajaxUrl = setOrPush(ajaxUrl, url);
+ ajaxType = setOrPush(ajaxType, type);
+ ajaxHash = setOrPush(ajaxHash, hash);
if (success) {
hash.success = function(json) {
@@ -83,16 +83,61 @@ module("the REST adapter", {
}
});
+var setOrPush = function(current, next){
+ if (Ember.isArray(current)){
+ current.push(next);
+ return current;
+ } else {
+ return next;
+ }
+};
+
var expectUrl = function(url, desc) {
- equal(ajaxUrl, url, "the URL is " + desc);
+ var actualUrl;
+ if (Ember.isArray(ajaxUrl)) {
+ actualUrl = ajaxUrl[0];
+ } else {
+ actualUrl = ajaxUrl;
+ }
+ equal(actualUrl, url, "the URL is " + desc);
};
var expectType = function(type) {
- equal(type, ajaxType, "the HTTP method is " + type);
+ var actualType;
+ if (Ember.isArray(ajaxType)) {
+ actualType = ajaxType[0];
+ } else {
+ actualType = ajaxType;
+ }
+ equal(type, actualType, "the HTTP method is " + type);
};
var expectData = function(hash) {
- deepEqual(hash, ajaxHash.data, "the hash was passed along");
+ var actualHash;
+ if (Ember.isArray(ajaxHash)) {
+ actualHash = ajaxHash[0];
+ } else {
+ actualHash = ajaxHash;
+ }
+ deepEqual(hash, actualHash.data, "the hash was passed along");
+};
+
+var accumulateAjaxRequests = function(){
+ ajaxUrl = [];
+ ajaxType = [];
+ ajaxHash = [];
+};
+
+var nextRequest = function(){
+ if (Ember.isArray(ajaxUrl)){
+ ajaxUrl.shift();
+ }
+ if (Ember.isArray(ajaxType)){
+ ajaxType.shift();
+ }
+ if (Ember.isArray(ajaxHash)){
+ ajaxHash.shift();
+ }
};
var expectState = function(state, value, p) {
@@ -574,6 +619,53 @@ test("additional data can be sideloaded in a GET with many IDs", function() {
});
});
+test("the number of records fetched by findMany can be limited by fetchBatchSize", function(){
+ equal(ajaxUrl, undefined, "no Ajax calls have been made yet");
+
+ set(adapter, 'fetchBatchSize', 2);
+
+ accumulateAjaxRequests();
+
+ var people = store.findMany(Person, [ 1, 2, 3 ]);
+
+ equal(2, ajaxUrl.length, "two Ajax requests have been made");
+
+ expectUrl("/people");
+ expectType("GET");
+ expectData({ ids: [ 1, 2 ] });
+
+ ajaxHash[0].success({
+ people: [
+ { id: 1, name: "Rein Heinrichs" },
+ { id: 2, name: "Tom Dale" }
+ ]
+ });
+
+ nextRequest();
+
+ expectUrl("/people");
+ expectType("GET");
+ expectData({ ids: [ 3 ] });
+
+ ajaxHash[0].success({
+ people: [
+ { id: 3, name: "Yehuda Katz" }
+ ]
+ });
+
+ var rein = people.objectAt(0);
+ equal(get(rein, 'name'), "Rein Heinrichs");
+ equal(get(rein, 'id'), 1);
+
+ var tom = people.objectAt(1);
+ equal(get(tom, 'name'), "Tom Dale");
+ equal(get(tom, 'id'), 2);
+
+ var yehuda = people.objectAt(2);
+ equal(get(yehuda, 'name'), "Yehuda Katz");
+ equal(get(yehuda, 'id'), 3);
+});
+
test("finding people by a query", function() {
var people = store.find(Person, { page: 1 });
Something went wrong with that request. Please try again.