From 2f9a3273b83a7472ba13aed7bbc54356bb080c1b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miroslav=20Bajto=C5=A1?= Date: Wed, 2 Apr 2014 09:55:49 +0200 Subject: [PATCH 1/6] Template refactoring: extract ngdocForMethod Extract the template of ngdoc comment for a resource method into a function. No changes of functionality. --- lib/services.template | 193 ++++++++++++++++++++++-------------------- 1 file changed, 99 insertions(+), 94 deletions(-) diff --git a/lib/services.template b/lib/services.template index 9fc1ec9..4b9ac3c 100644 --- a/lib/services.template +++ b/lib/services.template @@ -54,100 +54,8 @@ module.factory( <% meta.methods.forEach(function(action) { var methodName = action.name.split('.').join('$'); -%> - /** - * @ngdoc method - * @name lbServices.<%- modelName %>#<%- methodName %> - * @methodOf lbServices.<%- modelName %> - * - * @description - * -<% if (!action.description) { -action.description = '\n' + - '(The remote method definition does not provide any description.)\n' + - ''; -} -%> - * <%-: action.description | replace:/\n/g, '\n * ' %> - * -<% -var params = action.accepts; -var postData; -if (action.getHttpMethod() == 'POST' || action.getHttpMethod() == 'PUT') { - params = params.filter(function(arg) { - return arg.http && (arg.http.source == 'query' || arg.http.source == 'path'); - }); - postData = action.accepts.filter(function(arg) { - return params.indexOf(arg) == -1; - }); -} --%> - * @param {Object=} parameters Request parameters. -<% if (params.length == 0) { -%> - * - * This method does not accept any parameters. - * Supply an empty object or omit this argument altogether. -<% } else { params.forEach(function(arg) { -%> - * - * - `<%- arg.arg %>` – `{<%- getJsDocType(arg) %>}` - <%- -(arg.description || '').replace(/\n/g, '\n * ') %> -<% }); } -%> -<% if (modelName === 'User' && methodName === 'login') { -%> - * - * - `rememberMe` - `boolean` - Whether the authentication credentials - * should be remembered in localStorage across app/browser restarts. - * Default: `true`. -<% } -%> -<% if (postData) { -%> - * - * @param {Object} postData Request data. -<% if (postData.length == 0) { -%> - * - * This method does not accept any data. Supply an empty object. -<% } else if (postData.length == 1 && postData[0].http && - postData[0].http.source == 'body') { -%> - * - * This method expects a subset of model properties as request parameters. -<% } else { -postData.forEach(function(arg) { -%> - * - * - `<%- arg.arg %>` – `{<%- getJsDocType(arg) %>}` - <%- -(arg.description || '').replace(/\n/g, '\n * ') %> -<% }); - } -} -%> - * -<% var returnType = action.isReturningArray() ? 'Array.': 'Object'; -%> - * @param {Function(<%- returnType %>, Object)=} successCb - * Success callback with two arguments: `value`, `responseHeaders`. - * - * @param {Function(Object)=} errorCb Error callback with one argument: - * `httpResponse`. - * - * @return {<%- returnType %>} An empty reference that will be - * populated with the actual data once the response is returned - * from the server. - * -<% if (!action.returns || action.returns.length == 0) { -%> - * This method returns no data. -<% } else if (action.returns[0].root) { -%> -<% if (action.returns[0].description) { -%> - * <%- action.returns[0].description -.replace(/\n/g, '\n * ').trimRight() %> -<% } else { -%> - * - * (The remote method definition does not provide any description. - * This usually means the response is a `<%- modelName %>` object.) - * -<% } -%> -<% } else { -%> - * Data properties: -<% action.returns.forEach(function(arg) { -%> - * - * - `<%- arg.arg %>` – `{<%- getJsDocType(arg) %>}` - <%- -(arg.description || '').replace(/\n/g, '\n * ') %> -<% }); - } --%> - */ + +<% ngdocForMethod(modelName, methodName, action); -%> <%-: methodName | q %>: { url: urlBase + <%-: action.getFullPath() | q %>, method: <%-: action.getHttpMethod() | q %>, @@ -300,4 +208,101 @@ function getJsDocType(arg) { if (!arg.required) type += '='; return type; } + +function ngdocForMethod(modelName, methodName, action) { +-%> + /** + * @ngdoc method + * @name lbServices.<%- modelName %>#<%- methodName %> + * @methodOf lbServices.<%- modelName %> + * + * @description + * +<% if (!action.description) { +action.description = '\n' + + '(The remote method definition does not provide any description.)\n' + + ''; +} -%> + * <%-: action.description | replace:/\n/g, '\n * ' %> + * +<% +var params = action.accepts; +var postData; +if (action.getHttpMethod() == 'POST' || action.getHttpMethod() == 'PUT') { + params = params.filter(function(arg) { + return arg.http && (arg.http.source == 'query' || arg.http.source == 'path'); + }); + postData = action.accepts.filter(function(arg) { + return params.indexOf(arg) == -1; + }); +} -%> + * @param {Object=} parameters Request parameters. +<% if (params.length == 0) { -%> + * + * This method does not accept any parameters. + * Supply an empty object or omit this argument altogether. +<% } else { params.forEach(function(arg) { -%> + * + * - `<%- arg.arg %>` – `{<%- getJsDocType(arg) %>}` - <%- +(arg.description || '').replace(/\n/g, '\n * ') %> +<% }); } -%> +<% if (modelName === 'User' && methodName === 'login') { -%> + * + * - `rememberMe` - `boolean` - Whether the authentication credentials + * should be remembered in localStorage across app/browser restarts. + * Default: `true`. +<% } -%> +<% if (postData) { -%> + * + * @param {Object} postData Request data. +<% if (postData.length == 0) { -%> + * + * This method does not accept any data. Supply an empty object. +<% } else if (postData.length == 1 && postData[0].http && + postData[0].http.source == 'body') { -%> + * + * This method expects a subset of model properties as request parameters. +<% } else { +postData.forEach(function(arg) { -%> + * + * - `<%- arg.arg %>` – `{<%- getJsDocType(arg) %>}` - <%- +(arg.description || '').replace(/\n/g, '\n * ') %> +<% }); + } +} -%> + * +<% var returnType = action.isReturningArray() ? 'Array.': 'Object'; -%> + * @param {Function(<%- returnType %>, Object)=} successCb + * Success callback with two arguments: `value`, `responseHeaders`. + * + * @param {Function(Object)=} errorCb Error callback with one argument: + * `httpResponse`. + * + * @return {<%- returnType %>} An empty reference that will be + * populated with the actual data once the response is returned + * from the server. + * +<% if (!action.returns || action.returns.length == 0) { -%> + * This method returns no data. +<% } else if (action.returns[0].root) { -%> +<% if (action.returns[0].description) { -%> + * <%- action.returns[0].description +.replace(/\n/g, '\n * ').trimRight() %> +<% } else { -%> + * + * (The remote method definition does not provide any description. + * This usually means the response is a `<%- modelName %>` object.) + * +<% } -%> +<% } else { -%> + * Data properties: +<% action.returns.forEach(function(arg) { -%> + * + * - `<%- arg.arg %>` – `{<%- getJsDocType(arg) %>}` - <%- +(arg.description || '').replace(/\n/g, '\n * ') %> +<% }); + } +-%> + */ +<% } // end of ngdocForMethod -%> From 0faab4936353ea82a8f89ffd099fa6e05e9b92f7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miroslav=20Bajto=C5=A1?= Date: Thu, 3 Apr 2014 08:01:34 +0200 Subject: [PATCH 2/6] Support internal and @deprecated methods in ngdoc --- lib/services.template | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/lib/services.template b/lib/services.template index 4b9ac3c..b77c6db 100644 --- a/lib/services.template +++ b/lib/services.template @@ -210,11 +210,20 @@ function getJsDocType(arg) { } function ngdocForMethod(modelName, methodName, action) { + if (action.internal) { +-%> + // INTERNAL. <%- action.internal %> +<% + return; + } -%> /** * @ngdoc method * @name lbServices.<%- modelName %>#<%- methodName %> * @methodOf lbServices.<%- modelName %> +<% if (action.deprecated) { -%> + * @deprecated <%- action.deprecated %> +<% } -%> * * @description * From c6fa10f29e556f1149e982f9fd459ca4835d062f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miroslav=20Bajto=C5=A1?= Date: Thu, 3 Apr 2014 08:03:12 +0200 Subject: [PATCH 3/6] Allow e2e tests to run custom setup code on server Sample usage: // data for the POST /setup request JSON.stringify({ models: { Product: { properties: { name: 'string' } }, }, setupFn: (function(app, cb) { app.models.Product.create( { name: 'a-product' }, function(err, product) { if (err) return cb(err); cb(null, { productId: product.id } }); }).toString() }) The server will run the provided function and wait until the callback is called. Data passed to the callback are exposed to browser tests as "testData" and can be accessed via the angular injector: $injector.get("testData").productId --- test.e2e/test-server.js | 49 +++++++++++++++++++++++++++++++++-------- 1 file changed, 40 insertions(+), 9 deletions(-) diff --git a/test.e2e/test-server.js b/test.e2e/test-server.js index ec91d17..9c95961 100644 --- a/test.e2e/test-server.js +++ b/test.e2e/test-server.js @@ -42,13 +42,23 @@ Sample request } } // other model objects - } - */ + }, + setupFn: (function(app, cb) { + Customer.create( + { name: 'a-customer' }, + function(err, customer) { + if (err) return cb(err); + cb(null, { customer: customer }); + }); + }).toString() +} +*/ masterApp.post('/setup', function(req, res, next) { var opts = req.body; var name = opts.name; var models = opts.models; var enableAuth = opts.enableAuth; + var setupFn = compileSetupFn(name, opts.setupFn); if (!name) return next(new Error('"name" is a required parameter')); @@ -74,16 +84,37 @@ masterApp.post('/setup', function(req, res, next) { lbApp.set('restApiRoot', '/'); lbApp.installMiddleware(); - try { - servicesScript = generator.services(lbApp, name, apiUrl); - } catch (err) { - console.error('Cannot generate services script:', err.stack); - servicesScript = 'throw new Error("Error generating services script.");'; - } + setupFn(lbApp, function(err, data) { + if (err) { + console.error('app setup function failed', err); + res.send(500, err); + return; + } + + try { + servicesScript = generator.services(lbApp, name, apiUrl); + } catch (err) { + console.error('Cannot generate services script:', err.stack); + servicesScript = 'throw new Error("Error generating services script.");'; + } + + servicesScript += '\nmodule.value("testData", ' + + JSON.stringify(data, null, 2) + ');\n'; + + res.send(200, { servicesUrl: baseUrl + 'services?' + name }); + }.bind(this)); - res.send(200, { servicesUrl: baseUrl + 'services?' + name }); }); +function compileSetupFn(name, source) { + if (!source) + return function(app, cb) { cb(); }; + + var debug = require('debug')('test:' + name); + /*jshint evil:true */ + return eval('(' + source + ')'); +} + masterApp.get('/services', function(req, res, next) { res.set('Content-Type', 'application/javascript'); res.send(200, servicesScript); From 1b59e4045da94db7a8c29dc63bd6815bc6593091 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miroslav=20Bajto=C5=A1?= Date: Thu, 3 Apr 2014 08:11:40 +0200 Subject: [PATCH 4/6] Prevent reuse of model classes between tests The LoopBack keeps all models in a static registry. When two tests are using the same model name, some of the model classes may get shared, causing bugs difficult to debug. This commit modifies "/setup" handler to reset the static registry to the initial state every time a new application is created. --- test.e2e/test-server.js | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/test.e2e/test-server.js b/test.e2e/test-server.js index 9c95961..4092aa8 100644 --- a/test.e2e/test-server.js +++ b/test.e2e/test-server.js @@ -7,6 +7,7 @@ a custom LoopBack instance and generate & access lb-services.js var express = require('express'); var loopback = require('loopback'); var generator = require('..'); +var extend = require('util')._extend; var port = process.env.PORT || 3838; var baseUrl; @@ -20,6 +21,10 @@ var servicesScript; // for tests using the built-in User model loopback.User.settings.saltWorkFactor = 4; +// Save the pre-build models so that we can restore them before every test +var initialModels = loopback.Model.modelBuilder.models; +var initialDefinitions = loopback.Model.modelBuilder.definitions; + // Enable all domains to access our server via AJAX // This way the script running in Karma page can // talk to our service running on a different host/port. @@ -66,6 +71,10 @@ masterApp.post('/setup', function(req, res, next) { if (!models || typeof models !== 'object') return next(new Error('"models" must be a valid object')); + // hack: clear the static model registry populated by previous test apps + loopback.Model.modelBuilder.models = extend({}, initialModels); + loopback.Model.modelBuilder.definitions = extend({}, initialDefinitions); + lbApp = loopback(); lbApp.dataSource('db', { connector: 'memory', defaultForType: 'db' }); From 83404c0b212e250efb2b5e1a6f7dabffc97837b3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miroslav=20Bajto=C5=A1?= Date: Thu, 3 Apr 2014 09:44:09 +0200 Subject: [PATCH 5/6] Support hasAndBelongsToMany scope methods The relation "Product hasAndBelongsToMany Category" creates a scope property on the Product model with three methods: get, create, delete. At the Angular side, these methods are defined on the Product $resource, therefore they return the result as Product instead of Category. The solution presented in this commit has two parts: 1. The Category $resource has internal methods calling remote (REST) Product.categories.* methods. This way the result is returned as Category. 2. The Product $resource has extra properties (methods) with hand-coded implementation delegating to the internal Category methods. Methods & delegations: Product.categories() -> Category['::get::Product::categories']() Product.categories.create() -> Category['::create::Product::categories']() Product.categories.destroyAll() -> Category['::delete::Product::categories']() The original scope methods (e.g. prototype$__get__{scope}) are marked as deprecated, the deprecation text includes the name of the new method to use. --- lib/services.js | 100 ++++++++++++++++++++++++++++ lib/services.template | 36 ++++++++-- test.e2e/spec/services.spec.js | 117 +++++++++++++++++++++++++++++++++ 3 files changed, 248 insertions(+), 5 deletions(-) diff --git a/lib/services.js b/lib/services.js index 033f71e..8c8b86e 100644 --- a/lib/services.js +++ b/lib/services.js @@ -37,5 +37,105 @@ function describeModels(app) { result[name] = c; }); + + buildScopes(result); + return result; } + +var SCOPE_METHOD_REGEX = /^prototype.__([^_]+)__(.+)$/; + +function buildScopes(models) { + for (var modelName in models) { + buildScopesOfModel(models, modelName); + } +} + +function buildScopesOfModel(models, modelName) { + var modelClass = models[modelName]; + + modelClass.scopes = {}; + modelClass.methods.forEach(function(method) { + buildScopeMethod(models, modelName, method); + }); + + return modelClass; +} + +// reverse-engineer scope method +// defined by loopback-datasource-juggler/lib/scope.js +function buildScopeMethod(models, modelName, method) { + var modelClass = models[modelName]; + var match = method.name.match(SCOPE_METHOD_REGEX); + if (!match) return; + + var op = match[1]; + var scopeName = match[2]; + var modelPrototype = modelClass.sharedClass.ctor.prototype; + var targetClass = modelPrototype[scopeName]._targetClass; + + if (modelClass.scopes[scopeName] === undefined) { + if (!targetClass) { + console.error( + 'Warning: scope %s.%s is missing _targetClass property.' + + '\nThe Angular code for this scope won\'t be generated.' + + '\nPlease upgrade to the latest version of' + + '\nloopback-datasource-juggler to fix the problem.', + modelName, scopeName); + modelClass.scopes[scopeName] = null; + return; + } + + if (!findModelByName(models, targetClass)) { + console.error( + 'Warning: scope %s.%s targets class %j, which is not exposed ' + + '\nvia remoting. The Angular code for this scope won\'t be generated.', + modelName, scopeName, targetClass); + modelClass.scopes[scopeName] = null; + return; + } + + modelClass.scopes[scopeName] = { + methods: {}, + targetClass: targetClass + }; + } else if (modelClass.scopes[scopeName] === null) { + // Skip the scope, the warning was already reported + return; + } + + var apiName = scopeName; + if (op == 'get') { + // no-op, create the scope accessor + } else if (op == 'delete') { + apiName += '.destroyAll'; + } else { + apiName += '.' + op; + } + + method.deprecated = 'Use ' + modelName + '.' + apiName + '() instead.'; + + // build a reverse record to be used in ngResource + // Product.__find__categories -> Category.::find::product::categories + var reverseName = '::' + op + '::' + modelName + '::' + scopeName; + + var reverseMethod = Object.create(method); + reverseMethod.name = reverseName; + delete reverseMethod.deprecated; + reverseMethod.internal = 'Use ' + modelName + '.' + apiName + '() instead.'; + + var reverseModel = findModelByName(models, targetClass); + reverseModel.methods.push(reverseMethod); + + var scopeMethod = Object.create(method); + scopeMethod.name = reverseName; + delete scopeMethod.deprecated; + modelClass.scopes[scopeName].methods[apiName] = scopeMethod; +} + +function findModelByName(models, name) { + for (var n in models) { + if (n.toLowerCase() == name.toLowerCase()) + return models[n]; + } +} diff --git a/lib/services.template b/lib/services.template index b77c6db..177c23a 100644 --- a/lib/services.template +++ b/lib/services.template @@ -42,8 +42,8 @@ var module = angular.module(<%-: moduleName | q %>, ['ngResource']); */ module.factory( <%-: modelName | q %>, - ['LoopBackResource', 'LoopBackAuth', function(Resource, LoopBackAuth) { - return Resource( + ['LoopBackResource', 'LoopBackAuth', '$injector', function(Resource, LoopBackAuth, $injector) { + var R = Resource( urlBase + <%-: meta.ctor.getFullPath() | q %>, <% /* Constructor arguments are hardcoded for now. @@ -54,7 +54,6 @@ module.factory( <% meta.methods.forEach(function(action) { var methodName = action.name.split('.').join('$'); -%> - <% ngdocForMethod(modelName, methodName, action); -%> <%-: methodName | q %>: { url: urlBase + <%-: action.getFullPath() | q %>, @@ -122,6 +121,29 @@ module.factory( <% } -%> } ); + +<% for (var scopeName in meta.scopes) { + var scope = meta.scopes[scopeName]; + if (!scope) continue; + var scopeMethods = scope.methods; + // Angular names always start with a capital letter + var targetClass = scope.targetClass[0].toUpperCase() + scope.targetClass.slice(1); + + // sort the names to make sure the get method creating the scope + // is emitted first (R.categories before R.categories.create) + Object.keys(scopeMethods).sort().forEach(function(methodName) { + var action = scopeMethods[methodName]; + ngdocForMethod(modelName, methodName, action, targetClass); +-%> + R.<%- methodName %> = function() { + var TargetResource = $injector.get(<%-: targetClass | q %>); + var action = TargetResource[<%-: action.name | q %>]; + return action.apply(R, arguments); + }; +<% }); // forEach methods name -%> +<% } // for each scope -%> + + return R; }]); <% } // for modelName in models -%> @@ -209,7 +231,11 @@ function getJsDocType(arg) { return type; } -function ngdocForMethod(modelName, methodName, action) { +function ngdocForMethod(modelName, methodName, action, responseModelName) { + // always add an empty line before the ngdoc comment: +-%> + +<% if (action.internal) { -%> // INTERNAL. <%- action.internal %> @@ -301,7 +327,7 @@ postData.forEach(function(arg) { -%> <% } else { -%> * * (The remote method definition does not provide any description. - * This usually means the response is a `<%- modelName %>` object.) + * This usually means the response is a `<%- responseModelName || modelName %>` object.) * <% } -%> <% } else { -%> diff --git a/test.e2e/spec/services.spec.js b/test.e2e/spec/services.spec.js index cc740d7..5b5ba25 100644 --- a/test.e2e/spec/services.spec.js +++ b/test.e2e/spec/services.spec.js @@ -258,5 +258,122 @@ define(['angular', 'given', 'util'], function(angular, given, util) { }); } }); + + describe('for models with hasAndBelongsToMany relations', function() { + var $injector, Product, Category, testData; + before(function() { + return given.servicesForLoopBackApp( + { + models: { + Product: { + properties: { name: 'string' }, + options: { + relations: { + categories: { + model: 'Category', + type: 'hasAndBelongsToMany' + } + } + } + }, + Category: { + properties: { name: 'string' }, + options: { + relations: { + products: { + model: 'Product', + type: 'hasAndBelongsToMany' + } + } + } + } + }, + setupFn: (function(app, cb) { + /*globals debug:true */ + app.models.Product.create({ name: 'p1' }, function(err, prod) { + if (err) return cb(err); + debug('Created product', prod); + + prod.categories.create({ name: 'c1' }, function(err, cat) { + if (err) return cb(err); + debug('Created category', cat); + + prod.categories(true, function(err, list) { + if (err) return cb(err); + debug('Categories of product', list); + + cb(null, { + product: prod, + category: cat + }); + }); + }); + }); + }).toString() + }) + .then(function(createInjector) { + $injector = createInjector(); + Product = $injector.get('Product'); + Category = $injector.get('Category'); + testData = $injector.get('testData'); + }); + }); + + it('provides scope methods', function() { + expect(Object.keys(Product), 'Product properties') + .to.contain('categories'); + expect(Object.keys(Product.categories), 'Product.categories properties') + .to.have.members([ + 'create', + 'destroyAll' + ]); + }); + + it('gets related models with correct prototype', function() { + var list = Product.categories({ id: testData.product.id }); + return list.$promise.then(function() { + // eql does not work for arrays with objects correctly :( + expect(list).to.have.length(1); + expect(list[0]).to.eql(new Category(testData.category)); + }); + }); + + it('creates a related model', function() { + var cat = Product.categories.create( + { id: testData.product.id }, + { name: 'another-cat' }); + return cat.$promise + .then(function() { + expect(cat).to.be.an.instanceof(Category); + expect(cat).to.have.property('name', 'another-cat'); + }) + .then(function() { + var list = Product.categories({ id: testData.product.id }); + return list.$promise.then(function() { + var names = list.map(function(c) { return c.name; }); + expect(names).to.eql([testData.category.name, cat.name]); + }); + }); + }); + + // Skipped due to strongloop/loopback-datasource-juggler#95 + it.skip('removes all related models', function() { + return Product.categories.destroyAll({ id: testData.product.id }) + .$promise + .then(function() { + var list = Product.categories({ id: testData.product.id }); + return list.$promise.then(function() { + expect(list, 'product categories').to.have.length(0); + }); + }) + .then(function() { + var all = Product.find({ filter: true }); + return all.$promise + .then(function() { + expect(all, 'all categories').to.have.length(0); + }); + }); + }); + }); }); }); From e42427c893a994dafb2697235d70ac103b7a5dfe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miroslav=20Bajto=C5=A1?= Date: Thu, 3 Apr 2014 10:29:47 +0200 Subject: [PATCH 6/6] sharedCtor arguments in ngdoc of prototype methods The URL of prototype methods includes sharedCtor parameters like ":id", for example GET /api/Products/:id/categories Because all $resource actions are static (non-prototype) in ngResource, callers has to include sharedCtor parameters in the arguments. Product.categories({ id: productId }); This commit modifies the generated ngdoc to include sharedCtor parameters in actions for prototype methods. --- lib/services.js | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/lib/services.js b/lib/services.js index 8c8b86e..f986f77 100644 --- a/lib/services.js +++ b/lib/services.js @@ -35,6 +35,16 @@ function describeModels(app) { return; } + // The URL of prototype methods include sharedCtor parameters like ":id" + // Because all $resource methods are static (non-prototype) in ngResource, + // the sharedCtor parameters should be added to the parameters + // of prototype methods. + c.methods.forEach(function fixArgsOfPrototypeMethods(method) { + var ctor = method.restClass.ctor; + if (!ctor || method.sharedMethod.isStatic) return; + method.accepts = ctor.accepts.concat(method.accepts); + }); + result[name] = c; });