Coar Notify Integration

[frabacche]

Description

Quality Assurance Events and LDN Message Protocol.

Based on these old Pull Requests:

• contract: https://github.com/DSpace/RestContract/pull/181
• backend: https://github.com/DSpace/DSpace/pull/8184
• frontend: https://github.com/DSpace/dspace-angular/pull/1562

Related to new Pull Requests:

• contract: https://github.com/DSpace/RestContract/pull/249
• frontend: https://github.com/DSpace/dspace-angular/pull/2681

Continues on:

• backend: https://github.com/DSpace/DSpace/pull/9268
• frontend: https://github.com/DSpace/dspace-angular/pull/2751
• contract: https://github.com/DSpace/RestContract/pull/251

Instructions for Reviewers

Technical implementations documented here : https://wiki.lyrasis.org/pages/viewpage.action?pageId=319815713

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 & integration tests). Exceptions may be made if previously agreed upon.
• [ ] My PR passes Checkstyle validation based on the Code Style Guide.
• [ ] My PR includes Javadoc for all new (or modified) public methods and classes. It also includes Javadoc for large or complex private methods.
• [ ] My PR passes all tests and includes new/updated Unit or Integration Tests based on the Code Testing Guide.
• [ ] If my PR includes new libraries/dependencies (in any pom.xml), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.
• [ ] If my PR modifies REST API endpoints, I've opened a separate REST Contract PR related to this change.
• [ ] If my PR includes new configurations, I've provided basic technical documentation in the PR itself.
• [ ] If my PR fixes an issue ticket, I've linked them together.

[github-actions[bot]]

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

[github-actions[bot]]

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

[frabacche]

@tdonohue failing check for "server-side request forgery" is a false-positive. The url of the post we are doing in that code is configured only from the admin panel; I added a comment about that on the source java class. Can we flag it? Thank you.

[abollini]

@frabacche I have dismissed the message as it is a false positive. The url is (eventually) a redirection requested by a trusted server (the server url are entered by the administrators)

[github-actions[bot]]

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

[steph-ieffam]

@frabacche : Thanks for this PR. I gave this a code review only today (as it's not in a testable state, and I'm waiting on the OpenAIRE suggestions PR to be ready first).

I did my best to only review code that is specific to COAR Notify, but I may need to do a secondary review after the OpenAIRE Suggestions PR is merged.

Overall, the code looks good. I do have some small suggestions for improvements though and also some questions. It is possible that some of my questions are misunderstandings since I cannot yet test the code.

Also, I wanted to note that the COAR Notify technical documentation is inaccessible at https://4science.atlassian.net/wiki/spaces/DEV/pages/3649404929/COAR+Notify (Or at least I cannot view it). Could we move this to the DSpace wiki so that others can see it?

Hello @tdonohue sorry for the wrong link in the PR, the actual link is https://wiki.lyrasis.org/pages/viewpage.action?pageId=319815713 which is currently marked as draft. In the next days we will give you a feedback on each comment for this set of PRs.

Thanks

[github-actions[bot]]

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

[github-actions[bot]]

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

[tdonohue]

@frabacche : It looks like this PR has a merge conflict. Could you resolve that so that builds/tests run again? It was likely caused by the merger of #9195 yesterday

[tdonohue]

@frabacche : I'm noticing the the ITs are still not working for this PR. The ITs have been running for ~3 hours now and still haven't completed. In a normal build of main, the ITs usually only take about 25-30mins. Something in this PR has caused them to slow down immensely...it still seems to have begun in this commit https://github.com/DSpace/DSpace/commit/ab2058201c8c9a588be3db044ae11511372cc259

[frabacche]

@frabacche : I'm noticing the the ITs are still not working for this PR. The ITs have been running for ~3 hours now and still haven't completed. In a normal build of main, the ITs usually only take about 25-30mins. Something in this PR has caused them to slow down immensely...it still seems to have begun in this commit ab20582

I am currently investigating all the issues above.

[github-actions[bot]]

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

[tdonohue]

@frabacche : Apologies but this has a merge conflict now after I merged #9241 . I suspect this will need some updates as REST model objects now need to all override a new getTypePlural() method. See the examples in the code changes in #9241... e.g. https://github.com/DSpace/DSpace/pull/9241/files#diff-d1ca1f69391f16f77a8143273537d3dddc5ed14b9e434d20bc757f18e46972ee

[frabacche]

hi @tdonohue let's have a recap of the situation here.

• IT tests took several hours to complete because of a typo inside the xml test configuration file /dspace-api/src/test/data/dspaceFolder/config/item-submission.xml ;. It has been fixed - the timing is now back to normal;
• some LDN IT java classes with the usage of Mocked services had no .close() call of the http resource objects in use: so I add it;
• ldn.enabled is the only configuration property that activates/deactivates the ldn inbox and the ldn message queue batches: extractor and timeoutChecker;
• the merge of PR #9241 has been managed: PLURAL_NAME have been added where it was missing, and it has been used into RestRepository Annotations;

For these reasons I refreshed the review request of this pr to you.

Please consider I went deep into [LDN Notify documentation same linked above in old comments so you can find it updated. There's a paragraph in Work In Progress to show in detail the test case of the LDN integration at item submission time.

[tdonohue]

@frabacche : It appears our messages crossed one another. I've already reviewed/tested the latest code in this PR (as of commit eff4dd5). See my review above: https://github.com/DSpace/DSpace/pull/9218#pullrequestreview-1896960727

[steph-ieffam]

@tdonohue I am replying to the following point

2. I found that if I send an LDN message which does NOT match with an Inbound Pattern, the message will be processed and will generate a Quality Assurance event. This seems like a possible bug? Here's what I did:

   • First, in "LDN Services" make sure you have NO services that accept the "Announce Review" inbound pattern.
   • Send an "Announce Review" via Postman for an Item in your system. It will go into the queue
   • Once it is processed, that "Announce Review" will become a QA event on the Item even though it didn't match with any registered LDN Service (Is this a bug? My assumption is that every message MUST match with an LDN Service that you've configured. In this case it does not.)

Registered LDN Inbourd pattern are related to the external LDN service. So it's normal that the repository is accepting any notification since there's actually no such thing as inbound pattern for the local repository. Basically in the LDN inbound pattern we're registering the "operations/actions" supported by the external service not the ones we the repository is allowed to receive. Received LDN Messages are always stored. Some of them are skipped for processing (for example the unmapped actions) and they are stored with the appropriate queue status.

[tdonohue]

@steph-ieffam : Thanks for clarifying that "Inbound Pattern" references the external service. That was not clear to me, so we need to make sure to document that better. The term "inbound" had implied to me that it was for incoming messages (from the external service) into DSpace. I now understand though that it means inbound messages for the external service (coming from DSpace).

[frabacche]

hi @tdonohue we have updated the Postman collection uploaded on the lowest part of the documentation. Every request inside the LDN group has been cleaned up. We are currently taking care of all that you said above (according to the frontend side) and we will provide a feedback in the next few hours.

[frabacche]

hi @tdonohue

• configuration for blocking incoming LDN messages has been set. New properties: ldn.notify.inbox.block-untrusted and ldn.notify.inbox.block-untrusted-ip both true as default. Add IT classes for untrusted message blocking;
• CoarNotify section on item submission has been hidden;
• item-filters requested in restApi for the Notify Service configuration is now picked from a new map, hardcoded into item-filters.xml spring file;
• range IP check: we noticed that using Postman and pointing to server url such as "localhost" the request ip is on ipv6 and our check fails. (as already provided) the server url must be an explicit IPv4 such as 127.0.0.1. Java IT class has been updated with the range-IP-check test.

At this point we need to establish what are the best item-filters to show.

[frabacche]

we propose to fill the item-filter with these filters: Would this be ok? All the comments have received a feedback here: we are currently completing the works on the fronted PR.

[tdonohue]

@frabacche : that seems like a good improvement to me. Thanks! We just obviously need to document the meaning of each filter.

[frabacche]

@frabacche : that seems like a good improvement to me. Thanks! We just obviously need to document the meaning of each filter.

We just updated the documentation with a focus on the inbound pattern->item filter section.

[github-actions[bot]]

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

[tdonohue]

@frabacche : please rebase this so that it no longer has merge conflicts so that it can be tested again.

[tdonohue]

@frabacche : I gave this a code review today. It appears all my prior feedback is addressed. However, I'm unable to test or merge this until the merge conflict is resolved.

[tdonohue]

@frabacche : Thanks for rebasing this. Just a note, it appears to now be failing some Integration Tests in QAEventRestRepositoryIT. You may want to look into it, because the test failures are implying that some behavior has accidentally changed in this PR.

In the meantime, I will test this again today.. but obviously it cannot be merged with failing tests.

[steph-ieffam]

Hello @tdonohue, @frabacche has solved the merge conflict that came out yesterday. Now he's finalizing with the fix on integration tests. He's actively working on this PR

[frabacche]

Hi @tdonohue - tests IT are fine and all comments have been addressed.