Refactor data services to use composition

[ybnd]

References

• Fixes #742

Description

This PR refactors all data services into a composition pattern

• Data services should extend BaseDataService or IdentifiableDataService

  • BaseDataService contains only the most basic methods shared by all
  • IdentifiableDataService also contains methods that address resources by their ID. We chose to use inheritance instead of composition because of the number of services that depend on IDs.

• All other DataService functionality was split up into "feature" classes, that services can now compose.

  • The following feature classes are available:
    • CreateData
    • FindAllData
    • SearchData
    • PatchData
    • PutData
    • DeleteData
  • To support one of these features, a concrete data service class must
    • implement the feature's interface (e.g. SearchData)
    • wrap an instance of that feature's implementation (e.g. SearchDataImpl)
    • forward the interface's methods to that instance
  • Some logic that used to be modified on a case-by-case basis by overrides had to be reworked
    • "ID endpoints" can be adjusted by passing a constructIdEndpoint function. In this way they can be synchronised between all instances a data service uses.
    • "search endpoints" are handled in the same way so we can just reuse SearchDataImpl everywhere

• A special case was made forHrefDataService: it now uses a "full" BaseDataService under the hood, but only exposes findByHref and findListByHref while changing their signature to support

  itemRD$: Observable<RemoteData<Item>> = hrefOnlyDataService.findByHref<Item>(...);
  

• The @dataService decorator is now typed more strictly

  • For the purposes of LinkService, it is sufficient for data services to implement findByHref and findListByHref BaseDataService and HrefOnlyDataService implement a common interface that only contains these two methods: HALDataService
  • @dataService can only decorate classes that implement HALDataService
  • This also means that DATA_SERVICE_FACTORY can now guarantee that all data services can

Instructions for Reviewers

• Go through the app and test as many CRUD operations as is reasonable

  • Browse / search / DSO pages
  • Submission
  • Workflow
  • EPerson / Group administration
  • Registries
  • ...

  Everything should be functionally the same as it was before this PR, no unexpected errors should come up.

• @dataService decorators on classes that don't implement HALDataService should now be flagged as an error by TypeScript

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.
• [x] My PR passes TSLint validation using yarn run lint
• [x] My PR doesn't introduce circular dependencies
• [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, third-party dependencies (in package.json), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.

[lgtm-com[bot]]

This pull request introduces 5 alerts and fixes 4 when merging fbaab6912162e789adcf363d9fa7f2996cdb2aec into 9a5a7c13068af5db56b9450e1d4f004ac59f606c - view on LGTM.com

new alerts:

• 5 for Unused variable, import, function or class

fixed alerts:

• 4 for Unused variable, import, function or class

[lgtm-com[bot]]

This pull request fixes 4 alerts when merging ee26084d6adcd4c25e470cd68c2419c2a7eb393d into 9a5a7c13068af5db56b9450e1d4f004ac59f606c - view on LGTM.com

fixed alerts:

• 4 for Unused variable, import, function or class

[lgtm-com[bot]]

This pull request introduces 2 alerts and fixes 4 when merging 3e3d674194395d7782bcfe42ca489165f86f6248 into 342a71251337f69b524a4012afaf075afa8e82d5 - view on LGTM.com

new alerts:

• 2 for Unused variable, import, function or class

fixed alerts:

• 4 for Unused variable, import, function or class

[lgtm-com[bot]]

This pull request fixes 4 alerts when merging a6fb4a6303ab47ac1f54218cdad1d05168d40af4 into 342a71251337f69b524a4012afaf075afa8e82d5 - view on LGTM.com

fixed alerts:

• 4 for Unused variable, import, function or class

[ybnd]

Last commit ~~seems to have introduced some weird e2e failures~~ broke everything, so I've reverted it

[lgtm-com[bot]]

This pull request introduces 1 alert and fixes 4 when merging 147c7180d0684e93eea06daa5ccb85889616f925 into e7dc5f8d149e5dbbc0dee4f121596f9fbc104cb2 - view on LGTM.com

new alerts:

• 1 for Unused variable, import, function or class

fixed alerts:

• 4 for Unused variable, import, function or class

[lgtm-com[bot]]

This pull request fixes 4 alerts when merging cc697041ebd14830aa65c2e8e9c97d92f9188127 into e7dc5f8d149e5dbbc0dee4f121596f9fbc104cb2 - view on LGTM.com

fixed alerts:

• 4 for Unused variable, import, function or class

[tdonohue]

@ybnd : You might already be aware of it, but this PR seems to be broken now...after the most recent refactors there are compilation errors. See the automated CI tests for details.

[ybnd]

Looks like this was due to the recent merging of #1792 -- the refactored ProcessDataService didn't implement DeleteData because it didn't use it before.

[lgtm-com[bot]]

This pull request fixes 4 alerts when merging 3002094e065fef1d280d3b968f00f99e6dd59f34 into 6d361beb8812fa6c26e7c957ee2f4990e6612d6b - view on LGTM.com

fixed alerts:

• 4 for Unused variable, import, function or class

[lgtm-com[bot]]

This pull request fixes 4 alerts when merging 919b4062602c32eec3a19187505e7561e7a28b99 into 6d361beb8812fa6c26e7c957ee2f4990e6612d6b - view on LGTM.com

fixed alerts:

• 4 for Unused variable, import, function or class

[lgtm-com[bot]]

This pull request fixes 4 alerts when merging dda396092ba272ed315f09792ee037896ad220cd into 6d361beb8812fa6c26e7c957ee2f4990e6612d6b - view on LGTM.com

fixed alerts:

• 4 for Unused variable, import, function or class

[lgtm-com[bot]]

This pull request fixes 4 alerts when merging c517ed0dfabd9f973d8ca89c6223eeba621eba8f into 6d361beb8812fa6c26e7c957ee2f4990e6612d6b - view on LGTM.com

fixed alerts:

• 4 for Unused variable, import, function or class