Account service without juggler #51

b-admike · 2017-05-23T15:55:32Z

connect to https://github.com/strongloop-internal/scrum-apex/issues/215
connect to https://github.com/strongloop-internal/scrum-apex/issues/228

kjdelisle · 2017-05-25T23:00:50Z

services/accountsanjuggler/repositories/account/models/account/model-definition.json

+      "required": true
+    }
+  }
+}


missing newline

raymondfeng · 2017-06-23T20:12:16Z

services/accountsansjuggler/controllers/AccountController.ts

+
+   async deleteAccount(id) {
+    return await this.repository.deleteById(id);
+}


Please fix the indentation.

raymondfeng · 2017-06-23T20:12:59Z

services/accountsansjuggler/bin/get-account

@@ -0,0 +1,2 @@
+#!/usr/bin/env bash


A better name for accountsanjuggler?

raymondfeng · 2017-06-23T20:23:55Z

services/accountsansjuggler/repositories/account/datasources/mysqlds.ts

+    Where
+} from 'loopback-next/packages/repository';
+
+export class MySqlConn implements CrudConnector {


Please refactor it into a new file.

raymondfeng · 2017-06-23T20:24:55Z

services/accountsansjuggler/repositories/account/datasources/mysqlds.ts

+              })
+          })
+      }
+


Please format the code with correct indentation. If you use VSCode, try prettier javascript formatter.

ssh24 · 2017-06-29T15:51:59Z

services/account-without-juggler/repositories/account/datasources/mysqlconn.ts

@@ -0,0 +1,229 @@
+const debug = require("debug")("accountsansjuggler");


Should we change the debug tag to account-without-juggler instead?

ssh24 · 2017-06-29T15:57:04Z

services/account-without-juggler/test/unit/account-controller.ts

+    expect(getResult).to.not.be.empty();
+    expect(getResult).have.lengthOf(1);
+    expect(getResult[0].id).to.be.equal(testAcc.id);
+    expect(JSON.parse(JSON.stringify(getResult[0])).balance).to.be.equal(2000);


getResult[0].toObject().balance should work.

ssh24 · 2017-06-29T15:57:46Z

services/account-without-juggler/tslint.full.json

+  "extends": [
+    "./tslint.json"
+  ],
+  // This configuration files enabled rules which require type checking


Are these comments needed?

ssh24 · 2017-06-29T15:57:55Z

services/account-without-juggler/tslint.json

@@ -0,0 +1,72 @@
+{
+  // See https://palantir.github.io/tslint/rules/


I think these comments were copied from loopback-next. I added this particular comment in loopback-next to make it easier for people that are not familiar with tslint to find a reference of all rules.

What are your objections against having these comments in place?

Ah, I see. Disregard my comments in that case.

bajtos · 2017-06-29T16:32:18Z

services/account-without-juggler/index.ts

+    super();
+    const app = this;
+    app.controller(AccountController);
+    app.bind('repositories.account').toClass(accountRepository);


It would be nice to introduce app.repository API to take care of this.

app.repository(accountRepository);

The argument should be strongly typed to a base Repository interface, which is something that we cannot achieve via bind().toClass(), at least not now.

@raymondfeng thoughts on the above API to take care of this action?

I think we can land and create an issue (feature) for the sugar API mentioned by @bajtos. This PR is huge already.

@b-admike Please link to the follow up issue here for posterity.

Related issue: loopbackio/loopback-next#425

bajtos · 2017-06-29T16:33:36Z

services/account-without-juggler/package.json

+  "main": "index.ts",
+  "dependencies": {
+    "@loopback/testlab": "^4.0.0-alpha.5",
+    "@types/mocha": "^2.2.41",


I think both testlab and mocha should go to devDependencies, shouldn't it? Why do you actually need @types/mocha?

bajtos · 2017-06-29T16:37:25Z

services/account-without-juggler/repositories/account/datasources/mysqlconn.ts

+  connect() {
+    let self = this;
+    return new Promise<void>(function(resolve, reject) {
+      self.connection.connect(function(err: any) {


Have you considered using https://www.npmjs.com/package/mysql-promise or https://www.npmjs.com/package/promise-mysql, instead of this manual way of adding promise support?

bajtos · 2017-06-29T16:39:11Z

services/account-without-juggler/repositories/account/datasources/mysqlconn.ts

+    sqlStmt += "(" + vals.join(",") + ")" + ";";
+    debug("insert stmt ", sqlStmt);
+    return new Promise<Account>(function(resolve, reject) {
+      self.connection.query(sqlStmt, function(err: any, count: any) {


Have you considered using https://www.npmjs.com/package/@types/mysql to get better type information? (I don't know how it will work with promise-wrapped mysql packages though.)

bajtos · 2017-06-29T16:40:57Z

services/account-without-juggler/repositories/account/datasources/mysqlconn.ts

+    options: Options
+  ): Promise<EntityData> {
+    let self = this;
+    let sqlStmt = "INSERT INTO " + modelClass.name + " VALUES ";


I think you should escape modelCass.name in order to prevent SQL script injection. (Class names probably cannot be changed so much as to cause security problem, but I can imagine there may be valid class names that cannot be used in SQL without escaping.)

bajtos · 2017-06-29T16:44:13Z

services/account-without-juggler/repositories/account/datasources/mysqlconn.ts

+    let sqlStmt = "UPDATE " + modelClass.name + " SET ";
+    var vals = [];
+    for (var prop in data) {
+      vals.push(prop + "=" + data[prop]);


SQL injection.

bajtos · 2017-06-29T16:44:33Z

services/account-without-juggler/repositories/account/datasources/mysqlconn.ts

+    for (var prop in data) {
+      vals.push(prop + "=" + data[prop]);
+    }
+    sqlStmt += vals.join(",") + " WHERE " + "`id`=" + id + ";";


SQL injection - pass id as a parameter, let the driver to escape it.

bajtos · 2017-06-29T16:44:44Z

services/account-without-juggler/repositories/account/datasources/mysqlconn.ts

+    options: Options
+  ): Promise<boolean> {
+    let self = this;
+    let sqlStmt = "DELETE FROM " + modelClass.name + " WHERE `id`=" + id + ";";


SQL injection - pass id as a parameter, let the driver to escape it.

bajtos · 2017-06-29T16:45:57Z

services/account-without-juggler/test/unit/account-controller.ts

@@ -0,0 +1,73 @@
+import { AccountController } from "../../controllers/AccountController";


We use single quotes for string delimiters in LoopBack code (and in Node.js code in general). This comment applies to all other JS/TS source files in this pull request too.

bajtos · 2017-06-29T16:48:10Z

services/account-without-juggler/tslint.json

@@ -0,0 +1,72 @@
+{
+  // See https://palantir.github.io/tslint/rules/


I think these comments were copied from loopback-next. I added this particular comment in loopback-next to make it easier for people that are not familiar with tslint to find a reference of all rules.

What are your objections against having these comments in place?

ssh24 · 2017-07-06T20:40:19Z

services/account-without-juggler/repositories/account/datasources/mysqlconn.ts

+  private connection: any;
+
+  constructor(config: Object) {
+    db.configure(config, require("mysql2"));


Are we using the mysql2 driver?

That is not what our module (loopback-connector-mysql) uses though. Should we not test this with the same driver that our module currently uses?

superkhau · 2017-07-06T20:41:53Z

services/account-without-juggler/controllers/AccountController.ts

+
+@api(def)
+export class AccountController {
+  @inject('repositories.account') private repository: accountRepository;


move into constructor so that repository can be replaced by constructor injection

superkhau

Did only a part of the review for now, you can change it up and ping me again.

superkhau · 2017-07-06T20:43:10Z

services/account-without-juggler/index.ts

+    super();
+    const app = this;
+    app.controller(AccountController);
+    app.bind('repositories.account').toClass(accountRepository);


I think we can land and create an issue (feature) for the sugar API mentioned by @bajtos. This PR is huge already.

superkhau · 2017-07-06T20:45:07Z

services/account-without-juggler/repositories/account/datasources/mysql.json

+    "host":"localhost",
+    "user":"root",
+    "password": "dis-mysql",
+    "database": "test-db",


let's make it simpler for future users

password: "password" // password
database: "loopback-next-example" // name of example

should be consistent across all examples

superkhau · 2017-07-06T20:47:32Z

services/account-without-juggler/repositories/account/datasources/mysqlconn.ts

@@ -0,0 +1,201 @@
+const debug = require("debug")("account-without-juggler");


const debug = require('debug')('loopback:repositories:account:datasources:connections:mysql');

Single quote everything in this file too

superkhau · 2017-07-06T20:48:00Z

services/account-without-juggler/repositories/account/datasources/mysqlconn.ts

+} from "loopback-next/packages/repository";
+
+export class MySqlConn implements CrudConnector {
+  private connection: any;


: Connection, let's get away from any

We don't have that exported in Repository, but I was trying to make it of type IConnection from https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/mysql/index.d.ts#L39-#L83, but the return types for connect and disconnect don't align with ours (Promise<void>)

Let's not prefix with I. You can create your own type or interface: See https://github.com/strongloop/loopback-next/blob/master/packages/core/src/internal-types.ts. If we need to create a proxy to normalize the interfaces, so be it. cc @raymondfeng

superkhau · 2017-07-06T20:48:23Z

services/account-without-juggler/repositories/account/datasources/mysqlconn.ts

+  private connection: any;
+
+  constructor(config: Object) {
+    db.configure(config, require("mysql2"));


move the require up to the top of the file

superkhau · 2017-07-06T20:49:08Z

services/account-without-juggler/repositories/account/datasources/mysqlconn.ts

+    return this.connection.ping();
+  }
+
+  public updateAll(


public is default, remove for consistency

My review isn't relevant due to age, don't let it hold this PR up.

b-admike · 2017-07-06T21:39:06Z

@ssh24 @superkhau feedback applied, PTAL

superkhau · 2017-07-07T01:19:51Z

services/account-without-juggler/controllers/AccountController.api.ts

+            in: 'query',
+            description: 'The id for the model instance to be deleted.',
+            required: true,
+             type:'object',


nit, indent off by one space here
and id -> ID or identifier

superkhau · 2017-07-07T01:20:39Z

services/account-without-juggler/controllers/AccountController.api.ts

+              properties: {
+                count: {
+                  type: 'number',
+                  description: 'number of records deleted',


superkhau · 2017-07-07T01:20:47Z

services/account-without-juggler/controllers/AccountController.api.ts

+              properties: {
+                count: {
+                  type: 'number',
+                  description: 'number of records updated',


The number
and period at the end of your sentence

superkhau · 2017-07-07T01:21:26Z

services/account-without-juggler/controllers/AccountController.api.ts

+         {
+            name: 'data',
+            in: 'body',
+            description: 'An object of model property name/value pairs',


Period at the end for your description

superkhau · 2017-07-07T01:21:48Z

services/account-without-juggler/controllers/AccountController.api.ts

+          {
+            name: 'id',
+            in: 'query',
+            description: 'The id of the model instance to be updated',


Period at the end for your description.

superkhau · 2017-07-07T01:22:28Z

services/account-without-juggler/controllers/AccountController.api.ts

+          200: {
+            schema: {
+              type: 'object',
+              description: 'update information',


Full sentence description here pls.

superkhau · 2017-07-07T01:22:53Z

services/account-without-juggler/controllers/AccountController.api.ts

+          200: {
+            schema: {
+              type: 'object',
+              description: 'delete information',


better desc here too

superkhau · 2017-07-07T01:23:36Z

services/account-without-juggler/controllers/AccountController.ts

+@api(def)
+export class AccountController {
+  constructor(
+    @inject('repositories.account') private repository: accountRepository


Shouldn't type be AccountRepository?

superkhau · 2017-07-07T01:25:25Z

services/account-without-juggler/repositories/account/index.ts

+import { Account } from './models/Account';
+
+
+export class accountRepository extends CrudRepositoryImpl<Account, string> {


Should be AccountRepository

superkhau · 2017-07-07T01:27:14Z

services/account-without-juggler/tslint.json

+      "check-type",
+      "check-typecast",
+      "check-preblock"
+    ]


Can you separate out this linting stuff into another PR?

b-admike · 2017-07-07T13:38:05Z

@superkhau PTAL

superkhau

LGTM, we can refactor anything else that needs updating later.

superkhau · 2017-07-07T20:46:51Z

services/account-without-juggler/index.ts

+    super();
+    const app = this;
+    app.controller(AccountController);
+    app.bind('repositories.account').toClass(accountRepository);


@b-admike Please link to the follow up issue here for posterity.

superkhau · 2017-07-07T20:47:24Z

services/account-without-juggler/repositories/account/datasources/mysql.json

+    "host":"localhost",
+    "user":"root",
+    "password": "password",
+    "database": "account-service",


Use the name of the example to distinguish from other databases the user may have locally before.

superkhau · 2017-07-07T20:47:58Z

services/account-without-juggler/repositories/account/datasources/mysqlconn.ts

+const mysql = require('mysql');
+const db = require('mysql-promise')();
+import { CrudConnector } from 'loopback-next/packages/repository/src/crud-connector';
+import { IError, IConnection } from '@types/mysql';


Do not use I for interfaces, should just be Error + Connection

superkhau · 2017-07-07T20:48:22Z

services/account-without-juggler/repositories/account/datasources/mysqlds.ts

+    this.settings = require("./mysql.json");
+    this.connector = new MySqlConn(this.settings);
+  }
+}


single quotes in the whole file pls

superkhau · 2017-07-07T20:48:32Z

services/account-without-juggler/repositories/account/datasources/mysqlds.ts

+  settings: Object;
+
+  constructor() {
+    this.settings = require("./mysql.json");


move to top of file

superkhau · 2017-07-07T20:48:59Z

services/account-without-juggler/repositories/account/index.ts

+
+export class AccountRepository extends CrudRepositoryImpl<Account, string> {
+  constructor() {
+    const ds = new MySQLDs();


superkhau · 2017-07-07T20:49:16Z

services/account-without-juggler/repositories/account/models/Account.ts

+export class Account extends Entity {
+  static definition = new ModelDefinition("Account", require("./account/model-definition").properties);
+  static modelName = "Account";
+}


ditto for single quotes

superkhau · 2017-07-07T20:49:35Z

services/account-without-juggler/repositories/account/models/account/model-definition.js

+      "required": true
+    }
+  }
+};


single quotes and keys don't need quotes

superkhau · 2017-07-07T20:50:18Z

services/account-without-juggler/test/unit/account-controller.ts

+function createAccountController() {
+  accCtrl = new AccountController();
+  accCtrl.repository = new accountRepository();
+}


No abbreviations with var names pls

superkhau · 2017-07-07T20:51:53Z

Please update the README with working instructions before landing too

b-admike · 2017-07-10T04:30:28Z

@slnode test please

b-admike added review and removed review labels May 23, 2017

b-admike closed this May 23, 2017

b-admike force-pushed the validate-repo-without-juggler branch from 230dd41 to c63c953 Compare May 23, 2017 18:27

b-admike reopened this May 25, 2017

b-admike added the review label May 25, 2017

kjdelisle previously requested changes May 25, 2017

View reviewed changes

b-admike force-pushed the validate-repo-without-juggler branch from cdb602c to d3d6a79 Compare June 15, 2017 15:29

b-admike changed the title ~~Validate repo without juggler~~ [DO NOT MERGE] Validate repo without juggler Jun 19, 2017

b-admike force-pushed the validate-repo-without-juggler branch 2 times, most recently from fe897d2 to 8cb7ab4 Compare June 23, 2017 20:00

raymondfeng reviewed Jun 23, 2017

View reviewed changes

services/accountsansjuggler/bin/get-account Outdated

@@ -0,0 +1,2 @@

#!/usr/bin/env bash

Copy link

Member

raymondfeng Jun 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A better name for accountsanjuggler?

raymondfeng reviewed Jun 23, 2017

View reviewed changes

b-admike force-pushed the validate-repo-without-juggler branch 4 times, most recently from 730cc20 to 0c8ba95 Compare June 28, 2017 13:37

b-admike changed the title ~~[DO NOT MERGE] Validate repo without juggler~~ Account service without juggler Jun 28, 2017

b-admike requested review from bajtos, superkhau and raymondfeng June 29, 2017 13:50

ssh24 suggested changes Jun 29, 2017

View reviewed changes

bajtos reviewed Jun 29, 2017

View reviewed changes

dhmlau mentioned this pull request Jun 30, 2017

[LB-Next] Assess amount of work to convert existing LB3 connector to TypeScript loopbackio/loopback-next#421

Closed

b-admike force-pushed the validate-repo-without-juggler branch from ef60536 to 19964a3 Compare July 6, 2017 20:39

ssh24 reviewed Jul 6, 2017

View reviewed changes

superkhau reviewed Jul 6, 2017

View reviewed changes

superkhau requested changes Jul 6, 2017

View reviewed changes

b-admike force-pushed the validate-repo-without-juggler branch from 1974c36 to bd999bf Compare July 6, 2017 21:37

b-admike removed the request for review from raymondfeng July 6, 2017 21:39

ssh24 approved these changes Jul 6, 2017

View reviewed changes

superkhau requested changes Jul 7, 2017

View reviewed changes

superkhau reviewed Jul 7, 2017

View reviewed changes

b-admike force-pushed the validate-repo-without-juggler branch from eee0a2a to 4867443 Compare July 7, 2017 13:37

b-admike force-pushed the validate-repo-without-juggler branch 2 times, most recently from 0dbd6b9 to 2496c42 Compare July 7, 2017 15:13

b-admike requested a review from raymondfeng July 7, 2017 20:43

superkhau approved these changes Jul 7, 2017

View reviewed changes

App using persistence branch without juggler

7b654da

b-admike force-pushed the validate-repo-without-juggler branch from 2496c42 to 7b654da Compare July 10, 2017 04:21

b-admike merged commit 413e753 into master Jul 10, 2017

b-admike deleted the validate-repo-without-juggler branch July 10, 2017 04:33

		@@ -0,0 +1,229 @@
		const debug = require("debug")("accountsansjuggler");

		@@ -0,0 +1,72 @@
		{
		// See https://palantir.github.io/tslint/rules/

		@@ -0,0 +1,73 @@
		import { AccountController } from "../../controllers/AccountController";

		@@ -0,0 +1,201 @@
		const debug = require("debug")("account-without-juggler");

		import { Account } from './models/Account';


		export class accountRepository extends CrudRepositoryImpl<Account, string> {

Account service without juggler #51

Account service without juggler #51

Conversation

b-admike commented May 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

superkhau Jul 6, 2017 • edited Loading

Choose a reason for hiding this comment

superkhau left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

b-admike commented Jul 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

b-admike commented Jul 7, 2017

superkhau left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

superkhau commented Jul 7, 2017 • edited Loading

b-admike commented Jul 10, 2017

b-admike commented May 23, 2017 •

edited

Loading

superkhau Jul 6, 2017 •

edited

Loading

superkhau commented Jul 7, 2017 •

edited

Loading