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.

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

Provide a setting to use a different REST url during SSR execution #3358

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
9 changes: 6 additions & 3 deletions server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,9 @@ let anonymousCache: LRU<string, any>;
// extend environment with app config for server
extendEnvironmentWithAppConfig(environment, appConfig);

// The REST server base URL
const REST_BASE_URL = environment.rest.ssrBaseUrl || environment.rest.baseUrl;

// The Express app is exported so that it can be used by serverless Functions.
export function app() {

Expand Down Expand Up @@ -156,7 +159,7 @@ export function app() {
* Proxy the sitemaps
*/
router.use('/sitemap**', createProxyMiddleware({
target: `${environment.rest.baseUrl}/sitemaps`,
target: `${REST_BASE_URL}/sitemaps`,
pathRewrite: path => path.replace(environment.ui.nameSpace, '/'),
changeOrigin: true,
}));
Expand All @@ -165,7 +168,7 @@ export function app() {
* Proxy the linksets
*/
router.use('/signposting**', createProxyMiddleware({
target: `${environment.rest.baseUrl}`,
target: `${REST_BASE_URL}`,
pathRewrite: path => path.replace(environment.ui.nameSpace, '/'),
changeOrigin: true,
}));
Expand Down Expand Up @@ -623,7 +626,7 @@ function start() {
* The callback function to serve health check requests
*/
function healthCheck(req, res) {
const baseUrl = `${environment.rest.baseUrl}${environment.actuators.endpointPath}`;
const baseUrl = `${REST_BASE_URL}${environment.actuators.endpointPath}`;
axios.get(baseUrl)
.then((response) => {
res.status(response.status).send(response.data);
Expand Down
6 changes: 6 additions & 0 deletions src/app/app.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ import {
} from './app-routes';
import { BROWSE_BY_DECORATOR_MAP } from './browse-by/browse-by-switcher/browse-by-decorator';
import { AuthInterceptor } from './core/auth/auth.interceptor';
import { DspaceRestInterceptor } from './core/dspace-rest/dspace-rest.interceptor';
import { LocaleInterceptor } from './core/locale/locale.interceptor';
import { LogInterceptor } from './core/log/log.interceptor';
import {
Expand Down Expand Up @@ -148,6 +149,11 @@ export const commonAppConfig: ApplicationConfig = {
useClass: LogInterceptor,
multi: true,
},
{
provide: HTTP_INTERCEPTORS,
useClass: DspaceRestInterceptor,
multi: true,
},
// register the dynamic matcher used by form. MUST be provided by the app module
...DYNAMIC_MATCHER_PROVIDERS,
provideCore(),
Expand Down
194 changes: 194 additions & 0 deletions src/app/core/dspace-rest/dspace-rest.interceptor.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,194 @@
import {
HTTP_INTERCEPTORS,
HttpClient,
} from '@angular/common/http';
import {
HttpClientTestingModule,
HttpTestingController,
} from '@angular/common/http/testing';
import { PLATFORM_ID } from '@angular/core';
import { TestBed } from '@angular/core/testing';

import {
APP_CONFIG,
AppConfig,
} from '../../../config/app-config.interface';
import { DspaceRestInterceptor } from './dspace-rest.interceptor';
import { DspaceRestService } from './dspace-rest.service';

describe('DspaceRestInterceptor', () => {
let httpMock: HttpTestingController;
let httpClient: HttpClient;
const appConfig: Partial<AppConfig> = {
rest: {
ssl: false,
host: 'localhost',
port: 8080,
nameSpace: '/server',
baseUrl: 'http://api.example.com/server',
},
};
const appConfigWithSSR: Partial<AppConfig> = {
rest: {
ssl: false,
host: 'localhost',
port: 8080,
nameSpace: '/server',
baseUrl: 'http://api.example.com/server',
ssrBaseUrl: 'http://ssr.example.com/server',
},
};

describe('When SSR base URL is not set ', () => {
describe('and it\'s in the browser', () => {
beforeEach(() => {
TestBed.configureTestingModule({
imports: [HttpClientTestingModule],
providers: [
DspaceRestService,
{
provide: HTTP_INTERCEPTORS,
useClass: DspaceRestInterceptor,
multi: true,
},
{ provide: APP_CONFIG, useValue: appConfig },
{ provide: PLATFORM_ID, useValue: 'browser' },
],
});

httpMock = TestBed.inject(HttpTestingController);
httpClient = TestBed.inject(HttpClient);
});

it('should not modify the request', () => {
const url = 'http://api.example.com/server/items';
httpClient.get(url).subscribe((response) => {
expect(response).toBeTruthy();
});

const req = httpMock.expectOne(url);
expect(req.request.url).toBe(url);
req.flush({});
httpMock.verify();
});
});

describe('and it\'s in SSR mode', () => {
beforeEach(() => {
TestBed.configureTestingModule({
imports: [HttpClientTestingModule],
providers: [
DspaceRestService,
{
provide: HTTP_INTERCEPTORS,
useClass: DspaceRestInterceptor,
multi: true,
},
{ provide: APP_CONFIG, useValue: appConfig },
{ provide: PLATFORM_ID, useValue: 'server' },
],
});

httpMock = TestBed.inject(HttpTestingController);
httpClient = TestBed.inject(HttpClient);
});

it('should not replace the base URL', () => {
const url = 'http://api.example.com/server/items';

httpClient.get(url).subscribe((response) => {
expect(response).toBeTruthy();
});

const req = httpMock.expectOne(url);
expect(req.request.url).toBe(url);
req.flush({});
httpMock.verify();
});
});
});

describe('When SSR base URL is set ', () => {
describe('and it\'s in the browser', () => {
beforeEach(() => {
TestBed.configureTestingModule({
imports: [HttpClientTestingModule],
providers: [
DspaceRestService,
{
provide: HTTP_INTERCEPTORS,
useClass: DspaceRestInterceptor,
multi: true,
},
{ provide: APP_CONFIG, useValue: appConfigWithSSR },
{ provide: PLATFORM_ID, useValue: 'browser' },
],
});

httpMock = TestBed.inject(HttpTestingController);
httpClient = TestBed.inject(HttpClient);
});

it('should not modify the request', () => {
const url = 'http://api.example.com/server/items';
httpClient.get(url).subscribe((response) => {
expect(response).toBeTruthy();
});

const req = httpMock.expectOne(url);
expect(req.request.url).toBe(url);
req.flush({});
httpMock.verify();
});
});

describe('and it\'s in SSR mode', () => {
beforeEach(() => {
TestBed.configureTestingModule({
imports: [HttpClientTestingModule],
providers: [
DspaceRestService,
{
provide: HTTP_INTERCEPTORS,
useClass: DspaceRestInterceptor,
multi: true,
},
{ provide: APP_CONFIG, useValue: appConfigWithSSR },
{ provide: PLATFORM_ID, useValue: 'server' },
],
});

httpMock = TestBed.inject(HttpTestingController);
httpClient = TestBed.inject(HttpClient);
});

it('should replace the base URL', () => {
const url = 'http://api.example.com/server/items';
const ssrBaseUrl = appConfigWithSSR.rest.ssrBaseUrl;

httpClient.get(url).subscribe((response) => {
expect(response).toBeTruthy();
});

const req = httpMock.expectOne(ssrBaseUrl + '/items');
expect(req.request.url).toBe(ssrBaseUrl + '/items');
req.flush({});
httpMock.verify();
});

it('should not replace any query param containing the base URL', () => {
const url = 'http://api.example.com/server/items?url=http://api.example.com/server/item/1';
const ssrBaseUrl = appConfigWithSSR.rest.ssrBaseUrl;

httpClient.get(url).subscribe((response) => {
expect(response).toBeTruthy();
});

const req = httpMock.expectOne(ssrBaseUrl + '/items?url=http://api.example.com/server/item/1');
expect(req.request.url).toBe(ssrBaseUrl + '/items?url=http://api.example.com/server/item/1');
req.flush({});
httpMock.verify();
});
});
});
});
52 changes: 52 additions & 0 deletions src/app/core/dspace-rest/dspace-rest.interceptor.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
import { isPlatformBrowser } from '@angular/common';
import {
HttpEvent,
HttpHandler,
HttpInterceptor,
HttpRequest,
} from '@angular/common/http';
import {
Inject,
Injectable,
PLATFORM_ID,
} from '@angular/core';
import { Observable } from 'rxjs';

import {
APP_CONFIG,
AppConfig,
} from '../../../config/app-config.interface';
import { isEmpty } from '../../shared/empty.util';

@Injectable()
/**
* This Interceptor is used to use the configured base URL for the request made during SSR execution
*/
export class DspaceRestInterceptor implements HttpInterceptor {

/**
* Contains the configured application base URL
* @protected
*/
protected baseUrl: string;
protected ssrBaseUrl: string;

constructor(
@Inject(APP_CONFIG) protected appConfig: AppConfig,
@Inject(PLATFORM_ID) private platformId: string,
) {
this.baseUrl = this.appConfig.rest.baseUrl;
this.ssrBaseUrl = this.appConfig.rest.ssrBaseUrl;
}

intercept(request: HttpRequest<unknown>, next: HttpHandler): Observable<HttpEvent<unknown>> {
if (isPlatformBrowser(this.platformId) || isEmpty(this.ssrBaseUrl) || this.baseUrl === this.ssrBaseUrl) {
return next.handle(request);
}

// Different SSR Base URL specified so replace it in the current request url
const url = request.url.replace(this.baseUrl, this.ssrBaseUrl);
const newRequest: HttpRequest<any> = request.clone({ url });
return next.handle(newRequest);
}
}
3 changes: 2 additions & 1 deletion src/app/core/services/server-hard-redirect.service.spec.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1,14 @@
import { TestBed } from '@angular/core/testing';

import { environment } from '../../../environments/environment.test';
import { ServerHardRedirectService } from './server-hard-redirect.service';

describe('ServerHardRedirectService', () => {

const mockRequest = jasmine.createSpyObj(['get']);
const mockResponse = jasmine.createSpyObj(['redirect', 'end']);

const service: ServerHardRedirectService = new ServerHardRedirectService(mockRequest, mockResponse);
const service: ServerHardRedirectService = new ServerHardRedirectService(environment, mockRequest, mockResponse);
const origin = 'https://test-host.com:4000';

beforeEach(() => {
Expand Down
19 changes: 15 additions & 4 deletions src/app/core/services/server-hard-redirect.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,15 @@
Response,
} from 'express';

import {
APP_CONFIG,
AppConfig,
} from '../../../config/app-config.interface';
import {
REQUEST,
RESPONSE,
} from '../../../express.tokens';
import { isNotEmpty } from '../../shared/empty.util';
import { HardRedirectService } from './hard-redirect.service';

/**
Expand All @@ -20,6 +25,7 @@
export class ServerHardRedirectService extends HardRedirectService {

constructor(
@Inject(APP_CONFIG) protected appConfig: AppConfig,
@Inject(REQUEST) protected req: Request,
@Inject(RESPONSE) protected res: Response,
) {
Expand All @@ -35,17 +41,22 @@
* optional HTTP status code to use for redirect (default = 302, which is a temporary redirect)
*/
redirect(url: string, statusCode?: number) {

if (url === this.req.url) {
return;
}

let redirectUrl = url;
// If redirect url contains SSR base url then replace with public base url
if (isNotEmpty(this.appConfig.rest.ssrBaseUrl) && this.appConfig.rest.baseUrl !== this.appConfig.rest.ssrBaseUrl) {
redirectUrl = url.replace(this.appConfig.rest.ssrBaseUrl, this.appConfig.rest.baseUrl);

Check warning on line 51 in src/app/core/services/server-hard-redirect.service.ts

View check run for this annotation

Codecov / codecov/patch

src/app/core/services/server-hard-redirect.service.ts#L51

Added line #L51 was not covered by tests
Copy link
Member

Choose a reason for hiding this comment

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

Can we add a basic test to server-hard-redirect.service.spec.ts to prove this replacement is working properly? This code appears to be missing specs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

}

if (this.res.finished) {
const req: any = this.req;
req._r_count = (req._r_count || 0) + 1;

console.warn('Attempted to redirect on a finished response. From',
this.req.url, 'to', url);
this.req.url, 'to', redirectUrl);

if (req._r_count > 10) {
console.error('Detected a redirection loop. killing the nodejs process');
Expand All @@ -59,9 +70,9 @@
status = 302;
}

console.log(`Redirecting from ${this.req.url} to ${url} with ${status}`);
console.info(`Redirecting from ${this.req.url} to ${redirectUrl} with ${status}`);

this.res.redirect(status, url);
this.res.redirect(status, redirectUrl);
this.res.end();
// I haven't found a way to correctly stop Angular rendering.
// So we just let it end its work, though we have already closed
Expand Down
Loading
Loading