fix(data): correctly handle HttpOptions when provided (#3795)

Closes #3794

Author: cam-m

modules/data/spec/dataservices/default-data.service.spec.ts

@@ -1,12 +1,17 @@
 import { TestBed } from '@angular/core/testing';
 
-import { HttpClient } from '@angular/common/http';
+import {
+  HttpClient,
+  HttpContext,
+  HttpContextToken,
+  HttpParams,
+} from '@angular/common/http';
 import {
   HttpClientTestingModule,
   HttpTestingController,
 } from '@angular/common/http/testing';
 
-import { of } from 'rxjs';
+import { Observable, of } from 'rxjs';
 
 import { Update } from '@ngrx/entity';
 
@@ -17,6 +22,8 @@ import {
   HttpUrlGenerator,
   DefaultDataServiceConfig,
   DataServiceError,
+  HttpMethods,
+  QueryParams,
 } from '../../';
 import { HttpOptions } from '../../src/dataservices/interfaces';
 
@@ -26,6 +33,35 @@ class Hero {
   version?: number;
 }
 
+export const IS_CACHE_ENABLED = new HttpContextToken<boolean>(() => false);
+/**
+ * This is to test that we don't inadvertently delete http options when users override methods on DefaultDataService.
+ */
+class CustomDataService<T> extends DefaultDataService<T> {
+  override getWithQuery(
+    queryParams: QueryParams | string | undefined,
+    options?: HttpOptions
+  ): Observable<T[]> {
+    const qParams =
+      typeof queryParams === 'string'
+        ? { fromString: queryParams }
+        : { fromObject: queryParams };
+    const params = new HttpParams(qParams);
+
+    return this.execute(
+      'GET',
+      this.entitiesUrl,
+      undefined,
+      {
+        params,
+        observe: 'body',
+        context: new HttpContext().set(IS_CACHE_ENABLED, true),
+      },
+      options
+    );
+  }
+}
+
 ////////  Tests  /////////////
 describe('DefaultDataService', () => {
   let httpClient: HttpClient;
@@ -34,6 +70,7 @@ describe('DefaultDataService', () => {
   const heroesUrl = 'api/heroes/';
   let httpUrlGenerator: HttpUrlGenerator;
   let service: DefaultDataService<Hero>;
+  let customService: CustomDataService<Hero>;
 
   //// HttpClient testing boilerplate
   beforeEach(() => {
@@ -52,6 +89,11 @@ describe('DefaultDataService', () => {
     });
 
     service = new DefaultDataService('Hero', httpClient, httpUrlGenerator);
+    customService = new CustomDataService<Hero>(
+      'Hero',
+      httpClient,
+      httpUrlGenerator
+    );
   });
 
   afterEach(() => {
@@ -327,6 +369,26 @@ describe('DefaultDataService', () => {
       req.flush(expectedHeroes);
     });
 
+    it('should support custom data services that provide their own http options to execute', (done) => {
+      const httpOptions: HttpOptions = {
+        httpParams: { fromString: 'name=B' },
+      } as HttpOptions;
+
+      customService.getWithQuery(undefined, httpOptions).subscribe((heroes) => {
+        expect(heroes).toEqual(expectedHeroes);
+        done();
+      }, fail);
+
+      // HeroService should have made one request to GET heroes
+      // from expected URL with query params
+      const req = httpTestingController.expectOne(heroesUrl + '?name=B');
+      expect(req.request.method).toEqual('GET');
+      expect(req.request.context.get(IS_CACHE_ENABLED)).toEqual(true);
+
+      // Respond with the mock heroes
+      req.flush(expectedHeroes);
+    });
+
     it('should be OK returning no heroes', (done) => {
       service.getWithQuery({ name: 'B' }).subscribe((heroes) => {
         expect(heroes.length).toEqual(0);

modules/data/src/dataservices/default-data.service.ts

@@ -73,7 +73,7 @@ export class DefaultDataService<T> implements EntityCollectionDataService<T> {
   add(entity: T, options?: HttpOptions): Observable<T> {
     const entityOrError =
       entity || new Error(`No "${this.entityName}" entity to add`);
-    return this.execute('POST', this.entityUrl, entityOrError, options);
+    return this.execute('POST', this.entityUrl, entityOrError, null, options);
   }
 
   delete(
@@ -84,22 +84,29 @@ export class DefaultDataService<T> implements EntityCollectionDataService<T> {
     if (key == null) {
       err = new Error(`No "${this.entityName}" key to delete`);
     }
-    return this.execute('DELETE', this.entityUrl + key, err, options).pipe(
+
+    return this.execute(
+      'DELETE',
+      this.entityUrl + key,
+      err,
+      null,
+      options
+    ).pipe(
       // forward the id of deleted entity as the result of the HTTP DELETE
       map((result) => key as number | string)
     );
   }
 
   getAll(options?: HttpOptions): Observable<T[]> {
-    return this.execute('GET', this.entitiesUrl, options);
+    return this.execute('GET', this.entitiesUrl, null, options);
   }
 
   getById(key: number | string, options?: HttpOptions): Observable<T> {
     let err: Error | undefined;
     if (key == null) {
       err = new Error(`No "${this.entityName}" key to get`);
     }
-    return this.execute('GET', this.entityUrl + key, err, options);
+    return this.execute('GET', this.entityUrl + key, err, null, options);
   }
 
   getWithQuery(
@@ -127,14 +134,20 @@ export class DefaultDataService<T> implements EntityCollectionDataService<T> {
       id == null
         ? new Error(`No "${this.entityName}" update data or id`)
         : update.changes;
-    return this.execute('PUT', this.entityUrl + id, updateOrError, options);
+    return this.execute(
+      'PUT',
+      this.entityUrl + id,
+      updateOrError,
+      null,
+      options
+    );
   }
 
   // Important! Only call if the backend service supports upserts as a POST to the target URL
   upsert(entity: T, options?: HttpOptions): Observable<T> {
     const entityOrError =
       entity || new Error(`No "${this.entityName}" entity to upsert`);
-    return this.execute('POST', this.entityUrl, entityOrError, options);
+    return this.execute('POST', this.entityUrl, entityOrError, null, options);
   }
 
   protected execute(
@@ -144,9 +157,9 @@ export class DefaultDataService<T> implements EntityCollectionDataService<T> {
     options?: any, // options or undefined/null
     httpOptions?: HttpOptions // these override any options passed via options
   ): Observable<any> {
-    let ngHttpClientOptions: any = undefined;
+    let entityActionHttpClientOptions: any = undefined;
     if (httpOptions) {
-      ngHttpClientOptions = {
+      entityActionHttpClientOptions = {
         headers: httpOptions?.httpHeaders
           ? new HttpHeaders(httpOptions?.httpHeaders)
           : undefined,
@@ -156,24 +169,29 @@ export class DefaultDataService<T> implements EntityCollectionDataService<T> {
       };
     }
 
+    // Now we may have:
+    // options: containing headers, params, or any other allowed http options already in angular's api
+    // entityActionHttpClientOptions: headers and params in angular's api
+
+    // We therefore need to merge these so that the action ones override the
+    // existing keys where applicable.
+
     // If any options have been specified, pass them to http client. Note
     // the new http options, if specified, will override any options passed
     // from the deprecated options parameter
     let mergedOptions: any = undefined;
-    if (options || ngHttpClientOptions) {
-      if (isDevMode() && options && ngHttpClientOptions) {
+    if (options || entityActionHttpClientOptions) {
+      if (isDevMode() && options && entityActionHttpClientOptions) {
         console.warn(
           '@ngrx/data: options.httpParams will be merged with queryParams when both are are provided to getWithQuery(). In the event of a conflict HttpOptions.httpParams will override queryParams`. The queryParams parameter of getWithQuery() will be removed in next major release.'
         );
       }
 
-      mergedOptions = {};
-      if (ngHttpClientOptions?.headers) {
-        mergedOptions.headers = ngHttpClientOptions?.headers;
-      }
-      if (ngHttpClientOptions?.params || options?.params) {
-        mergedOptions.params = ngHttpClientOptions?.params ?? options?.params;
-      }
+      mergedOptions = {
+        ...options,
+        headers: entityActionHttpClientOptions?.headers ?? options?.headers,
+        params: entityActionHttpClientOptions?.params ?? options?.params,
+      };
     }
 
     const req: RequestData = {
@@ -191,7 +209,7 @@ export class DefaultDataService<T> implements EntityCollectionDataService<T> {
 
     switch (method) {
       case 'DELETE': {
-        result$ = this.http.delete(url, ngHttpClientOptions);
+        result$ = this.http.delete(url, mergedOptions);
         if (this.saveDelay) {
           result$ = result$.pipe(delay(this.saveDelay));
         }
@@ -205,15 +223,15 @@ export class DefaultDataService<T> implements EntityCollectionDataService<T> {
         break;
       }
       case 'POST': {
-        result$ = this.http.post(url, data, ngHttpClientOptions);
+        result$ = this.http.post(url, data, mergedOptions);
         if (this.saveDelay) {
           result$ = result$.pipe(delay(this.saveDelay));
         }
         break;
       }
       // N.B.: It must return an Update<T>
       case 'PUT': {
-        result$ = this.http.put(url, data, ngHttpClientOptions);
+        result$ = this.http.put(url, data, mergedOptions);
         if (this.saveDelay) {
           result$ = result$.pipe(delay(this.saveDelay));
         }