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

If there's no correlation ID cookie, server side rendering crashes #1279

Closed
artlowel opened this issue Jul 26, 2021 · 1 comment · Fixed by #1280
Closed

If there's no correlation ID cookie, server side rendering crashes #1279

artlowel opened this issue Jul 26, 2021 · 1 comment · Fixed by #1280
Assignees
Labels
Milestone

Comments

@artlowel
Copy link
Member

artlowel commented Jul 26, 2021

Describe the bug
The log interceptor gets the correlation ID from the cookie and adds it as a request header. However if you set a request header that's undefined, HttpClient will crash when sending the request.

If it doesn't exist, a correlation ID is generated in an APP_INITIALIZER function, and stored as a cookie. But you can't store cookies server side, that method isn't implemented

To Reproduce

  • open an incognito window, or clear your cookies
  • disable javascript
  • open the dspace UI in prod mode
  • It will keep loading forever

Expected behavior

  • The correlation ID should be moved to the ngrx store
  • That initializer function should check if there's a correlation ID in the store. If there is it shouldn't do anything. If there isn't, it should check if there's a cookie and init the store with it, or generate a new ID and put that in the store
  • The interceptor should get the value from the store
  • The interceptor should check if the id exists before adding it as a header, to ensure we don't get undefined headers

That last part is the critical thing to get rid of the symptoms, so I'll create a small PR to add that check, and some better error logging for these kinds of issues.

Until we fix the other parts, we won't get correlation IDs for the very first request of a client with JS, or for any request of a client without JS.

The rest can be done in an estimated 3 hours (moved to #1281)

I already spent 2 hours on analysis. (this was the work done in #1280)

To prevent similar issues it may also be a good idea to remove the 'set' and 'remove' methods from the cookieservice interface, and implement them only on ClientCookieService. That way you need to be sure you're on the client before calling them. I tried throwing an exception when these methods are called server-side in my PR, but that happens a lot, so that would litter the logs with unnecessary warnings.

Related work
Introduced by #1255

@artlowel artlowel added bug needs triage New issue needs triage and/or scheduling labels Jul 26, 2021
@tdonohue tdonohue added high priority and removed needs triage New issue needs triage and/or scheduling labels Jul 26, 2021
@tdonohue tdonohue added this to the 7.0 milestone Jul 26, 2021
@tdonohue tdonohue added e/5 Estimate in hours e/2 Estimate in hours and removed e/5 Estimate in hours labels Jul 26, 2021
@tdonohue
Copy link
Member

tdonohue commented Jul 26, 2021

This ticket has been split into two:

  1. Current 1279 ticket: re-scoped to only fix the immediate SSR bug, see Fix issue where the app wouldn't load for the first request in prod mode #1280
  2. Additional CORRELATION_ID issues were moved to CORRELATION_ID doesn't exist for first request of every client and for every request of non-JS clients #1281 and rescheduled for 7.1

4science-it pushed a commit to 4Science/dspace-angular that referenced this issue Jan 26, 2024
Item mapper main DSC-1073

Approved-by: Stefano Maffei
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants