Skip to content
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

Merged
merged 16 commits into from
Aug 27, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions doc/release-notes/9317-delete-saved-search.md
Copy link
Member

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.

Copy link
Contributor Author

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

Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
### Saved search deletion

Saved searches can now be removed using API `/api/admin/savedsearches/$id`. See PR #10198.
This is reflected in the [Saved Search Native API section](https://dataverse-guide--10198.org.readthedocs.build/en/10198/api/native-api.html#saved-search) of the Guide.
9 changes: 8 additions & 1 deletion doc/sphinx-guides/source/api/native-api.rst
Original file line number Diff line number Diff line change
Expand Up @@ -5852,7 +5852,8 @@ 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.
Copy link
Member

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?

Copy link
Contributor Author

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.

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.
luddaniel marked this conversation as resolved.
Show resolved Hide resolved
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.

List all saved searches. ::

Expand All @@ -5862,6 +5863,12 @@ List a saved search by database id. ::

GET http://$SERVER/api/admin/savedsearches/$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.::

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

Execute a saved search by database id and make links to Dataverse collections and datasets that are found. The JSON response indicates which Dataverse collections and datasets were newly linked versus already linked. The ``debug=true`` query parameter adds to the JSON response extra information about the saved search being executed (which you could also get by listing the saved search). ::

PUT http://$SERVER/api/admin/savedsearches/makelinks/$id?debug=true
Expand Down
19 changes: 10 additions & 9 deletions src/main/java/edu/harvard/iq/dataverse/api/SavedSearches.java
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ public Response list() {
savedSearchesBuilder.add(thisSavedSearch);
}
JsonObjectBuilder response = Json.createObjectBuilder();
response.add("saved searches", savedSearchesBuilder);
response.add("savedSearches", savedSearchesBuilder);
luddaniel marked this conversation as resolved.
Show resolved Hide resolved
return ok(response);
}

Expand Down Expand Up @@ -90,7 +90,6 @@ private JsonObjectBuilder toJson(SavedSearch savedSearch) {

@POST
public Response add(JsonObject body) {

if (body == null) {
return error(BAD_REQUEST, "JSON is expected.");
}
Expand Down Expand Up @@ -159,7 +158,7 @@ public Response add(JsonObject body) {

try {
SavedSearch persistedSavedSearch = savedSearchSvc.add(toPersist);
return ok("Added: " + persistedSavedSearch);
return ok("Added: " + persistedSavedSearch, Json.createObjectBuilder().add("id", persistedSavedSearch.getId()));
luddaniel marked this conversation as resolved.
Show resolved Hide resolved
} catch (EJBException ex) {
StringBuilder errors = new StringBuilder();
Throwable throwable = ex.getCause();
Expand All @@ -173,16 +172,18 @@ public Response add(JsonObject body) {

@DELETE
@Path("{id}")
public Response delete(@PathParam("id") long doomedId) {
boolean disabled = true;
if (disabled) {
return error(BAD_REQUEST, "Saved Searches can not safely be deleted because links can not safely be deleted. See https://github.com/IQSS/dataverse/issues/1364 for details.");
}
public Response delete(@PathParam("id") long doomedId, @QueryParam("unlink") boolean unlink) {
SavedSearch doomed = savedSearchSvc.find(doomedId);
if (doomed == null) {
return error(NOT_FOUND, "Could not find saved search id " + doomedId);
}
boolean wasDeleted = savedSearchSvc.delete(doomedId);
boolean wasDeleted;
try {
wasDeleted = savedSearchSvc.delete(doomedId, unlink);
Copy link
Contributor

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"

Copy link
Contributor Author

@luddaniel luddaniel Aug 23, 2024

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.

Copy link
Contributor

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.

} catch (Exception e) {
return error(INTERNAL_SERVER_ERROR, "Problem while trying to unlink links of saved search id " + doomedId);
luddaniel marked this conversation as resolved.
Show resolved Hide resolved
}

if (wasDeleted) {
return ok(Json.createObjectBuilder().add("Deleted", doomedId));
} else {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,29 +2,28 @@

import edu.harvard.iq.dataverse.Dataset;
import edu.harvard.iq.dataverse.DatasetLinkingDataverse;
import edu.harvard.iq.dataverse.DatasetLinkingServiceBean;
import edu.harvard.iq.dataverse.Dataverse;
import edu.harvard.iq.dataverse.DataverseLinkingDataverse;
import edu.harvard.iq.dataverse.DataverseLinkingServiceBean;
import edu.harvard.iq.dataverse.DvObject;
import edu.harvard.iq.dataverse.DvObjectServiceBean;
import edu.harvard.iq.dataverse.EjbDataverseEngine;
import edu.harvard.iq.dataverse.authorization.users.AuthenticatedUser;
import edu.harvard.iq.dataverse.authorization.users.GuestUser;
import edu.harvard.iq.dataverse.engine.command.DataverseRequest;
import edu.harvard.iq.dataverse.search.SearchServiceBean;
import edu.harvard.iq.dataverse.search.SolrQueryResponse;
import edu.harvard.iq.dataverse.search.SolrSearchResult;
import edu.harvard.iq.dataverse.engine.command.exception.CommandException;
import edu.harvard.iq.dataverse.engine.command.impl.DeleteDatasetLinkingDataverseCommand;
import edu.harvard.iq.dataverse.engine.command.impl.DeleteDataverseLinkingDataverseCommand;
import edu.harvard.iq.dataverse.engine.command.impl.LinkDatasetCommand;
import edu.harvard.iq.dataverse.engine.command.impl.LinkDataverseCommand;
import edu.harvard.iq.dataverse.search.SearchException;
import edu.harvard.iq.dataverse.search.SearchFields;
import edu.harvard.iq.dataverse.search.SearchServiceBean;
import edu.harvard.iq.dataverse.search.SolrQueryResponse;
import edu.harvard.iq.dataverse.search.SolrSearchResult;
import edu.harvard.iq.dataverse.search.SortBy;
import edu.harvard.iq.dataverse.util.SystemConfig;
import java.util.ArrayList;
import java.util.Date;
import java.util.List;
import java.util.logging.Level;
import java.util.logging.Logger;
import jakarta.ejb.EJB;
import jakarta.ejb.Schedule;
import jakarta.ejb.Stateless;
Expand All @@ -39,6 +38,12 @@
import jakarta.persistence.TypedQuery;
import jakarta.servlet.http.HttpServletRequest;

import java.util.ArrayList;
import java.util.Date;
import java.util.List;
import java.util.logging.Level;
import java.util.logging.Logger;

@Stateless
@Named
public class SavedSearchServiceBean {
Expand All @@ -50,6 +55,10 @@ public class SavedSearchServiceBean {
@EJB
DvObjectServiceBean dvObjectService;
@EJB
protected DatasetLinkingServiceBean dsLinkingService;
@EJB
protected DataverseLinkingServiceBean dvLinkingService;
@EJB
EjbDataverseEngine commandEngine;
@EJB
SystemConfig systemConfig;
Expand Down Expand Up @@ -96,21 +105,25 @@ public SavedSearch add(SavedSearch toPersist) {
try {
persisted = em.merge(toPersist);
} catch (Exception ex) {
System.out.println("exeption: " + ex);
logger.fine("Failed to add SavedSearch" + ex);
}
return persisted;
}

public boolean delete(long id) {
public boolean delete(long id, boolean unlink) throws SearchException, CommandException {
SavedSearch doomed = find(id);
boolean wasDeleted = false;
if (doomed != null) {
System.out.println("deleting saved search id " + doomed.getId());
logger.info("Deleting saved search id " + doomed.getId());
if(unlink) {
DataverseRequest dataverseRequest = new DataverseRequest(doomed.getCreator(), getHttpServletRequest());
removeLinks(dataverseRequest, doomed);
}
em.remove(doomed);
em.flush();
wasDeleted = true;
} else {
System.out.println("problem deleting saved search id " + id);
logger.info("Problem deleting saved search id " + id);
}
return wasDeleted;
}
Expand Down Expand Up @@ -240,6 +253,45 @@ public JsonObjectBuilder makeLinksForSingleSavedSearch(DataverseRequest dvReq, S
return response;
}

/**
* This method to the reverse of a makeLinksForSingleSavedSearch method.
* It removes all Dataset and Dataverse links that match savedSearch's query.
* @param dvReq
* @param savedSearch
* @throws SearchException
* @throws CommandException
*/
public void removeLinks(DataverseRequest dvReq, SavedSearch savedSearch) throws SearchException, CommandException {
logger.fine("UNLINK SAVED SEARCH (" + savedSearch.getId() + ") START search and unlink process");
Date start = new Date();
Dataverse linkingDataverse = savedSearch.getDefinitionPoint();

SolrQueryResponse queryResponse = findHits(savedSearch);
for (SolrSearchResult solrSearchResult : queryResponse.getSolrSearchResults()) {

DvObject dvObjectThatDefinitionPointWillLinkTo = dvObjectService.findDvObject(solrSearchResult.getEntityId());
if (dvObjectThatDefinitionPointWillLinkTo == null) {
continue;
}

if (dvObjectThatDefinitionPointWillLinkTo.isInstanceofDataverse()) {
Dataverse linkedDataverse = (Dataverse) dvObjectThatDefinitionPointWillLinkTo;
DataverseLinkingDataverse dvld = dvLinkingService.findDataverseLinkingDataverse(linkedDataverse.getId(), linkingDataverse.getId());
if(dvld != null) {
Dataverse dv = commandEngine.submitInNewTransaction(new DeleteDataverseLinkingDataverseCommand(dvReq, linkingDataverse, dvld, true));
luddaniel marked this conversation as resolved.
Show resolved Hide resolved
}
} else if (dvObjectThatDefinitionPointWillLinkTo.isInstanceofDataset()) {
Dataset linkedDataset = (Dataset) dvObjectThatDefinitionPointWillLinkTo;
DatasetLinkingDataverse dsld = dsLinkingService.findDatasetLinkingDataverse(linkedDataset.getId(), linkingDataverse.getId());
if(dsld != null) {
Dataset ds = commandEngine.submitInNewTransaction(new DeleteDatasetLinkingDataverseCommand(dvReq, linkedDataset, dsld, true));
}
}
}

logger.fine("UNLINK SAVED SEARCH (" + savedSearch.getId() + ") total time in ms: " + (new Date().getTime() - start.getTime()));
}

private SolrQueryResponse findHits(SavedSearch savedSearch) throws SearchException {
String sortField = SearchFields.TYPE; // first return dataverses, then datasets
String sortOrder = SortBy.DESCENDING;
Expand Down
Loading
Loading