Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Mar 15, 2023
+102 −22
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 64 additions & 2 deletions modules/data/spec/dataservices/default-data.service.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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';

Expand All @@ -17,6 +22,8 @@ import {
HttpUrlGenerator,
DefaultDataServiceConfig,
DataServiceError,
HttpMethods,
QueryParams,
} from '../../';
import { HttpOptions } from '../../src/dataservices/interfaces';

Expand All @@ -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;
Expand All @@ -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(() => {
Expand All @@ -52,6 +89,11 @@ describe('DefaultDataService', () => {
});

service = new DefaultDataService('Hero', httpClient, httpUrlGenerator);
customService = new CustomDataService<Hero>(
'Hero',
httpClient,
httpUrlGenerator
);
});

afterEach(() => {
Expand Down Expand Up @@ -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);
Expand Down
58 changes: 38 additions & 20 deletions modules/data/src/dataservices/default-data.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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(
Expand Down Expand Up @@ -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(
Expand All @@ -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,
Expand All @@ -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 = {
Expand All @@ -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));
}
Expand All @@ -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));
}
Expand Down