WFM api framework - HTTP RESTfull api for WFM (ex-webapi) #75

wtrocki · 2017-08-08T16:32:13Z

Motivation

Allow to use express based API to operate on WFM objects.

Note: this changes are WFM specific and it will require integration with WFM module = sparate PR in angular.js repository

Notes

I have done some investigation for pagination and I did not found anything that will meet all of our criteria. I wrote simple pagination mechanism for this module. We can extract this to separate module if needed somewhere else but I think it makes sense to have it here.

Progress

paolobueno · 2017-08-09T12:42:53Z

demo/server/src/modules/wfm-web-api/DataService.ts

+
+  public list() {
+    return this.db.collection(this.collectionName).find({});
+  }


I think this class, or an interface definition of it should be async, not all storage libs can fit a synchronous api.

wtrocki · 2017-08-10T10:37:57Z

demo/server/Gruntfile.js

@@ -14,6 +14,7 @@ module.exports = function (grunt) {
        },
        exec: {
            'run': 'ts-node src/index.ts',
+            'runDebug': 'ts-node --inspect src/index.ts',


@austincunningham We can debug now.

Why do we need grunt over this same line in an npm script?

Ah, I guess it's for reloading the app? There's nodemon for that too

hmm.. actually it's for life reload that was supported in previous application..
I couldn't really get that running so life reload will not work now :(

paolobueno · 2017-08-10T20:19:43Z

cloud/wfm-rest-api/example/index.ts

+    api.setDb(db);
+  } else {
+    console.info('MongoDb not connected', err);
+    process.exit();


Should add some non-zero value to make sure the process exits with an error: process.exit(1).

Alternatively this could be done before starting the http server at all:
promisify(MongoClient.connect)(...).then(db => api.setDb(db)).then(() => app.listen(...));

paolobueno · 2017-08-10T20:21:42Z

cloud/wfm-rest-api/src/ApiConfig.ts

+/**
+ * Default module configuration
+ */
+export const defaultConfiguration = {


could implement ApiConfig, just to inherit jsdoc info

paolobueno · 2017-08-10T20:21:53Z

cloud/wfm-rest-api/src/ApiConfig.ts

+  resultApiName?: string;
+  workorderCollectionName?: string;
+  workflowCollectionName?: string;
+  resultCollectionName?: string;


need jsdocs for these keys

paolobueno · 2017-08-10T20:32:25Z

cloud/wfm-rest-api/src/WfmRestApi.ts

+    const router: express.Router = express.Router();
+    const workorderController = new ApiController(router, this.workorderService, this.config.workorderApiName);
+    const workflowController = new ApiController(router, this.workflowService, this.config.workflowApiName);
+    const resultController = new ApiController(router, this.resultService, this.config.resultApiName);


-1 for extra class here, this is using a constructor only for side-effects (mutating router).

Just need something like

router.use(`/${workorderApiName}`, buildRouter(this.workorderService))

so you don't need to mutate router inside another function, and reduces # of params for buildRouter.

👍 That will be also easy to unit tests. Do you see api exposing each crud method or we should get entire CRUD operations as now?

I think it's okay to return a router with all the methods, the idea behind router.use('/mount-point', middleware) is that that middleware can properly handle everything from that point onwards.

paolobueno · 2017-08-10T20:34:47Z

cloud/wfm-rest-api/src/data-api/CrudRepository.ts

+ * Interface is being used internally to perform database operations for WFM models
+ * It's not recomended to use it for other application business logic.
+ */
+export interface CrudRepository {


I don't like using the term CRUD itself, feels like too much slang and signals boilerplate code.

Could be PagingDataRepository or something, much fancier :D

paolobueno · 2017-08-10T20:38:09Z

cloud/wfm-rest-api/src/data-api/PaginationEngine.ts

+ * Note: pages are counted from number 1.
+ * Page 0 requests will return empty data.
+ */
+export class PaginationEngine {


Maybe MongoPaginationEngine? Implementation is mongo-specific.

paolobueno · 2017-08-10T20:41:46Z

cloud/wfm-rest-api/src/impl/ApiController.ts

+      });
+    });
+  }
+  protected errorHandler(req: express.Request, res: express.Response, error: ApiError) {


We don't need a local error handler for this router.
This would prevent the application to define it's own global error handler, and even try to handle errors from this module in a different way.

We do have a global error handler in the app by the way: https://github.com/feedhenry-raincatcher/raincatcher-core/blob/master/demo/server/src/app.ts#L66

For example, production error handlers can redirect a user to a 'oops' page, log exceptions to external storage or email, etc.

Not sure what you suggesting here. I wanted this module to be responsible for error handling as app currently tries to render html. This will throw nice json.

What will be your suggestion - throw error here ?

Ah, in order to allow the top level app to use the global error handler you add the third param and call next(err), the app might crash on uncaught errors.

The global error handler that I linked should return a formatted json just the same, but it's possible to override it at that global point.

See http://expressjs.com/en/guide/error-handling.html, more specifically:

PaidContent.find(function (err, doc) { if (err) return next(err) res.json(doc) })

I just do not wanted this module to be self sufficient and do not leak some of the elements to users top level application, but this aproach will be more aligned with express patterns.

wtrocki · 2017-08-11T14:36:21Z

demo/server/config-dev.json

@@ -1,7 +1,10 @@
 {
  "morganOptions": null,
  "logStackTraces": true,
-  "sync":{
+  "security": {
+    "apiRole": "ADMIN"


@JameelB is that admin or ADMIN ?

it's admin

coveralls · 2017-08-13T08:27:34Z

Changes Unknown when pulling 16d8bb0 on wtrocki:RAINCATCH-1145 into ** on feedhenry-raincatcher:master**.

JameelB · 2017-08-14T09:11:15Z

cloud/wfm-rest-api/README.md

+
+Module used to expose express based api for WFM objects.
+
+### WFM speficic implementations


JameelB · 2017-08-14T09:12:31Z

cloud/wfm-rest-api/README.md

+## Rest API
+
+Module provides a way to dynamically create API for different business objects.
+Created api will use simplied implementations for typical create, read, delete and update operations.


simplified

JameelB · 2017-08-14T09:12:56Z

cloud/wfm-rest-api/README.md

+
+## Rest API definitions
+
+Definitions apply to every to object exposed by this API. Placeholder `{object}` can be replaced by `workflow`, `workorder` and `result`.


remove to in every to object

JameelB · 2017-08-14T09:16:29Z

cloud/wfm-rest-api/example/README.md

+
+Mongodb running on standard port
+
+### Runining example


JameelB · 2017-08-14T09:20:08Z

cloud/wfm-rest-api/src/data-api/ApiError.ts

@@ -0,0 +1,9 @@
+
+/**
+ * Interface used to construct standarized response for api error handlers.


standardized

JameelB · 2017-08-14T09:20:12Z

cloud/wfm-rest-api/src/data-api/ApiError.ts

@@ -0,0 +1,9 @@
+
+/**
+ * Interface used to construct standarized response for api error handlers.


standardized

JameelB · 2017-08-14T09:37:16Z

cloud/wfm-rest-api/src/impl/ApiController.ts

+    getLogger().debug('Api delete method called', { params: req.params });
+
+    if (!req.params.id) {
+      const error = new ApiError(errorCodes.MISSING_ID, 'Missing id parameter');


Missing statusCode 400?

JameelB · 2017-08-14T09:37:41Z

cloud/wfm-rest-api/src/impl/ApiController.ts

+    getLogger().debug('Api update method called', { body: req.body });
+
+    if (!req.body) {
+      const error = new ApiError(errorCodes.CLIENT_ERROR, 'Missing request body');


Missing statusCode 400?

JameelB · 2017-08-14T09:38:19Z

cloud/wfm-rest-api/src/data-api/ApiError.ts

+ * Interface used to construct standarized response for api error handlers.
+ */
+export class ApiError extends Error {
+  constructor(public code: string, public message: string, statusCode: number = 500) {


code and statusCode not used?

TypeScript magic. Adding public is exposing them as fields.

may need to add public to statusCode as well?

jstaffor · 2017-08-14T10:39:17Z

cloud/wfm-rest-api/README.md

+## Rest API
+
+Module provides a way to dynamically create API for different business objects.
+Created api will use simplied implementations for typical create, read, delete and update operations.


Possibly better to say
"create, read, update and delete" - think of CRUD

jstaffor · 2017-08-14T10:40:54Z

cloud/wfm-rest-api/README.md

+
+Module provides a way to dynamically create API for different business objects.
+Created api will use simplied implementations for typical create, read, delete and update operations.
+It's not recomended to use it outside WFM framework to store user related objects. Please use mongodb driver directly.


What does "it" refer to in "to use it" - the rest api?

outside "the" WFM framework

jstaffor · 2017-08-14T10:43:43Z

cloud/wfm-rest-api/README.md

+}
+```
+
+> **Note:** If you apply security middleware additional `401` and `403` statuses may be returned


Suggestion: "If you apply middleware security, additional "

jstaffor · 2017-08-14T10:44:27Z

cloud/wfm-rest-api/example/README.md

+
+    curl http://localhost:3000/api/workorders
+
+Note: This example assumes that you have run demo application that populated your database.


...run "the" demo application...

coveralls · 2017-08-14T10:59:04Z

Changes Unknown when pulling 8efbf0c on wtrocki:RAINCATCH-1145 into ** on feedhenry-raincatcher:master**.

coveralls · 2017-08-14T13:45:24Z

Changes Unknown when pulling 112d2ca on wtrocki:RAINCATCH-1145 into ** on feedhenry-raincatcher:master**.

paolobueno · 2017-08-14T14:28:48Z

cloud/wfm-rest-api/src/ApiConfig.ts

+  /** Used to create workflow route */
+  workflowApiName?: string;
+  /** Used to create result route */
+  resultApiName?: string;


I think we can get rid of the Used tos here, and more detail for the client developer: Root path for the Result http endpoints.

austincunningham

verified

coveralls · 2017-08-14T14:37:08Z

Changes Unknown when pulling 6388fe3 on wtrocki:RAINCATCH-1145 into ** on feedhenry-raincatcher:master**.

coveralls · 2017-08-14T14:37:08Z

Changes Unknown when pulling 6388fe3 on wtrocki:RAINCATCH-1145 into ** on feedhenry-raincatcher:master**.

wtrocki requested review from austincunningham, JameelB, paolobueno and witmicko as code owners August 8, 2017 16:32

paolobueno reviewed Aug 9, 2017

View reviewed changes

wtrocki commented Aug 10, 2017

View reviewed changes

wtrocki changed the title ~~WIP WFM api~~ WFM api framework (ex-webapi) Aug 10, 2017

paolobueno reviewed Aug 10, 2017

View reviewed changes

feedhenry-raincatcher deleted a comment from coveralls Aug 10, 2017

wtrocki force-pushed the RAINCATCH-1145 branch 2 times, most recently from d691630 to 8cfaf10 Compare August 10, 2017 21:23

wtrocki changed the title ~~WFM api framework (ex-webapi)~~ WFM api framework - HTTP RESTfull api for WFM (ex-webapi) Aug 11, 2017

feedhenry-raincatcher deleted a comment from paolobueno Aug 11, 2017

wtrocki commented Aug 11, 2017

View reviewed changes

wtrocki force-pushed the RAINCATCH-1145 branch from ab13cee to de0fba1 Compare August 11, 2017 17:20

feedhenry-raincatcher deleted a comment from coveralls Aug 11, 2017

paolobueno mentioned this pull request Aug 12, 2017

Suggestions for raincatch 1145 wtrocki/raincatcher-core#1

Merged

wtrocki added 3 commits August 13, 2017 08:11

WIP WFM api

dd2980e

Remove db reference

218695e

Moving code to module

a4c72e3

paolobueno added 6 commits August 13, 2017 09:12

Add some types to tests

b7394ef

Fix count param

80cf854

Refactor handlers

93af31d

Add id generation on create

0dcb392

Refactor router mutation to router creation

ccac162

Use chai-as-promised in tests, fix tests somewhat

62a30c1

feedhenry-raincatcher deleted a comment from coveralls Aug 14, 2017

JameelB reviewed Aug 14, 2017

View reviewed changes

jstaffor reviewed Aug 14, 2017

View reviewed changes

wtrocki force-pushed the RAINCATCH-1145 branch from 16d8bb0 to 8efbf0c Compare August 14, 2017 10:54

wtrocki mentioned this pull request Aug 14, 2017

Adding cloud auth for sync #82

Merged

1 task

wtrocki force-pushed the RAINCATCH-1145 branch from 8efbf0c to 112d2ca Compare August 14, 2017 13:42

feedhenry-raincatcher deleted a comment from coveralls Aug 14, 2017

Minor improvements in docs

4ed0ae0

paolobueno reviewed Aug 14, 2017

View reviewed changes

Rename config apidocs

6388fe3

wtrocki force-pushed the RAINCATCH-1145 branch from 112d2ca to 6388fe3 Compare August 14, 2017 14:32

austincunningham approved these changes Aug 14, 2017

View reviewed changes

paolobueno approved these changes Aug 14, 2017

View reviewed changes

wtrocki merged commit f3029bb into feedhenry-raincatcher:master Aug 14, 2017

wtrocki deleted the RAINCATCH-1145 branch August 14, 2017 14:44


		Module used to expose express based api for WFM objects.

		### WFM speficic implementations


		## Rest API definitions

		Definitions apply to every to object exposed by this API. Placeholder `{object}` can be replaced by `workflow`, `workorder` and `result`.

		@@ -0,0 +1,9 @@

		/**
		* Interface used to construct standarized response for api error handlers.


		curl http://localhost:3000/api/workorders

		Note: This example assumes that you have run demo application that populated your database.

WFM api framework - HTTP RESTfull api for WFM (ex-webapi) #75

WFM api framework - HTTP RESTfull api for WFM (ex-webapi) #75

Conversation

wtrocki commented Aug 8, 2017 • edited

Motivation

Notes

Progress

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

wtrocki Aug 10, 2017 • edited

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

coveralls commented Aug 13, 2017 • edited

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

coveralls commented Aug 14, 2017 • edited

coveralls commented Aug 14, 2017

Choose a reason for hiding this comment

austincunningham left a comment

Choose a reason for hiding this comment

coveralls commented Aug 14, 2017

coveralls commented Aug 14, 2017

wtrocki commented Aug 8, 2017 •

edited

wtrocki Aug 10, 2017 •

edited

coveralls commented Aug 13, 2017 •

edited

coveralls commented Aug 14, 2017 •

edited