Support deletion of saved searches

[luddaniel]

What this PR does / why we need it:

• Allow to delete saved search by calling DELETE /api/admin/savedsearches/{id}
• Allow to delete saved search and unlink links related to the deleted saved search by adding ?unlink=true

Which issue(s) this PR closes:

• Closes Allow deletion of saved searches #9317
• Closes Improving Saved Search feature #10628

Is there a release notes update needed for this change?:

• https://guides.dataverse.org/en/latest/api/native-api.html#saved-search must be updated
• Explain that deletion with unlink=true is not perfect and may be followed /makelinks/all depending on the need if other saved searches could recreate some deleted links or by reindexing some Dataverse or Dataset.

Suggestions on how to test this:

This is how I tested it :

Create Data

create 1 collection + 2 datasets under + publish all with subject "Agricultural Sciences"

Create saved search

create another collection that will be contain linked Data)

savedSearch.json contains :

{
    "query": "*",
    "creatorId": 1,
    "definitionPointId": 5,
    "filterQueries": [
        "subject_ss:Agricultural Sciences"
    ]
}

curl http://localhost:8080/api/admin/savedsearches -X POST -H 'Content-type:application/json' --upload-file savedSearch.json | jq .
curl -X PUT http://localhost:8080/api/admin/savedsearches/makelinks/all?debug=true | jq .

Now collection2 contains 3 linked Dvobject.

Remove without unlink

curl http://localhost:8080/api/admin/savedsearches/list | jq . -> 1 item id = 1
curl -X DELETE http://localhost:8080/api/admin/savedsearches/1 | jq . -> OK
curl http://localhost:8080/api/admin/savedsearches/list | jq . -> no item

Links are still here

Re-Create saved search

curl -s http://localhost:8080/api/admin/savedsearches -X POST -H 'Content-type:application/json' --upload-file savedSearch.json | jq .
curl -X PUT http://localhost:8080/api/admin/savedsearches/makelinks/all?debug=true | jq .

Remove with unlink

curl http://localhost:8080/api/admin/savedsearches/list | jq . -> 1 item id = 2
curl -X DELETE http://localhost:8080/api/admin/savedsearches/2?unlink=true | jq . -> OK
curl http://localhost:8080/api/admin/savedsearches/list | jq . -> no item

Links are deleted and not visible in collection2

Docs

Preview updated docs at https://dataverse-guide--10198.org.readthedocs.build/en/10198/api/native-api.html#saved-search

[coveralls]

coverage: 20.62% (-0.008%) from 20.628%
when pulling 008507b on Recherche-Data-Gouv:9317-delete-saved-search
into 9bfa6ac on IQSS:develop.

[pdurbin]

This is a great start! @luddaniel how do you feel about adding API tests, an update to the API Guide, and a release note snippet?

[luddaniel]

This is a great start! @luddaniel how do you feel about adding API tests, an update to the API Guide, and a release note snippet?

Hello @pdurbin, everything has been added to the PR :

• API Guide update
• Release note snippet
• Integration tests (which revealed that API endpoints were not restricted to Superusers only 🤣), tests were kept simple for now.
• Superusers only fix

[pdurbin]

@luddaniel overall this is looking good but I added a few questions.

[pdurbin]

I just added some more feedback.

[pdurbin]

@luddaniel thanks again for this pull request. I finally looked a bit closer the code and I'm concerned about all the backward-incompatible changes. Should we really do them now? Let's see what others on the team think.

Also, I'm out for the next two weeks so I'll move this PR back to "ready for review" and unassign myself so someone else can take a look.

[pdurbin]

Do we need this backward-incompatible change? I'm only going to leave this comment once but it applies to all endpoints that formerly were not superuser-only.

@luddaniel I know we talked about this a bit at https://dataverse.zulipchat.com/#narrow/stream/379673-dev/topic/admin.20API.20and.20superuser.20authorization/near/443079852 but my understanding was that we were talking theoretically about the future. I didn't realize you were thinking about making backward-incompatible changes now, in the name of consistency. We should probably pause and get rough consensus from more of the core team and decide on the timing for introducing these changes, if at all.

[pdurbin]

There's a conversation going on in #10628 about how these docs used to explain to people that they might have to wait up to 24 hours for their saved search to show results. Should we add that back as part of this pull request?

[luddaniel]

Updated with The update of all saved search is run by a timer once a week (See :ref:saved-search-timer) so if you just created a saved search, you can run manually makelinks endpoint that will find new dataverses and datasets that match the saved search and then link the search results to the dataverse in which the saved search is defined.

[pdurbin]

I'm going to leave comments below about backward incompatible changes. I'm pretty sure my preference is to NOT introduce any backward-incompatible changes but if we do the should be listed in doc/sphinx-guides/source/api/changelog.rst and the release notes should give an overview of backward-incompatible changes and tell people to check that changelog.

[luddaniel]

I agree, I removed every authentication requirement for /api/admin/savedsearches

[sekmiller]

Do we want to put this in a command? There's a "CreateSavedSearchCommand"

[luddaniel]

Hey @sekmiller, if my understanding is correct Command are used to share the same code between UI and API endpoints.

I note that CreateSavedSearchCommand is not used anymore (it was used in DataversePage.java for UI purposes in the past).
I note that adding Saved Search using POST API is not using CreateSavedSearchCommand.
I note that the code has been initiated back in 2015 and has been for a long time labelled experimental.

It could be prematured to create a Command that could be called from interface. What do you think ?
Sorry, but I'm more "quick win and no refactor while it's not needed" kind of mind.

[sekmiller]

One reason that I suggested the command was because commands also check for permissions. I didn't notice that this is a superuser only feature - so that permission check wouldn't be available in the command.
We can leave it be for now, but if we ever open it up to other users we might want to revisit the command issue.

[sekmiller]

@luddaniel I made a few comments throughout the code. The most important one is a suggestion that we add a Delete Saved Search command. Please take a look. Thanks!

[DS-INRAE]

Just a quick update on this PR : @luddaniel is currently in vacations, he'll be able to come back to this PR after August 19th.

[sekmiller]

@luddaniel Please note my questions above and resolve the merge conflicts with our develop branch. Thanks!

[sekmiller]

Thanks for the updates and responses.

[pdurbin]

Hmm, Jenkins hasn't run on this PR since June 14. I just kicked off a run manually: https://jenkins.dataverse.org/job/IQSS-Dataverse-Develop-PR/view/change-requests/job/PR-10198/

I did run mvn test -Dtest=SavedSearchIT locally, and it passed.

[pdurbin]

I just kicked off a run manually: https://jenkins.dataverse.org/job/IQSS-Dataverse-Develop-PR/view/change-requests/job/PR-10198/

The test show as failing but the API tests passed: https://jenkins.dataverse.org/job/IQSS-Dataverse-Develop-PR/job/PR-10198/14/testReport/

@donsizemore fixed the unrelated jacoco failure in gdcc/dataverse-ansible@9602ecc

[pdurbin]

Nice feature. Seems to work fine. I tested it with this:

curl -H "X-Dataverse-key:$API_TOKEN" -X DELETE "http://localhost:8080/api/admin/savedsearches/3?unlink=true"

Merging! Thanks, @luddaniel! ❤️

[pdurbin]

I'd be remiss if I didn't say that if I had access to push to https://github.com/Recherche-Data-Gouv I would have made the following small tweaks (and, if I had the energy, would have convert the old style API docs to the new style with environment variables).

This is all minor stuff though and not worth holding up progress. Perhaps it could go in a future pull request.

pdurbin@beamish dataverse % git diff --word-diff doc 
diff --git a/doc/sphinx-guides/source/api/native-api.rst b/doc/sphinx-guides/source/api/native-api.rst
index b16ea55bd2..9725602df4 100644
--- a/doc/sphinx-guides/source/api/native-api.rst
+++ b/doc/sphinx-guides/source/api/native-api.rst
@@ -5853,7 +5853,7 @@ Saved Search
~~~~~~~~~~~~

The Saved Search, Linked Dataverses, and Linked Datasets features are only accessible to superusers except for linking a dataset. The following API endpoints were added to help people with access to the "admin" API make use of these features in their current form. Keep in mind that they are partially experimental.
The update of all saved search is run by a timer once a week (See :ref:`saved-search-timer`) so if you just created a saved search, you can run manually {+the+} ``makelinks`` endpoint that will find new dataverses and datasets that match the saved search and then link the search results to the dataverse in which the saved search is defined.

List all saved searches. ::

@@ -5865,7 +5865,7 @@ List a saved search by database id. ::

Delete a saved search by database id.

The ``unlink=true`` query parameter unlinks all links (linked dataset or Dataverse collection) associated with the deleted saved search. Use of this parameter should be well considered as you cannot know if the links were created manually or by the saved search. After deleting a saved search with ``unlink=true``, we recommend running ``/makelinks/all`` just in case there was a dataset that was linked by another saved search. (Saved searches can link the same dataset.) Reindexing might be necessary as [-well.::-]{+well. ::+}

  DELETE http://$SERVER/api/admin/savedsearches/$id?unlink=true

pdurbin@beamish dataverse % git diff src            
diff --git a/src/main/java/edu/harvard/iq/dataverse/api/SavedSearches.java b/src/main/java/edu/harvard/iq/dataverse/api/SavedSearches.java
index 33a11a2df2..e6519c9ff3 100644
--- a/src/main/java/edu/harvard/iq/dataverse/api/SavedSearches.java
+++ b/src/main/java/edu/harvard/iq/dataverse/api/SavedSearches.java
@@ -181,7 +181,7 @@ public class SavedSearches extends AbstractApiBean {
         try {
             wasDeleted = savedSearchSvc.delete(doomedId, unlink);
         } catch (Exception e) {
-            return error(INTERNAL_SERVER_ERROR, "Problem while trying to unlink links of saved search id " + doomedId);
+            return error(INTERNAL_SERVER_ERROR, "Problem while trying to unlink links of saved search id " + doomedId + ". Exception: " + e.getLocalizedMessage());
         }
 
         if (wasDeleted) {
diff --git a/src/test/java/edu/harvard/iq/dataverse/api/SavedSearchIT.java b/src/test/java/edu/harvard/iq/dataverse/api/SavedSearchIT.java
index 90357596c2..cdcd92d930 100644
--- a/src/test/java/edu/harvard/iq/dataverse/api/SavedSearchIT.java
+++ b/src/test/java/edu/harvard/iq/dataverse/api/SavedSearchIT.java
@@ -22,7 +22,7 @@ public class SavedSearchIT {
 
     @BeforeAll
     public static void setUpClass() {
-
+        RestAssured.baseURI = UtilIT.getRestAssuredBaseUri();
     }
 
     @AfterAll
@@ -52,6 +52,7 @@ public class SavedSearchIT {
         createDatasetResponse2.prettyPrint();
         Integer datasetId2 = UtilIT.getDatasetIdFromResponse(createDatasetResponse2);
 
+        // TODO: put all these methods in UtilIT
         // missing body
         Response resp = RestAssured.given()
                 .contentType("application/json")
pdurbin@beamish dataverse % 

[DS-INRAE]

Thanks for the suggestions Phil!
We'll address them in a future PR indeed.
Just in case for future PRs, I've also invited you to Recherche-Data-Gouv :)

[pdurbin]

@DS-INRA I joined! Thanks!

[DS-INRAE]

@pdurbin I added your suggestions in a new issue so we don't forget them :) :

• Improvements on PR 10198 #10893

[pdurbin]

@DS-INRAE thanks! No big deal. It's minor stuff. And next time I'll push little changes like this myself before merging, now that you've given me access. 😄 😈