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.

Sign up for GitHub

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

Jump to bottom

Support deletion of saved searches #10198

Merged
merged 16 commits into from
Aug 27, 2024
Merged

Support deletion of saved searches #10198

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
2dc84db
#9317 - Allowing to delete saved search
luddaniel Dec 20, 2023
ac80e0a
#9317 - Adding release note
luddaniel Jan 5, 2024
a50f963
#9317 - Updated API Guide
luddaniel Jan 31, 2024
98f7b53
Merge branch 'develop' into 9317-delete-saved-search
luddaniel Jan 31, 2024
4fabfec
#9317 - Adding Integration Tests + Superusers only security check on …
luddaniel Mar 6, 2024
0dc2ea9
Fixed wrong asset class import
luddaniel Jun 11, 2024
b6f4b7f
Merge branch 'develop' into 9317-delete-saved-search
luddaniel Jun 11, 2024
614da60
Simplify assertion
luddaniel Jun 11, 2024
6836366
Added suggested doc modification
luddaniel Jun 13, 2024
633cd6c
Try to fix guide doc issue
luddaniel Jun 13, 2024
92e61fc
Removed Authentication requirement for /api/admin/savedsearches and a…
luddaniel Jul 3, 2024
4061c08
Apply suggestions from code review
luddaniel Jul 3, 2024
5f2de4a
Turned println to logger.fine + renamed a method + add java doc + upd…
luddaniel Jul 3, 2024
008507b
Merge branch 'develop' into 9317-delete-saved-search
luddaniel Jul 4, 2024
8acfdc9
Merge branch 'develop' into 9317-delete-saved-search
luddaniel Aug 23, 2024
4022c9f
Removed unused imports and switch some fine loggers to info
luddaniel Aug 23, 2024
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
14 changes: 8 additions & 6 deletions src/main/java/edu/harvard/iq/dataverse/api/SavedSearches.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,16 +173,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 •
edited
Loading

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
62 changes: 53 additions & 9 deletions src/main/java/edu/harvard/iq/dataverse/search/savedsearch/SavedSearchServiceBean.java
View file
Open in desktop
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 @@ -101,11 +110,15 @@ public SavedSearch add(SavedSearch toPersist) {
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());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While we're in this part of the code, should we delete this println? Or maybe convert it to our regular logger and set the level to "fine"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

if(unlink) {
DataverseRequest dataverseRequest = new DataverseRequest(doomed.getCreator(), getHttpServletRequest());
unLinksForSingleSavedSearch(dataverseRequest, doomed);
}
em.remove(doomed);
em.flush();
wasDeleted = true;
Expand Down Expand Up @@ -240,6 +253,37 @@ public JsonObjectBuilder makeLinksForSingleSavedSearch(DataverseRequest dvReq, S
return response;
}

public void unLinksForSingleSavedSearch(DataverseRequest dvReq, SavedSearch savedSearch) throws SearchException, CommandException {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't love this method name. unLinkForSingleSavedSearch might be better. Or removeLinkForSingleSavedSearch?

I'm not even sure we need "single" in there. I probably added it back in the day for a reason but now it seems verbose. 🤷

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 was too focused on mimicry the makeLinksForSingleSavedSearch method name, I renamed it removeLinks and added a java doc.

logger.info("UNLINK SAVED SEARCH (" + savedSearch.getId() + ") START search and unlink process");
luddaniel marked this conversation as resolved.
Show resolved Hide resolved
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.info("UNLINK SAVED SEARCH (" + savedSearch.getId() + ") total time in ms: " + (new Date().getTime() - start.getTime()));
luddaniel marked this conversation as resolved.
Show resolved Hide resolved
}

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