dspace-angular icon indicating copy to clipboard operation
dspace-angular copied to clipboard

Fixes for #2891 #2889

Open atarix83 opened this issue 1 year ago • 3 comments

References

  • Fixes #2891
  • Fixes #2889

Description

This is a small follow-up PR to fix the mentioned issues

Checklist

  • [x] 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.
  • [x] My PR passes ESLint validation using yarn lint
  • [x] My PR doesn't introduce circular dependencies (verified via yarn check-circ-deps)
  • [x] My PR includes TypeDoc comments for all new (or modified) public methods and classes. It also includes TypeDoc for large or complex private methods.
  • [x] My PR passes all specs/tests and includes new/updated specs or tests based on the Code Testing Guide.
  • [x] 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.
  • [x] If my PR includes new features or configurations, I've provided basic technical documentation in the PR itself.
  • [x] If my PR fixes an issue ticket, I've linked them together.

atarix83 avatar Apr 05 '24 07:04 atarix83

@tdonohue @artlowel

regarding the CodeQL alert, i've added some checks how is described in the documentation but it persists. Do you have any suggestion, could we ignore it or do i need to investigate further?

atarix83 avatar Apr 05 '24 13:04 atarix83

@atarix83 The suggestion from CodeQL is to turn LazyDataServicesMap into an actual Map object:

in data-services.map.ts:

export const LAZY_DATA_SERVICES = new Map<string, () => Promise<Type<HALDataService<any>>>>()
LAZY_DATA_SERVICES.set(AUTHORIZATION.value, () => import('./data/feature-authorization/authorization-data.service').then(m => m.AuthorizationDataService));
...

in lazy-service pass both the map and the key, so you can write something like:

if (map.has(key)) {
   const loader = map.get(key):
   loader();
} else {
   trow new Error(`The key ${key} was not found in the map ${map}`);
}

where map has the type Map<string, () => Promise<any>> in the method to ensure that loader() is indeed a function that returns a promise. If that doesn't appease CodeQL, you may need to explicitly test that loader is a function as well.

artlowel avatar Apr 08 '24 11:04 artlowel

thanks @artlowel for suggestions, now every checks pass and PR is to be reviewed

atarix83 avatar Apr 09 '24 07:04 atarix83