strongloop / loopback-sdk-angular

Service for auto-generating Angular $resource services for LoopBack
Other
155 stars 82 forks source link

create() with array of model instances #122

Closed GrandmasterGrogu closed 9 years ago

GrandmasterGrogu commented 9 years ago

This may be an Angular configuration issue or a minor Loopback Angular service issue with what it returns, not sure.

When sending multiple data objects to create(), it returns an array and this error:

Error: [$resource:badcfg] Error in resource configuration. Expected response to contain an object but got an array http://errors.angularjs.org/1.2.28/$resource/badcfg?p0=object&p1=array minErr/<@http://localhost:9000/bower_components/angular/angular.js:78:12 resourceFactory/</Resource[name]/promise<@http://localhost:9000/bower_components/angular-resource/angular-resource.js:546:1 qFactory/defer/deferred.promise.then/wrappedCallback@http://localhost:9000/bower_components/angular/angular.js:11682:31 qFactory/ref/<.then/<@http://localhost:9000/bower_components/angular/angular.js:11768:26 $RootScopeProvider/this.$get</Scope.prototype.$eval@http://localhost:9000/bower_components/angular/angular.js:12811:16 $RootScopeProvider/this.$get</Scope.prototype.$digest@http://localhost:9000/bower_components/angular/angular.js:12623:15 $RootScopeProvider/this.$get</Scope.prototype.$apply@http://localhost:9000/bower_components/angular/angular.js:12915:13 done@http://localhost:9000/bower_components/angular/angular.js:8450:34 completeRequest@http://localhost:9000/bower_components/angular/angular.js:8664:7 createHttpBackend/</xhr.onreadystatechange@http://localhost:9000/bower_components/angular/angular.js:8603:1
consoleLog/<()angular.js (line 10126)
$ExceptionHandlerProvider/this.$get</<(exception=
Error: [$resource:badcfg] Error in resource configuration. Expected response to contain an object but got an array http://errors.angularjs.org/1.2.28/$resource/badcfg?p0=object&p1=array

return new Error(message);

, cause=undefined)angular.js (line 7398)
qFactory/defer/deferred.promise.then/wrappedCallback()angular.js (line 11685)
qFactory/ref/<.then/<()angular.js (line 11768)
$RootScopeProvider/this.$get</Scope.prototype.$eval(expr=function(), locals=undefined)angular.js (line 12811)
$RootScopeProvider/this.$get</Scope.prototype.$digest()angular.js (line 12623)
$RootScopeProvider/this.$get</Scope.prototype.$apply(expr=undefined)angular.js (line 12915)
done(status=200, response="[{"testpoolid":8,"testid...estid":8,"priority":2}]", headersString="Content-Type: application/json; charset=utf-8\r\n", statusText="OK")angular.js (line 8450)
completeRequest(callback=done(status, response, headersString, statusText), status=200, response="[{"testpoolid":8,"testid...estid":8,"priority":2}]", headersString="Content-Type: application/json; charset=utf-8\r\n", statusText="OK")angular.js (line 8664)
createHttpBackend/</xhr.onreadystatechange()angular.js (line 8603)

return logFn.apply(console, args);

I read somewhere that it may be expecting an object or some sort of $resource configuration.

bajtos commented 9 years ago

When sending multiple data objects to create(), it returns an array and this error.

This is sort of expected, the create method generated by the SDK does not support creating multiple instances via an array.

A possible solution is to extend the generator to produce two methods:

They will share most of the configuration except the isArray flag.

Would you mind contributing this feature yourself?

GrandmasterGrogu commented 9 years ago

Sure, I'll give it a shot. I'll let you know if I run into any issues.

jonathan-casarrubias commented 9 years ago

what is the best way to inherit a method?

In the file services.js line 61 I'm adding:

    c.methods.forEach(function fixArgsOfPrototypeMethods(method) {
      if(method.name=="create"){ // Hook for create methods so we add a createMany
        var createMany = method;   //tried a clone or deep copy but wont work
            createMany.name = "createMany";
            createMany.createMany = true;
            c.methods.splice(key+1, 0, createMany);
      }
      var ctor = method.restClass.ctor;
      if (!ctor || method.sharedMethod.isStatic) return;
      method.accepts = ctor.accepts.concat(method.accepts);
    });

so in the file services.template.ejs line 61 also:

 if (action.isReturningArray() || action.createMany) { // isreturningArray always return false on create methods

So until this point I'm able to see on lb-services the createMany for each model with the isArray: true in it, obviously the problem is that I have 2 times the createMany and the create disappeared because I'm overriding the create method with the createMany due the reference.

I tried cloning the "method" object but it breaks the flow, and since I would like to pull request this feature, I would like to know what is the best way to extend the create method.

Thanks in advance for your help

bajtos commented 9 years ago

what is the best way to inherit a method?

I would use prototypal inheritance via Object.create, see e.g. https://github.com/strongloop/loopback-sdk-angular/blob/fa7668b8e11e04fad8d239558846804e5d6315f8/lib/services.js#L155-L159

I am not very keen on testing action.createMany to decide whether the action returns an array. A cleaner solution is to override isReturningArray() in the child object.

// a mockup
var createMany = Object.create(action);
action.name = "createMany";
action.isReturningArray = function() { return true; };
jonathan-casarrubias commented 9 years ago

Yes that is actually cleaner, I will implement this and if works I will pull request. thanks a lot

GrandmasterGrogu commented 9 years ago

Looks like you guys got it underway, I got sidetracked with some other parts of my project.

I haven't tested it completely yet, but this Angular-side approach seems to work too (though I would assume is less efficient): An asynchronous for loop

 var requests = [];
angular.forEach($scope.arrayOfObjects, function (value, key) {
var deferred = $q.defer();
requests.push(deferred);
YourLoopBackModel.create(JSON-style Object}, deferred.resolve, deferred.reject); 
                                      });

$q.all(requests).then(function () {
                                          //TODO handle results                                         
                                          });
jonathan-casarrubias commented 9 years ago

Right, that should probably work... the issue with that is that you make a bunch of requests that may flood your backend, of course depending the traffic of your system you could drastically decrease the performance.

Adding the following lines of code will build createMany methods for you and you will be able to store multiple instances in just 1 request

File: loopback-sdk-angular/lib/services.js

61 c.methods.forEach(function fixArgsOfPrototypeMethods(method, key) { //Below this line add the following
62      if(method.name=="create"){ 
63           var createMany = Object.create(method);
64               createMany.name = "createMany";
65               createMany.isReturningArray = function() { return true; };
66               c.methods.splice(key+1, 0, createMany);
67      }

// enable
YourModel.createMany();

and

168  reverseModel.methods.push(reverseMethod); // Below this line add the following
169  if(reverseMethod.name.match(/create/)){
170   var createMany = Object.create(reverseMethod);
171        createMany.name = createMany.name.replace(/create/, 'createMany');
172        createMany.internal = createMany.internal.replace(/create/, 'createMany');
174        createMany.isReturningArray = function() { return true; };
175        reverseModel.methods.push(createMany);
176  }

//enable
YourModel.relation.createMany()

Examples:

Account.createMany();
Post.comments.createMany();

cheers jon

pulkitsinghal commented 9 years ago

@jonathan-casarrubias - I started using .createMany([]) and I noticed something very odd: my loopback server seems to process the pages (of 500 objects each) that I send to be saved via createMany, slower than if i do so via a nodejs-remote-client invocation of .create([])

Now I don't have super cool tests with timing metrics to prove this "feeling" but I thought I'd just at least confirm:

  1. if the implementation has any known inherent slowdowns in it (like its server side counterpart might nor use .create([])?)
  2. or if you too are noticing unpredicted slowdown on server-side when using createMany?
jonathan-casarrubias commented 9 years ago

Hi @pulkitsinghal the createMany method for Angular is just a configuration that allows you to receive an array of results after you persists your instances.

For example, if you try to use Model.create() in Angular and send an array, these instances will persist since the limitation is not from the API endpoint '/api/Model/create', the problem comes when the backend response is an array of results, the angular resource will say something like "Expected response to contain an object but got an array", though your instances should be successfully created.

In other words. the methods create and createMany from angular, use exactly the same endpoint from your api, thus createMany does not modify anything in the backend, is just a configuration for angular within the generated lb-services to allow you receive an array from the results on that endpoint.

The createMany should not add any overhead to the request, but what I ignore is if there is some differences in the back end between the Rest API Endpoint and using the Node API that makes you feel the endpoint is slower than using the node api mthod .create([]).

But definitely is not anything related with the angular-sdk you may need to add a ticket in the loopback repo.

Hope this info helps you find your path. Cheers! Jon

pulkitsinghal commented 9 years ago

@jonathan-casarrubias Yeah I thought that you would be using just the core .create([]) so that can't be it ... thank you for confirming ... I'll continue looking elsewhere for my performance dilemma.

jonathan-casarrubias commented 9 years ago

I took the time to see the code you referenced here, and I see you are persisting data in chunks -I believe of 500 rows?- that definitely will add overhead, it won't be the same if you upload the full data and then store in 1 process, than sending multiple requests which are multiple processes

Have you already tried to store all the instances in just 1 request? did the loopback server got stucked?

I have not tried to add that many instances at the same time, for my project requirements we are talking less than 100 by request and works just fine.. but you seems to have different requirements.

I would try to persist everything in 1 request or just leave it to the backend, but my logic says that should be really the same, since you still will have upload time overhead etc..

pulkitsinghal commented 9 years ago

@jonathan-casarrubias - can we chat privately on https://gitter.im/ just a bit more so I can share some metrics?

bajtos commented 9 years ago

I am closing this issue as the feature was landed by #138. If you manage to create a test case reproducing the problem described in the comments above then please open a new issue.