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

atarix83 · 2024-09-26T14:02:06Z

References

Add references/links to any related issues or PRs. These may include:

Fixes Unable to route network requests to different DSpace REST API URL during SSR #1485
Requires Provide a setting to use a different REST url during SSR execution DSpace#9856

Description

This pull request provide the possibility to use a different DSpace REST url during the SSR

Instructions for Reviewers

List of changes in this PR:

Added a new setting ssrBaseUrl where to specify a different DSpace REST url to use during SSR
Added a new interceptor which replaces the base url used to contact the REST server, if a different DSpace REST url is provided
Changed the behaviour of the thumbnail component. The thumbnail component now renders exclusively in the browser. This change is necessary to prevent issues during server-side rendering (SSR). When a page containing a thumbnail is rendered on the server, the HTML content delivered to the browser would attempt to download the thumbnail using the URL specified in the ssrBaseUrl property. If this URL was not publicly accessible, it would result in an error. By rendering the thumbnail only in the browser, we avoid this problem.
An additional change is required with the ServerHardRedirectService. In case the url to redirect contains the ssrBaseUrl it's reinstated with the public base url. This is necessary otherwise download bitstream page doesn't work.

Include guidance for how to test or review your PR.
Unfortunately it is difficult to test this PR. The best would be to deploy it in a production like environment and configure the internal base url property.
This might be done on both Angular and REST side, like in the example below where public base url is https://dspace-rest.org/server and internal base url is http://localhost:8080/server.

Angular: in your config.prod.yml file set the ssrBaseUrl property, e.g :

rest:
  ssl: true
  host: dspace-rest.org
  port: 443
  nameSpace: /server
  ssrBaseUrl: http://localhost:8080/server

REST: in your local.cfg file set the dspace.server.ssr.url property, e.g. :

# Public URL of DSpace backend ('server' webapp). May require a port number if not using standard ports (80 or 443)
# DO NOT end it with '/'.
# This is where REST API and all enabled server modules (OAI-PMH, SWORD, SWORDv2, RDF, etc) will respond.
# NOTE: This URL must be accessible to all DSpace users (should not use 'localhost' in Production)
# and is usually "synced" with the "rest" section in the DSpace User Interface's config.*.yml.
# It corresponds to the URL that you would type into your browser to access the REST API.
dspace.server.url = https://dspace-rest.org/server

# Additional URL of DSpace backend which could be used by DSpace frontend during SSR execution.
# May require a port number if not using standard ports (80 or 443)
# DO NOT end it with '/'.
dspace.server.ssr.url = http://localhost:8080/server

In order to test it locally you could add an alias hostname for localhost ip to be used as public url

Checklist

…R execution

tdonohue · 2024-09-26T17:00:30Z

@atarix83 : While I realize this is still in "draft", I'm assigning this to @artlowel and I for review. @artlowel , I know you have a lot on your plate, but this seems like a high priority fix that would be good to get your or your team's feedback on.

ybnd

Everything works as expected, but still "leaks" the SSR URL and does double work since the transfer state is ignored

ybnd · 2024-10-03T16:04:33Z

src/modules/app/browser-init.service.ts

-        map(() => true),
-      ),
-    );
+    // The app state can be transferred only when SSR and CSR are using the same base url for the REST API


A separate REST URL could be a performance benefit during SSR, but if we discard all cached requests we'll always do double work and CSR performance will suffer.

Could we consider just running a search/replace on the transfer state to replace the SSR URL with the CSR URL?

This shouldn't be too heavy of an operation so I think the net impact on performance could still be positive

I did a very quick check and replacing all URLs right after the full HTML has been generated takes at most 0.25% of the total time needed for SSR

Only tested for /home, not authenticated

SSR took ~400ms, HTML search replace took between 0 and 1ms

Otherwise we should at least exclude the transfer state entirely; right now it's sent but never read.

I understand your suggestion, but I believe implementing a search/replace operation for URLs might not be a good idea due to performance concerns. Even though the initial tests show a minimal impact, this might not be the case for all routes or under different conditions and it could become a bottleneck.
Moreover adding an extra step in the Server-side rendering process can increase the overall latency, negatively affecting the user experience, particularly for pages with larger amount of request.
Honestly I don't see a real issue to fetch again the request during the CSR.

I'll try to exclude the transfer state entirely

Honestly I don't see a real issue to fetch again the request during the CSR

My main concern is that it has a significant impact on perceived performance, especially on slower connections.
This counteracts our push towards decreasing the amount of REST requests we send ~ #3110 (comment).

I've made a quick branch where you can toggle both things: ybnd/dspace-angular → task/main/DURACOM-288

ui.transferState → whether NGRX state is transferred from SSR to CSR

replaceRestUrl → whether the SSR URL is replaced after SSR is done

As far as I can tell, a simple RegEx to replace SSR URLs scales a lot better than DSpace itself

A /search page with 1000 results (!) takes ~50000ms to build with SSR on my machine

Replacing all URLs (~16000) takes ~20ms

On the other hand, for a /search page with 100 results, Lighthouse reports about double the blocking time if we don't transfer state on CSR.

With URL replacing & state transfer

Without state transfer

ybnd · 2024-10-03T16:13:07Z

src/modules/app/server-init.service.ts

@@ -100,6 +101,14 @@ export class ServerInitService extends InitService {
  }

  private saveAppConfigForCSR(): void {
-    this.transferState.set<AppConfig>(APP_CONFIG_STATE, environment as AppConfig);
+    if (isNotEmpty(environment.rest.ssrBaseUrl) && environment.rest.baseUrl !== environment.rest.ssrBaseUrl) {
+      // Avoid to transfer ssrBaseUrl in order to prevent security issues


I agree about the potential security concern, good idea.
The state transfer JSON will still list the SSR URL. It's a bit more "hidden" but not secure either.

github-actions · 2024-10-08T20:08:16Z

Hi @atarix83,
Conflicts have been detected against the base branch.
Please resolve these conflicts as soon as you can. Thanks!

# Conflicts: # src/app/thumbnail/thumbnail.component.spec.ts

tdonohue

Thanks @atarix83 ! I gave this a code review today, and have some minor requests for improvement. I haven't had a chance to fully test this yet, but I plan to do so next week.

tdonohue · 2024-11-15T20:32:58Z

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

+    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);


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.

tdonohue · 2024-11-15T20:41:22Z

src/config/server-config.interface.ts

@@ -6,4 +6,6 @@ export class ServerConfig implements Config {
  public port: number;
  public nameSpace: string;
  public baseUrl?: string;
+  public ssrBaseUrl?: string;


We need to add this new ssrBaseUrl setting to our config.example.yml. Currently, it's undocumented and "hidden".

tdonohue · 2024-11-15T20:43:28Z

src/config/server-config.interface.ts

@@ -6,4 +6,6 @@ export class ServerConfig implements Config {
  public port: number;
  public nameSpace: string;
  public baseUrl?: string;
+  public ssrBaseUrl?: string;
+  public hasSsrBaseUrl?: boolean;


Could we add a comment here to describe how/where this boolean value is set? This doesn't appear to be something we want people to specify in their config.yml, so it would be good to document that this is set automatically based on the value of ssrBaseUrl in your config.

…ting enter in another input form (onsubmit)

…esetting-on-enter_contribute-7.6' # Conflicts: # src/app/shared/form/builder/ds-dynamic-form-ui/models/scrollable-dropdown/dynamic-scrollable-dropdown.component.html

…ting the enter key in another input field

atarix83 · 2025-01-09T18:22:42Z

@tdonohue @ybnd

I've improved my implementation trying to address your feedback. Here the main changes:

added a new config property to disable transfer state (see here). By default even if the property is enabled when a different SSR base url is set the state is not transferred from server to client application.
added a new config to allow url replacement in the state transferred to the client application. default to false.

[DURACOM-288] Provide a setting to use a different REST url during SS…

8ec8eed

…R execution

github-actions bot assigned atarix83 Sep 26, 2024

tdonohue added high priority performance / caching Related to performance, caching or embedded objects labels Sep 26, 2024

tdonohue requested review from artlowel and tdonohue September 26, 2024 17:00

tdonohue changed the title ~~[DURACOM-288] Provide a setting to use a different REST url during SS…~~ Provide a setting to use a different REST url during SSR execution Sep 26, 2024

atarix83 mentioned this pull request Sep 26, 2024

Provide a setting to use a different REST url during SSR execution DSpace/DSpace#9856

Open

10 tasks

atarix83 marked this pull request as ready for review September 26, 2024 18:05

artlowel requested review from ybnd and removed request for artlowel September 27, 2024 07:46

[DURACOM-288] Additional test for thumbnail component

268cf64

ybnd suggested changes Oct 3, 2024

View reviewed changes

github-actions bot added the merge conflict label Oct 8, 2024

Merge branch 'main' into task/main/DURACOM-288

a1cbe3b

# Conflicts: # src/app/thumbnail/thumbnail.component.spec.ts

github-actions bot removed the merge conflict label Nov 7, 2024

[DURACOM-288] Fix after merge

a9223ea

tdonohue requested a review from ybnd November 15, 2024 22:03

tdonohue requested changes Nov 15, 2024

View reviewed changes

alexandrevryghem and others added 8 commits December 4, 2024 17:16

119799: Prevent submission from updating the dropdown values when hit…

ed4e794

…ting enter in another input form (onsubmit)

Merge remote-tracking branch 'alex/w2p-122005_fixed-dropdown-values-r…

c12a3e6

…esetting-on-enter_contribute-7.6' # Conflicts: # src/app/shared/form/builder/ds-dynamic-form-ui/models/scrollable-dropdown/dynamic-scrollable-dropdown.component.html

119799: Prevent the lookup/lookup-name fields from resetting when hit…

c8694e1

…ting the enter key in another input field

Merge branch 'main' into task/main/DURACOM-288

428f859

Add configuration to toggle replace/transfer

dae1e68

[DURACOM-288] Refactoring configuration to transfer state

1ee754e

[DURACOM-288] Add missing settings to config.example.yml

6b9bc66

[DURACOM-288] Add unit test to test SSR url replace

c35964a

atarix83 requested a review from tdonohue January 9, 2025 18:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

atarix83 commented Sep 26, 2024 •

edited

Loading

tdonohue commented Sep 26, 2024

ybnd left a comment

ybnd Oct 3, 2024

atarix83 Nov 7, 2024

ybnd Nov 20, 2024

ybnd Oct 3, 2024

github-actions bot commented Oct 8, 2024

tdonohue left a comment

tdonohue Nov 15, 2024

atarix83 Jan 9, 2025

tdonohue Nov 15, 2024

atarix83 Jan 9, 2025

tdonohue Nov 15, 2024

atarix83 commented Jan 9, 2025

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

Are you sure you want to change the base?

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

Conversation

atarix83 commented Sep 26, 2024 • edited Loading

References

Description

Instructions for Reviewers

Checklist

tdonohue commented Sep 26, 2024

ybnd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 8, 2024

tdonohue left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atarix83 commented Jan 9, 2025

atarix83 commented Sep 26, 2024 •

edited

Loading