Fix multiple api calls on route change

[artlowel]

References

• Fixes Repeated requests to the server/api endpoint.  #2482

Description

The issue was caused by the fact that the guard made a new request to the api every time something subscribed to the result of its canActivateChild method. That happened more then once per route change

The solution was to use the cached /api response, and invalidate that cache on every NavigationStart event. That way, the first subscriber triggers the re-request, and all the others use the cached version.

Instructions for Reviewers

Open the network tab in the browser's dev tools, and check that there's one a single /api request every time you change the route

Also verify that if the backend becomes unavailable, that you're redirected to the 500 error page the next time you change the route

Checklist

This checklist provides a reminder of what we are going to look for when reviewing your PR. You need not complete this checklist prior to creating your PR (draft PRs are always welcome). If you are unsure about an item in the checklist, don't hesitate to ask. We're here to help!

• My PR is small in size (e.g. less than 1,000 lines of code, not including comments & specs/tests), or I have provided reasons as to why that's not possible.
• My PR passes ESLint validation using yarn lint
• My PR doesn't introduce circular dependencies (verified via yarn check-circ-deps)
• My PR includes TypeDoc comments for all new (or modified) public methods and classes. It also includes TypeDoc for large or complex private methods.
• My PR passes all specs/tests and includes new/updated specs or tests based on the Code Testing Guide.
• If my PR includes new libraries/dependencies (in package.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.
• If my PR includes new features or configurations, I've provided basic technical documentation in the PR itself.
• If my PR fixes an issue ticket, I've linked them together.

[alanorth]

Thanks @artlowel! I tested the patch by opening the network tab of my browser dev tools and clearing it before navigating to another route. Before applying I would see two requests to /api. After applying I only see one request to /api. Also, as suggested, I stopped Tomcat and then the next time I tried to navigate to a new route I received the HTTP 500 error page.

I was +1 on this until I realized that it reliably breaks my login dropdown menu:

The console shows:

ERROR Error: undefined doesn't contain the link authn
    18005/getEndpointAt/nextHref$< hal-endpoint.service.ts:71
    RxJS 61
    liftedStateSubscription ngrx-store-devtools.mjs:825
    RxJS 36
    next ngrx-store.mjs:206
    dispatch ngrx-store.mjs:529
    setStaleByHref request.service.ts:369
    RxJS 26
core.mjs:8400:22
    Angular 2
    RxJS 6
    Angular 11

[artlowel]

@alanorth I can reproduce your issue only when the backend is down. It makes sense that you can't log in when the backend is down. After it is back up, logging in works again for me.

I did however discover a bug where the login dropdown goes blank if you enter the wrong account details. But I can reproduce that on both https://demo.dspace.org/ and on https://sandbox.dspace.org/, so it's not related to this PR. I created an issue for it: #2515

[alanorth]

@artlowel I just tried again, and my backend is definitely up because I can browse everywhere in the repository, but my login dropdown is white. Shift refresh, nothing. See quick demo (use Chrome to view the video, GitHub mp4 is broken on Firefox):

api-routes.mp4

Another thing I noticed is that some browse pages actually make THREE calls to /api! Check "Browse by Issue Date" for example. With this patch they only make one. So that's a win.

[artlowel]

@alanorth The white dropdown bug is the one I reported in #2515. It is not related to this PR, as it also occurs on both demo sites

[alanorth]

@artlowel if I revert the patch the white login dropdown issue goes away. Also note, in my video above, that I have not attempted login at all, so I'm not sure that bug is related to a failed login.

[artlowel]

@alanorth then that must mean that #2515 also happens for other errors, besides a failed login.

Can you check your console when the blank dropdown issue happens. Is undefined doesn't contain the link authn still the error shown? The reason I ask is because that error tends to mean that /api can't be retrieved (as authn is a link on /api) and in your video we see that it can be retrieved

[alanorth]

@artlowel yes, it is reliable. Right after applying the patch, Angular rebuilds, the page refreshes, and I get errors in the console. Three messages about missing links (one for authn, two for properties):

[artlowel]

@alanorth I'm able to reproduce your issue now. It happens only in dev mode. I'll take a look

[J4bbi]

Just to quickly note that I get the same errors. Running in development mode with sandbox as API.

I don't know if it's an issue but the CanActivateChild interface used in server-check.guard.ts is deprecated in favour of "functional injectable guards". Is this a concern? (I have no idea)

I put in some console.traces() and seems to me that a lot of services and functionality want to know how server/api is doing.

[artlowel]

@alanorth @J4bbi the issue should be fixed. Can you try again?

[artlowel]

@J4bbi

I don't know if it's an issue but the CanActivateChild interface used in server-check.guard.ts is deprecated in favour of "functional injectable guards". Is this a concern? (I have no idea)

It looks like CanActivate itself is also deprecated, which means that eventually we'll need to update all our guards. I think this is something that is best done all at once, e.g. in the context of the next angular upgrade

[alanorth]

Hey @artlowel. I tested again with the updated patch and I am able to log in. I also confirm that the calls to /api are still reduced to one as expected. I think I'm +1, but can wait for @J4bbi to comment as well.

[J4bbi]

Just one question. Everything works as it should

[J4bbi]

@artlowel , great. I just wonder, because that number seems somewhat arbitrary, if it's possible to leave a comment to explain why it's 6 hours and not something else, or if there is something dependent on it being at least a specific value?

[alanorth]

Merging this as it has been tested by myself and @J4bbi, with our review feedback addressed.

I think if we want to add a comment about the six hours you can add that later and we can merge it without testing all that.

[dspace-bot]

Backport failed for main, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally.

git fetch origin main
git worktree add -d .worktree/backport-2510-to-main origin/main
cd .worktree/backport-2510-to-main
git checkout -b backport-2510-to-main
ancref=$(git merge-base 2d733732f626bb3fb0a0e7a528334e5187fde542 67a6f58865b3b0da5da950adc10d21c49e83c043)
git cherry-pick -x $ancref..67a6f58865b3b0da5da950adc10d21c49e83c043

[artlowel]

@J4bbi

@artlowel , great. I just wonder, because that number seems somewhat arbitrary, if it's possible to leave a comment to explain why it's 6 hours and not something else, or if there is something dependent on it being at least a specific value?

It doesn't have to be any specific value no. The original idea was that it should basically never expire because all the /api endpoint was used for was to retrieve the links to all other endpoints, and those only change if major new features are added to dspace. So 6 hours was chosen as just "a long time".

With the introduction of the ServerCheckGuard, that this PR makes some tweaks to, that endpoint is used to check that the server is still running as well. So in practice those 6 hours will never be reached, because the timer will restart every time you switch routes. The reason I still rolled back the change though, is because I don't want to introduce issues where the validity expires while you're on a page and not doing anything. In theory, if everything else is properly designed to use dataservices and work with the cache that shouldn't be a problem, it would just cause /api to be re-requested automatically when that time is up. But I'd rather not risk introducing new edge cases that haven't been tested for

[alanorth]

@artlowel the automatic port to main failed. Would you be able to port it manually and make a PR? Assuming all tests pass we can merge it ASAP. Thanks!