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 all 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
24 changes: 24 additions & 0 deletions config/config.example.yml
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,32 @@ ui:

# Angular Server Side Rendering (SSR) settings
ssr:
# A boolean flag indicating whether the SSR configuration is enabled
# Defaults to true.
enabled: boolean;
Copy link
Member

Choose a reason for hiding this comment

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

Please remove this. While it is possible to disable SSR in this manner, I don't think we should encourage anyone to disable SSR entirely -- it will have a detrimental effect on SEO.

So, this enabled setting was purposefully excluded from config.example.yml to discourage anyone from disabling SSR entirely.


# Enable request performance profiling data collection and printing the results in the server console.
# Defaults to false.
enablePerformanceProfiler: boolean;
Copy link
Member

Choose a reason for hiding this comment

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

Please change boolean to false and remove the semicolon. This config.example.yml file is supposed to have default settings listed and be "valid" if they want to copy the config.example.yml into config.prod.yml and edit the defaults. So, this should say:

enablePerformanceProfiler: false

Also is there any risk in enabling this in production? This sounds like a debugging tool, so we may want to mention that we don't recommend enabling in production


# Whether to tell Angular to inline "critical" styles into the server-side rendered HTML.
# Determining which styles are critical is a relatively expensive operation; this option is
# disabled (false) by default to boost server performance at the expense of loading smoothness.
inlineCriticalCss: false

# Enable state transfer from the server-side application to the client-side application.
# Defaults to true.
# Note: When using an external application cache layer, it's recommended not to transfer the state to avoid caching it.
# Disabling it ensures that dynamic state information is not inadvertently cached, which can improve security and
# ensure that users always use the most up-to-date state.
transferState: boolean;
Copy link
Member

Choose a reason for hiding this comment

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

Same as above, this should have the default value listed:

transferState: true


# When a different REST base URL is used for the server-side application, the generated state contains references to
# REST resources with the internal URL configured, so it is not transferred to the client application, by default.
# Enabling this setting transfers the state to the client application and replaces internal URLs with the public
# URLs used by the client application.
replaceRestUrl: boolean;
Copy link
Member

@tdonohue tdonohue Jan 14, 2025

Choose a reason for hiding this comment

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

Same as above, this should have the default value listed:

replaceRestUrl: false

Why is this false by default? We may want to add an explanation to describe why someone may want this to be true or false. Are there any side effects to enabling it, or scenarios where you should definitely have it enabled or disabled? Currently, it's unclear to me why it is false and when users would want to set this to true.

Based on the discussion with @ybnd above , it sounds like setting this to true should decrease the number of REST API requests when a page goes through SSR? (As it shouldn't need to make duplicate requests in CSR)

(I like how for transferState above you described a scenario where you might want to set it to false instead of true. So, it'd be nice to do that here as well.)


# The REST API server settings
# NOTE: these settings define which (publicly available) REST API to use. They are usually
# 'synced' with the 'dspace.server.url' setting in your backend's local.cfg.
Expand All @@ -33,6 +54,9 @@ rest:
port: 443
# NOTE: Space is capitalized because 'namespace' is a reserved string in TypeScript
nameSpace: /server
# Provide a different REST url to be used during SSR execution. It must contain the whole url including protocol, server port and
# server namespace (uncomment to use it).
#ssrBaseUrl: http://localhost:8080/server

# Caching settings
cache:
Expand Down
14 changes: 11 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 @@ -266,6 +269,11 @@ function serverSideRender(req, res, next, sendToUser: boolean = true) {
})
.then((html) => {
if (hasValue(html)) {
// Replace REST URL with UI URL
if (environment.ssr.replaceRestUrl && REST_BASE_URL !== environment.rest.baseUrl) {
html = html.replace(new RegExp(REST_BASE_URL, 'g'), environment.rest.baseUrl);
}

// save server side rendered page to cache (if any are enabled)
saveToCache(req, html);
if (sendToUser) {
Expand Down Expand Up @@ -623,7 +631,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);
}
}
22 changes: 21 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);
let service: ServerHardRedirectService = new ServerHardRedirectService(environment, mockRequest, mockResponse);
const origin = 'https://test-host.com:4000';

beforeEach(() => {
Expand Down Expand Up @@ -68,4 +69,23 @@ describe('ServerHardRedirectService', () => {
});
});

describe('when SSR base url is set', () => {
const redirect = 'https://private-url:4000/server/api/bitstreams/uuid';
const replacedUrl = 'https://public-url/server/api/bitstreams/uuid';
const environmentWithSSRUrl: any = { ...environment, ...{ ...environment.rest, rest: {
ssrBaseUrl: 'https://private-url:4000/server',
baseUrl: 'https://public-url/server',
} } };
service = new ServerHardRedirectService(environmentWithSSRUrl, mockRequest, mockResponse);

beforeEach(() => {
service.redirect(redirect);
});

it('should perform a 302 redirect', () => {
expect(mockResponse.redirect).toHaveBeenCalledWith(302, replacedUrl);
expect(mockResponse.end).toHaveBeenCalled();
});
});

});
Loading
Loading