From 0fe33eecd1dedd56faedb43010c55a517c975f7e Mon Sep 17 00:00:00 2001 From: Tim Donohue Date: Wed, 21 Jun 2023 15:36:59 -0500 Subject: [PATCH 1/2] Implement basic 301 Redirect when SSR is used. --- .../core/data/dso-redirect.service.spec.ts | 24 +++++++++++++++++++ src/app/core/data/dso-redirect.service.ts | 21 +++++++++++++--- 2 files changed, 42 insertions(+), 3 deletions(-) diff --git a/src/app/core/data/dso-redirect.service.spec.ts b/src/app/core/data/dso-redirect.service.spec.ts index ca064b5608..64fbd94367 100644 --- a/src/app/core/data/dso-redirect.service.spec.ts +++ b/src/app/core/data/dso-redirect.service.spec.ts @@ -27,6 +27,9 @@ describe('DsoRedirectService', () => { const requestUUIDURL = `https://rest.api/rest/api/pid/find?id=${dsoUUID}`; const requestUUID = '34cfed7c-f597-49ef-9cbe-ea351f0023c2'; const objectCache = {} as ObjectCacheService; + const mockResponse = jasmine.createSpyObj(['redirect']); + const mockPlatformBrowser = 'browser'; + const mockPlatformServer = 'server'; beforeEach(() => { scheduler = getTestScheduler(); @@ -58,6 +61,8 @@ describe('DsoRedirectService', () => { objectCache, halService, router, + mockResponse, + mockPlatformBrowser // default to CSR except where explicitly SSR below ); }); @@ -141,6 +146,25 @@ describe('DsoRedirectService', () => { scheduler.flush(); expect(router.navigate).toHaveBeenCalledWith(['/communities/' + remoteData.payload.uuid]); }); + + it('should return 301 redirect when SSR is used', () => { + service = new DsoRedirectService( + requestService, + rdbService, + objectCache, + halService, + router, + mockResponse, + mockPlatformServer // explicitly SSR mode + ); + remoteData.payload.type = 'item'; + const redir = service.findByIdAndIDType(dsoHandle, IdentifierType.HANDLE); + // The framework would normally subscribe but do it here so we can test navigation. + redir.subscribe(); + scheduler.schedule(() => redir); + scheduler.flush(); + expect(mockResponse.redirect).toHaveBeenCalledWith(301, '/items/' + remoteData.payload.uuid); + }); }); describe('DataService', () => { // todo: should only test the id/uuid interpolation thingy diff --git a/src/app/core/data/dso-redirect.service.ts b/src/app/core/data/dso-redirect.service.ts index 81ce678e43..9b87590f7e 100644 --- a/src/app/core/data/dso-redirect.service.ts +++ b/src/app/core/data/dso-redirect.service.ts @@ -6,7 +6,7 @@ * http://www.dspace.org/license/ */ /* eslint-disable max-classes-per-file */ -import { Injectable } from '@angular/core'; +import { Inject, Injectable, Optional, PLATFORM_ID } from '@angular/core'; import { Router } from '@angular/router'; import { Observable } from 'rxjs'; import { tap } from 'rxjs/operators'; @@ -21,6 +21,8 @@ import { getFirstCompletedRemoteData } from '../shared/operators'; import { DSpaceObject } from '../shared/dspace-object.model'; import { IdentifiableDataService } from './base/identifiable-data.service'; import { getDSORoute } from '../../app-routing-paths'; +import { RESPONSE } from '@nguniversal/express-engine/tokens'; +import { isPlatformServer } from '@angular/common'; const ID_ENDPOINT = 'pid'; const UUID_ENDPOINT = 'dso'; @@ -75,12 +77,20 @@ export class DsoRedirectService { protected objectCache: ObjectCacheService, protected halService: HALEndpointService, private router: Router, + @Optional() @Inject(RESPONSE) private response: any, + @Inject(PLATFORM_ID) private platformId: any ) { this.dataService = new DsoByIdOrUUIDDataService(requestService, rdbService, objectCache, halService); } /** - * Retrieve a DSpaceObject by + * Redirect to a DSpaceObject's path using the given identifier type and ID. + * This is used to redirect paths like "/handle/[prefix]/[suffix]" to the object's path (e.g. /items/[uuid]). + * See LookupGuard for more examples. + * + * If this is called server side (via SSR), it performs a 301 Redirect. + * If this is called client side (via CSR), it simply uses the Angular router to do the redirect. + * * @param id the identifier of the object to retrieve * @param identifierType the type of the given identifier (defaults to UUID) */ @@ -94,7 +104,12 @@ export class DsoRedirectService { if (hasValue(dso.uuid)) { let newRoute = getDSORoute(dso); if (hasValue(newRoute)) { - this.router.navigate([newRoute]); + // If running via SSR, perform a "301 Moved Permanently" redirect for SEO purposes. + if (isPlatformServer(this.platformId)) { + this.response.redirect(301, newRoute); + } else { + this.router.navigate([newRoute]); + } } } } From d4a5308d0c7f845cbe0f0caa18437635e94a3067 Mon Sep 17 00:00:00 2001 From: Tim Donohue Date: Thu, 22 Jun 2023 13:37:14 -0500 Subject: [PATCH 2/2] Refactor to use HardRedirectService. Update its redirect method to support optional statusCode --- .../core/data/dso-redirect.service.spec.ts | 45 +++++-------------- src/app/core/data/dso-redirect.service.ts | 21 +++------ .../core/services/hard-redirect.service.ts | 4 +- .../server-hard-redirect.service.spec.ts | 19 ++++++-- .../services/server-hard-redirect.service.ts | 12 +++-- 5 files changed, 44 insertions(+), 57 deletions(-) diff --git a/src/app/core/data/dso-redirect.service.spec.ts b/src/app/core/data/dso-redirect.service.spec.ts index 64fbd94367..2122dc663a 100644 --- a/src/app/core/data/dso-redirect.service.spec.ts +++ b/src/app/core/data/dso-redirect.service.spec.ts @@ -10,6 +10,7 @@ import { RequestService } from './request.service'; import { createSuccessfulRemoteDataObject } from '../../shared/remote-data.utils'; import { Item } from '../shared/item.model'; import { EMBED_SEPARATOR } from './base/base-data.service'; +import { HardRedirectService } from '../services/hard-redirect.service'; describe('DsoRedirectService', () => { let scheduler: TestScheduler; @@ -17,7 +18,7 @@ describe('DsoRedirectService', () => { let halService: HALEndpointService; let requestService: RequestService; let rdbService: RemoteDataBuildService; - let router; + let redirectService: HardRedirectService; let remoteData; const dsoUUID = '9b4f22f4-164a-49db-8817-3316b6ee5746'; const dsoHandle = '1234567789/22'; @@ -27,9 +28,6 @@ describe('DsoRedirectService', () => { const requestUUIDURL = `https://rest.api/rest/api/pid/find?id=${dsoUUID}`; const requestUUID = '34cfed7c-f597-49ef-9cbe-ea351f0023c2'; const objectCache = {} as ObjectCacheService; - const mockResponse = jasmine.createSpyObj(['redirect']); - const mockPlatformBrowser = 'browser'; - const mockPlatformServer = 'server'; beforeEach(() => { scheduler = getTestScheduler(); @@ -41,9 +39,6 @@ describe('DsoRedirectService', () => { generateRequestId: requestUUID, send: true }); - router = { - navigate: jasmine.createSpy('navigate') - }; remoteData = createSuccessfulRemoteDataObject(Object.assign(new Item(), { type: 'item', @@ -55,14 +50,17 @@ describe('DsoRedirectService', () => { a: remoteData }) }); + + redirectService = jasmine.createSpyObj('redirectService', { + redirect: {} + }); + service = new DsoRedirectService( requestService, rdbService, objectCache, halService, - router, - mockResponse, - mockPlatformBrowser // default to CSR except where explicitly SSR below + redirectService ); }); @@ -109,7 +107,7 @@ describe('DsoRedirectService', () => { redir.subscribe(); scheduler.schedule(() => redir); scheduler.flush(); - expect(router.navigate).toHaveBeenCalledWith(['/items/' + remoteData.payload.uuid]); + expect(redirectService.redirect).toHaveBeenCalledWith('/items/' + remoteData.payload.uuid, 301); }); it('should navigate to entities route with the corresponding entity type', () => { remoteData.payload.type = 'item'; @@ -126,7 +124,7 @@ describe('DsoRedirectService', () => { redir.subscribe(); scheduler.schedule(() => redir); scheduler.flush(); - expect(router.navigate).toHaveBeenCalledWith(['/entities/publication/' + remoteData.payload.uuid]); + expect(redirectService.redirect).toHaveBeenCalledWith('/entities/publication/' + remoteData.payload.uuid, 301); }); it('should navigate to collections route', () => { @@ -135,7 +133,7 @@ describe('DsoRedirectService', () => { redir.subscribe(); scheduler.schedule(() => redir); scheduler.flush(); - expect(router.navigate).toHaveBeenCalledWith(['/collections/' + remoteData.payload.uuid]); + expect(redirectService.redirect).toHaveBeenCalledWith('/collections/' + remoteData.payload.uuid, 301); }); it('should navigate to communities route', () => { @@ -144,26 +142,7 @@ describe('DsoRedirectService', () => { redir.subscribe(); scheduler.schedule(() => redir); scheduler.flush(); - expect(router.navigate).toHaveBeenCalledWith(['/communities/' + remoteData.payload.uuid]); - }); - - it('should return 301 redirect when SSR is used', () => { - service = new DsoRedirectService( - requestService, - rdbService, - objectCache, - halService, - router, - mockResponse, - mockPlatformServer // explicitly SSR mode - ); - remoteData.payload.type = 'item'; - const redir = service.findByIdAndIDType(dsoHandle, IdentifierType.HANDLE); - // The framework would normally subscribe but do it here so we can test navigation. - redir.subscribe(); - scheduler.schedule(() => redir); - scheduler.flush(); - expect(mockResponse.redirect).toHaveBeenCalledWith(301, '/items/' + remoteData.payload.uuid); + expect(redirectService.redirect).toHaveBeenCalledWith('/communities/' + remoteData.payload.uuid, 301); }); }); diff --git a/src/app/core/data/dso-redirect.service.ts b/src/app/core/data/dso-redirect.service.ts index 9b87590f7e..a27d1fb11f 100644 --- a/src/app/core/data/dso-redirect.service.ts +++ b/src/app/core/data/dso-redirect.service.ts @@ -6,8 +6,7 @@ * http://www.dspace.org/license/ */ /* eslint-disable max-classes-per-file */ -import { Inject, Injectable, Optional, PLATFORM_ID } from '@angular/core'; -import { Router } from '@angular/router'; +import { Injectable } from '@angular/core'; import { Observable } from 'rxjs'; import { tap } from 'rxjs/operators'; import { hasValue } from '../../shared/empty.util'; @@ -21,8 +20,7 @@ import { getFirstCompletedRemoteData } from '../shared/operators'; import { DSpaceObject } from '../shared/dspace-object.model'; import { IdentifiableDataService } from './base/identifiable-data.service'; import { getDSORoute } from '../../app-routing-paths'; -import { RESPONSE } from '@nguniversal/express-engine/tokens'; -import { isPlatformServer } from '@angular/common'; +import { HardRedirectService } from '../services/hard-redirect.service'; const ID_ENDPOINT = 'pid'; const UUID_ENDPOINT = 'dso'; @@ -76,9 +74,7 @@ export class DsoRedirectService { protected rdbService: RemoteDataBuildService, protected objectCache: ObjectCacheService, protected halService: HALEndpointService, - private router: Router, - @Optional() @Inject(RESPONSE) private response: any, - @Inject(PLATFORM_ID) private platformId: any + private hardRedirectService: HardRedirectService ) { this.dataService = new DsoByIdOrUUIDDataService(requestService, rdbService, objectCache, halService); } @@ -88,9 +84,6 @@ export class DsoRedirectService { * This is used to redirect paths like "/handle/[prefix]/[suffix]" to the object's path (e.g. /items/[uuid]). * See LookupGuard for more examples. * - * If this is called server side (via SSR), it performs a 301 Redirect. - * If this is called client side (via CSR), it simply uses the Angular router to do the redirect. - * * @param id the identifier of the object to retrieve * @param identifierType the type of the given identifier (defaults to UUID) */ @@ -104,12 +97,8 @@ export class DsoRedirectService { if (hasValue(dso.uuid)) { let newRoute = getDSORoute(dso); if (hasValue(newRoute)) { - // If running via SSR, perform a "301 Moved Permanently" redirect for SEO purposes. - if (isPlatformServer(this.platformId)) { - this.response.redirect(301, newRoute); - } else { - this.router.navigate([newRoute]); - } + // Use a "301 Moved Permanently" redirect for SEO purposes + this.hardRedirectService.redirect(newRoute, 301); } } } diff --git a/src/app/core/services/hard-redirect.service.ts b/src/app/core/services/hard-redirect.service.ts index 3733059283..826c7e4fa9 100644 --- a/src/app/core/services/hard-redirect.service.ts +++ b/src/app/core/services/hard-redirect.service.ts @@ -11,8 +11,10 @@ export abstract class HardRedirectService { * * @param url * the page to redirect to + * @param statusCode + * optional HTTP status code to use for redirect (default = 302, which is a temporary redirect) */ - abstract redirect(url: string); + abstract redirect(url: string, statusCode?: number); /** * Get the current route, with query params included diff --git a/src/app/core/services/server-hard-redirect.service.spec.ts b/src/app/core/services/server-hard-redirect.service.spec.ts index 4501547b92..6bd5828921 100644 --- a/src/app/core/services/server-hard-redirect.service.spec.ts +++ b/src/app/core/services/server-hard-redirect.service.spec.ts @@ -22,20 +22,33 @@ describe('ServerHardRedirectService', () => { expect(service).toBeTruthy(); }); - describe('when performing a redirect', () => { - + describe('when performing a default redirect', () => { const redirect = 'test redirect'; beforeEach(() => { service.redirect(redirect); }); - it('should update the response object', () => { + it('should perform a 302 redirect', () => { expect(mockResponse.redirect).toHaveBeenCalledWith(302, redirect); expect(mockResponse.end).toHaveBeenCalled(); }); }); + describe('when performing a 301 redirect', () => { + const redirect = 'test 301 redirect'; + const redirectStatusCode = 301; + + beforeEach(() => { + service.redirect(redirect, redirectStatusCode); + }); + + it('should redirect with passed in status code', () => { + expect(mockResponse.redirect).toHaveBeenCalledWith(redirectStatusCode, redirect); + expect(mockResponse.end).toHaveBeenCalled(); + }); + }); + describe('when requesting the current route', () => { beforeEach(() => { diff --git a/src/app/core/services/server-hard-redirect.service.ts b/src/app/core/services/server-hard-redirect.service.ts index de8b45b0e5..8c45cc864b 100644 --- a/src/app/core/services/server-hard-redirect.service.ts +++ b/src/app/core/services/server-hard-redirect.service.ts @@ -17,10 +17,14 @@ export class ServerHardRedirectService extends HardRedirectService { } /** - * Perform a hard redirect to URL + * Perform a hard redirect to a given location. + * * @param url + * the page to redirect to + * @param statusCode + * optional HTTP status code to use for redirect (default = 302, which is a temporary redirect) */ - redirect(url: string) { + redirect(url: string, statusCode?: number) { if (url === this.req.url) { return; @@ -38,8 +42,8 @@ export class ServerHardRedirectService extends HardRedirectService { process.exit(1); } } else { - // attempt to use the already set status - let status = this.res.statusCode || 0; + // attempt to use passed in statusCode or the already set status (in request) + let status = statusCode || this.res.statusCode || 0; if (status < 300 || status >= 400) { // temporary redirect status = 302;