From b301ff687b7e6f30bc3b426071460cce8fec9fed Mon Sep 17 00:00:00 2001
From: Ward Bell <wardbell@hotmail.com>
Date: Wed, 21 Nov 2018 03:19:06 -0800
Subject: [PATCH] refactor: DataServiceError.message and more (beta.3) (#204)

* refactor: DataServiceError for better message (beta.3)
* fix: `EntityDispatcherBase.upsert` action (should upsert not add) (beta.3)
closes #201
---
 lib/CHANGELOG.md                              |  9 +++
 lib/package.json                              |  2 +-
 .../dataservices/data-service-error.spec.ts   | 32 +++++++++++
 lib/src/dataservices/data-service-error.ts    | 25 +++++++-
 lib/src/dispatchers/entity-dispatcher-base.ts | 57 ++++++++++++++-----
 .../entity-collection-service-base.ts         | 17 ++++--
 6 files changed, 122 insertions(+), 20 deletions(-)
 create mode 100644 lib/src/dataservices/data-service-error.spec.ts

diff --git a/lib/CHANGELOG.md b/lib/CHANGELOG.md
index 31c170c9..5409795a 100644
--- a/lib/CHANGELOG.md
+++ b/lib/CHANGELOG.md
@@ -22,6 +22,15 @@ In other words, it is safer to have something like the following in your `packag
 as this will keep you from installing `6.1.x`.
 
 <hr>
+<a id="6.1.0-beta.3"></a>
+
+# 6.1.0-beta.3 (2018-11-20)
+
+No breaking changes
+
+* DataServiceError does better job of creating a useful message from the error passed to its ctor.
+* Fix `EntityDispatcherBase.upsert` action (should upsert not add) issue #201.
+
 <a id="6.1.0-beta.2"></a>
 
 # 6.1.0-beta.2 (2018-10-22)
diff --git a/lib/package.json b/lib/package.json
index 6fea477a..1bba9f62 100644
--- a/lib/package.json
+++ b/lib/package.json
@@ -1,6 +1,6 @@
 {
   "name": "ngrx-data",
-  "version": "6.1.0-beta.2",
+  "version": "6.1.0-beta.3",
   "repository": "https://github.com/johnpapa/angular-ngrx-data.git",
   "license": "MIT",
   "peerDependencies": {
diff --git a/lib/src/dataservices/data-service-error.spec.ts b/lib/src/dataservices/data-service-error.spec.ts
new file mode 100644
index 00000000..83bfa21c
--- /dev/null
+++ b/lib/src/dataservices/data-service-error.spec.ts
@@ -0,0 +1,32 @@
+import { HttpErrorResponse } from '@angular/common/http';
+import { DataServiceError } from './data-service-error';
+import { RequestData } from './interfaces';
+
+describe('DataServiceError', () => {
+  describe('#message', () => {
+    it('should define message when ctor error is string', () => {
+      const expected = 'The error';
+      const dse = new DataServiceError(expected, null);
+      expect(dse.message).toBe(expected);
+    });
+
+    it('should define message when ctor error is new Error("message")', () => {
+      const expected = 'The error';
+      const dse = new DataServiceError(new Error(expected), null);
+      expect(dse.message).toBe(expected);
+    });
+
+    it('should define message when ctor error is typical HttpResponseError', () => {
+      const expected = 'The error';
+      const body = expected; // server error is typically in the body of the server response
+      const httpErr = new HttpErrorResponse({
+        status: 400,
+        statusText: 'Bad Request',
+        url: 'http://foo.com/bad',
+        error: body
+      });
+      const dse = new DataServiceError(httpErr, null);
+      expect(dse.message).toBe(expected);
+    });
+  });
+});
diff --git a/lib/src/dataservices/data-service-error.ts b/lib/src/dataservices/data-service-error.ts
index fab1a641..4109ae90 100644
--- a/lib/src/dataservices/data-service-error.ts
+++ b/lib/src/dataservices/data-service-error.ts
@@ -4,7 +4,7 @@ import { RequestData } from './interfaces';
 /**
  * Error from a DataService
  * The source error either comes from a failed HTTP response or was thrown within the service.
- * @param error the HttpResponse error or the error thrown by the service
+ * @param error the HttpErrorResponse or the error thrown by the service
  * @param requestData the HTTP request information such as the method and the url.
  */
 // If extend from Error, `dse instanceof DataServiceError` returns false
@@ -13,10 +13,31 @@ export class DataServiceError {
   message: string;
 
   constructor(public error: any, public requestData: RequestData) {
-    this.message = (error.error && error.error.message) || (error.message || (error.body && error.body.error) || error).toString();
+    this.message = typeof error === 'string' ? error : extractMessage(error);
   }
 }
 
+// Many ways the error can be shaped. These are the ways we recognize.
+function extractMessage(sourceError: any) {
+  const { error, body, message } = sourceError;
+  let errMessage: string;
+  if (error) {
+    // prefer HttpErrorResponse.error to its message property
+    errMessage = typeof error === 'string' ? error : error.message;
+  } else if (message) {
+    errMessage = message;
+  } else if (body) {
+    // try the body if no error or message property
+    errMessage = typeof body === 'string' ? body : body.error;
+  }
+
+  return typeof errMessage === 'string'
+    ? errMessage
+    : errMessage
+      ? JSON.stringify(errMessage)
+      : null;
+}
+
 /** Payload for an EntityAction data service error such as QUERY_ALL_ERROR */
 export interface EntityActionDataServiceError {
   error: DataServiceError;
diff --git a/lib/src/dispatchers/entity-dispatcher-base.ts b/lib/src/dispatchers/entity-dispatcher-base.ts
index 616c6b93..2f51922c 100644
--- a/lib/src/dispatchers/entity-dispatcher-base.ts
+++ b/lib/src/dispatchers/entity-dispatcher-base.ts
@@ -60,7 +60,10 @@ export class EntityDispatcherBase<T> implements EntityDispatcher<T> {
     this.guard = new EntityActionGuard(entityName, selectId);
     this.toUpdate = toUpdateFactory<T>(selectId);
 
-    const collectionSelector = createSelector(entityCacheSelector, cache => cache[entityName] as EntityCollection<T>);
+    const collectionSelector = createSelector(
+      entityCacheSelector,
+      cache => cache[entityName] as EntityCollection<T>
+    );
     this.entityCollection$ = store.select(collectionSelector);
   }
 
@@ -71,7 +74,11 @@ export class EntityDispatcherBase<T> implements EntityDispatcher<T> {
    * @param [options] additional options
    * @returns the EntityAction
    */
-  createEntityAction<P = any>(entityOp: EntityOp, data?: P, options?: EntityActionOptions): EntityAction<P> {
+  createEntityAction<P = any>(
+    entityOp: EntityOp,
+    data?: P,
+    options?: EntityActionOptions
+  ): EntityAction<P> {
     return this.entityActionFactory.create({
       entityName: this.entityName,
       entityOp,
@@ -88,7 +95,11 @@ export class EntityDispatcherBase<T> implements EntityDispatcher<T> {
    * @param [options] additional options
    * @returns the dispatched EntityAction
    */
-  createAndDispatch<P = any>(op: EntityOp, data?: P, options?: EntityActionOptions): EntityAction<P> {
+  createAndDispatch<P = any>(
+    op: EntityOp,
+    data?: P,
+    options?: EntityActionOptions
+  ): EntityAction<P> {
     const action = this.createEntityAction(op, data, options);
     this.dispatch(action);
     return action;
@@ -159,12 +170,18 @@ export class EntityDispatcherBase<T> implements EntityDispatcher<T> {
    */
   delete(key: number | string, options?: EntityActionOptions): Observable<number | string>;
   delete(arg: number | string | T, options?: EntityActionOptions): Observable<number | string> {
-    options = this.setSaveEntityActionOptions(options, this.defaultDispatcherOptions.optimisticDelete);
+    options = this.setSaveEntityActionOptions(
+      options,
+      this.defaultDispatcherOptions.optimisticDelete
+    );
     const key = this.getKey(arg);
     const action = this.createEntityAction(EntityOp.SAVE_DELETE_ONE, key, options);
     this.guard.mustBeKey(action);
     this.dispatch(action);
-    return this.getResponseData$<number | string>(options.correlationId).pipe(map(() => key), shareReplay(1));
+    return this.getResponseData$<number | string>(options.correlationId).pipe(
+      map(() => key),
+      shareReplay(1)
+    );
   }
 
   /**
@@ -278,7 +295,10 @@ export class EntityDispatcherBase<T> implements EntityDispatcher<T> {
     // update entity might be a partial of T but must at least have its key.
     // pass the Update<T> structure as the payload
     const update: Update<T> = this.toUpdate(entity);
-    options = this.setSaveEntityActionOptions(options, this.defaultDispatcherOptions.optimisticUpdate);
+    options = this.setSaveEntityActionOptions(
+      options,
+      this.defaultDispatcherOptions.optimisticUpdate
+    );
     const action = this.createEntityAction(EntityOp.SAVE_UPDATE_ONE, update, options);
     if (options.isOptimistic) {
       this.guard.mustBeEntity(action);
@@ -304,8 +324,11 @@ export class EntityDispatcherBase<T> implements EntityDispatcher<T> {
    * after server reports successful save or the save error.
    */
   upsert(entity: T, options?: EntityActionOptions): Observable<T> {
-    options = this.setSaveEntityActionOptions(options, this.defaultDispatcherOptions.optimisticUpsert);
-    const action = this.createEntityAction(EntityOp.SAVE_ADD_ONE, entity, options);
+    options = this.setSaveEntityActionOptions(
+      options,
+      this.defaultDispatcherOptions.optimisticUpsert
+    );
+    const action = this.createEntityAction(EntityOp.SAVE_UPSERT_ONE, entity, options);
     if (options.isOptimistic) {
       this.guard.mustBeEntity(action);
     }
@@ -496,7 +519,9 @@ export class EntityDispatcherBase<T> implements EntityDispatcher<T> {
         return (
           entityName === this.entityName &&
           correlationId === crid &&
-          (entityOp.endsWith(OP_SUCCESS) || entityOp.endsWith(OP_ERROR) || entityOp === EntityOp.CANCEL_PERSIST)
+          (entityOp.endsWith(OP_SUCCESS) ||
+            entityOp.endsWith(OP_ERROR) ||
+            entityOp === EntityOp.CANCEL_PERSIST)
         );
       }),
       take(1),
@@ -513,14 +538,20 @@ export class EntityDispatcherBase<T> implements EntityDispatcher<T> {
 
   private setQueryEntityActionOptions(options: EntityActionOptions): EntityActionOptions {
     options = options || {};
-    const correlationId = options.correlationId == null ? this.correlationIdGenerator.next() : options.correlationId;
+    const correlationId =
+      options.correlationId == null ? this.correlationIdGenerator.next() : options.correlationId;
     return { ...options, correlationId };
   }
 
-  private setSaveEntityActionOptions(options: EntityActionOptions, defaultOptimism: boolean): EntityActionOptions {
+  private setSaveEntityActionOptions(
+    options: EntityActionOptions,
+    defaultOptimism: boolean
+  ): EntityActionOptions {
     options = options || {};
-    const correlationId = options.correlationId == null ? this.correlationIdGenerator.next() : options.correlationId;
-    const isOptimistic = options.isOptimistic == null ? defaultOptimism || false : options.isOptimistic === true;
+    const correlationId =
+      options.correlationId == null ? this.correlationIdGenerator.next() : options.correlationId;
+    const isOptimistic =
+      options.isOptimistic == null ? defaultOptimism || false : options.isOptimistic === true;
     return { ...options, correlationId, isOptimistic };
   }
   // #endregion private helpers
diff --git a/lib/src/entity-services/entity-collection-service-base.ts b/lib/src/entity-services/entity-collection-service-base.ts
index e9e7da50..823d0c6d 100644
--- a/lib/src/entity-services/entity-collection-service-base.ts
+++ b/lib/src/entity-services/entity-collection-service-base.ts
@@ -25,7 +25,8 @@ import { QueryParams } from '../dataservices/interfaces';
  * @param EntityCollectionServiceElements The ingredients for this service
  * as a source of supporting services for creating an EntityCollectionService<T> instance.
  */
-export class EntityCollectionServiceBase<T, S$ extends EntitySelectors$<T> = EntitySelectors$<T>> implements EntityCollectionService<T> {
+export class EntityCollectionServiceBase<T, S$ extends EntitySelectors$<T> = EntitySelectors$<T>>
+  implements EntityCollectionService<T> {
   /** Dispatcher of EntityCommands (EntityActions) */
   readonly dispatcher: EntityDispatcher<T>;
 
@@ -73,7 +74,11 @@ export class EntityCollectionServiceBase<T, S$ extends EntitySelectors$<T> = Ent
    * @param [options] additional options
    * @returns the EntityAction
    */
-  createEntityAction<P = any>(op: EntityOp, data?: P, options?: EntityActionOptions): EntityAction<P> {
+  createEntityAction<P = any>(
+    op: EntityOp,
+    data?: P,
+    options?: EntityActionOptions
+  ): EntityAction<P> {
     return this.dispatcher.createEntityAction(op, data, options);
   }
 
@@ -85,7 +90,11 @@ export class EntityCollectionServiceBase<T, S$ extends EntitySelectors$<T> = Ent
    * @param [options] additional options
    * @returns the dispatched EntityAction
    */
-  createAndDispatch<P = any>(op: EntityOp, data?: P, options?: EntityActionOptions): EntityAction<P> {
+  createAndDispatch<P = any>(
+    op: EntityOp,
+    data?: P,
+    options?: EntityActionOptions
+  ): EntityAction<P> {
     return this.dispatcher.createAndDispatch(op, data, options);
   }
 
@@ -180,7 +189,7 @@ export class EntityCollectionServiceBase<T, S$ extends EntitySelectors$<T> = Ent
    * merge it into the cached collection.
    * @param key The primary key of the entity to get.
    * @param [options] options that influence merge behavior
-   * @returns Observable of the queried entities that are in the collection
+   * @returns Observable of the queried entity that is in the collection
    * after server reports success or the query error.
    */
   getByKey(key: any, options?: EntityActionOptions): Observable<T> {