-
Notifications
You must be signed in to change notification settings - Fork 495
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support deletion of saved searches #10198
Support deletion of saved searches #10198
Conversation
src/main/java/edu/harvard/iq/dataverse/search/savedsearch/SavedSearchServiceBean.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
… on SavedSearch API
Hello @pdurbin, everything has been added to the PR :
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@luddaniel overall this is looking good but I added a few questions.
src/main/java/edu/harvard/iq/dataverse/search/savedsearch/SavedSearchServiceBean.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just added some more feedback.
src/main/java/edu/harvard/iq/dataverse/search/savedsearch/SavedSearchServiceBean.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
return wr.getResponse(); | ||
} | ||
if (superuser == null || !superuser.isSuperuser()) { | ||
return error(Response.Status.UNAUTHORIZED, "Superusers only."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -5745,8 +5745,7 @@ The ``$identifier`` should start with an ``@`` if it's a user. Groups start with | |||
Saved Search | |||
~~~~~~~~~~~~ | |||
|
|||
The Saved Search, Linked Dataverses, and Linked Datasets features shipped with Dataverse 4.0, but as a "`superuser-only <https://github.com/IQSS/dataverse/issues/90#issuecomment-86094663>`_" because they are **experimental** (see `#1364 <https://github.com/IQSS/dataverse/issues/1364>`_, `#1813 <https://github.com/IQSS/dataverse/issues/1813>`_, `#1840 <https://github.com/IQSS/dataverse/issues/1840>`_, `#1890 <https://github.com/IQSS/dataverse/issues/1890>`_, `#1939 <https://github.com/IQSS/dataverse/issues/1939>`_, `#2167 <https://github.com/IQSS/dataverse/issues/2167>`_, `#2186 <https://github.com/IQSS/dataverse/issues/2186>`_, `#2053 <https://github.com/IQSS/dataverse/issues/2053>`_, and `#2543 <https://github.com/IQSS/dataverse/issues/2543>`_). The following API endpoints were added to help people with access to the "admin" API make use of these features in their current form. There is a known issue (`#1364 <https://github.com/IQSS/dataverse/issues/1364>`_) that once a link to a Dataverse collection or dataset is created, it cannot be removed (apart from database manipulation and reindexing) which is why a ``DELETE`` endpoint for saved searches is neither documented nor functional. The Linked Dataverse collections feature is `powered by Saved Search <https://github.com/IQSS/dataverse/issues/1852>`_ and therefore requires that the "makelinks" endpoint be executed on a periodic basis as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, I removed every authentication requirement for /api/admin/savedsearches
…dded SavedSearchIT to Jenkins CI
Co-authored-by: Philip Durbin <philipdurbin@gmail.com>
boolean wasDeleted = savedSearchSvc.delete(doomedId); | ||
boolean wasDeleted; | ||
try { | ||
wasDeleted = savedSearchSvc.delete(doomedId, unlink); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to put this in a command? There's a "CreateSavedSearchCommand"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
src/main/java/edu/harvard/iq/dataverse/search/savedsearch/SavedSearchServiceBean.java
Outdated
Show resolved
Hide resolved
src/main/java/edu/harvard/iq/dataverse/search/savedsearch/SavedSearchServiceBean.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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!
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. |
@luddaniel Please note my questions above and resolve the merge conflicts with our develop branch. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates and responses.
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 |
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 |
Nice feature. Seems to work fine. I tested it with this:
Merging! Thanks, @luddaniel! ❤️ |
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.
|
Thanks for the suggestions Phil! |
@DS-INRA I joined! Thanks! |
@pdurbin I added your suggestions in a new issue so we don't forget them :) : |
@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. 😄 😈 |
…-by-pdurbin Improvements on PR #10198
What this PR does / why we need it:
DELETE
/api/admin/savedsearches/{id}
?unlink=true
Which issue(s) this PR closes:
Is there a release notes update needed for this change?:
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 :
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 = 1curl -X DELETE http://localhost:8080/api/admin/savedsearches/1 | jq .
-> OKcurl http://localhost:8080/api/admin/savedsearches/list | jq .
-> no itemLinks 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 = 2curl -X DELETE http://localhost:8080/api/admin/savedsearches/2?unlink=true | jq .
-> OKcurl http://localhost:8080/api/admin/savedsearches/list | jq .
-> no itemLinks 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