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

Destroy Dataset with a Mapped DataFile #6860

Merged
merged 1 commit into from
May 4, 2020
Merged

Destroy Dataset with a Mapped DataFile #6860

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 8 additions & 0 deletions src/main/java/edu/harvard/iq/dataverse/DataFile.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import edu.harvard.iq.dataverse.util.FileUtil;
import edu.harvard.iq.dataverse.util.ShapefileHandler;
import edu.harvard.iq.dataverse.util.StringUtil;
import edu.harvard.iq.dataverse.worldmapauth.WorldMapToken;
import java.io.IOException;
import java.util.List;
import java.util.ArrayList;
Expand Down Expand Up @@ -214,6 +215,13 @@ public void setGuestbookResponses(List<GuestbookResponse> guestbookResponses) {
this.guestbookResponses = guestbookResponses;
}

// The WorldMap LayerMetadata and AuthToken are here to facilitate a
// clean cascade delete when the DataFile is deleted:
@OneToOne(mappedBy="dataFile", orphanRemoval = true, cascade={CascadeType.REMOVE, CascadeType.MERGE, CascadeType.PERSIST})
private MapLayerMetadata mapLayerMetadata;
@OneToMany(mappedBy="dataFile", orphanRemoval = true, cascade={CascadeType.REMOVE, CascadeType.MERGE, CascadeType.PERSIST})
private List<WorldMapToken> worldMapTokens;

private char ingestStatus = INGEST_STATUS_NONE;

@OneToOne(mappedBy = "thumbnailFile")
Expand Down
28 changes: 28 additions & 0 deletions src/main/java/edu/harvard/iq/dataverse/engine/command/impl/DeleteDataFileCommand.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,9 @@
import java.util.logging.Level;
import java.util.logging.Logger;
import edu.harvard.iq.dataverse.GlobalIdServiceBean;
import edu.harvard.iq.dataverse.UserNotification;
import java.sql.Timestamp;
import java.util.Date;

/**
* Deletes a data file, both DB entity and filesystem object.
Expand Down Expand Up @@ -210,6 +213,31 @@ public FileVisitResult postVisitDirectory(final Path dir, final IOException e)
} catch (Exception e) {
logger.log(Level.WARNING, "Identifier deletion was not successfull:", e.getMessage());
}

// If there is a Map Layer associated with this file, we may need to
// try and remove the layer data on the WorldMap side.
if (ctxt.mapLayerMetadata().findMetadataByDatafile(doomed) != null) {
// (We need an AuthenticatedUser in order to produce a WorldMap token!)
String id = getUser().getIdentifier();
id = id.startsWith("@") ? id.substring(1) : id;
AuthenticatedUser authenticatedUser = ctxt.authentication().getAuthenticatedUser(id);
try {
ctxt.mapLayerMetadata().deleteMapLayerFromWorldMap(doomed, authenticatedUser);

// We have the dedicatd command DeleteMapLayerMetadataCommand, but
// there's no need to use it explicitly, since the Dataverse-side
// MapLayerMetadata entity will be deleted by the database
// cascade on the DataFile. -- L.A. Apr. 2020
Copy link
Member

Choose a reason for hiding this comment

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

I had forgotten that there's a DeleteMapLayerMetadataCommand but this comment makes me wonder why we don't call it. I get that we don't need to call it thanks to the cascade, but would calling the command cause problems? Does it do more than we want? It seems to clean up thumbnails, for example. Also, there's tiny typo above: dedicatd.

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 still curious about these questions but there's no need to hold this up from QA so I moved it over. This pull request adds a lot of value.

Copy link
Contributor Author

@landreev landreev Apr 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry, missed the original question 4 hours ago. I guess the answer is, the delete layer command is a more surgical tool; it's for a case where we just need to delete the layer, without touching the file.
Calling it in the context of trying to wipe everything, including the datafile and all its associated physical files, would not hurt, strictly speaking - but I couldn't think of why we would want to call it either; seeing how the cascade is more reliable? It does remove the map thumbnails - but then we run a full physical file cleanup after DeleteDataFileCommand, that will delete all aux files associated with the datafile, thumbnails and otherwise.
And then again there is the fact the command doesn't do everything we need, either. It does not attempt to go out and remove whatever possible data may be sitting on the WorldMap side. I could only assume that it was by design.
My attitude with this PR was to check in something ASAP that works and doesn't break things.
I generally sympathize with the goals of making things cleaner; calling commands instead of database queries and such. But in the case of WorldMap, the whole is a big hack - isn't it? Our attempt to "think about it some more" to see if we were doing everything right resulted in a 2.5 year delay, and a whole bunch of these un-destructible datasets still sitting in production...

Copy link
Member

Choose a reason for hiding this comment

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

Yes, agreed. I don't think this will break anything and it definitely adds value. Thanks for the explanation.


} catch (Exception ex) {
// We are not going to treat it as a fatal condition and bail out,
// but we will send a notification to the user, warning them
// there may still be some data associated with the mapped layer,
// on the WorldMap side, un-deleted:
ctxt.notifications().sendNotification(authenticatedUser, new Timestamp(new Date().getTime()), UserNotification.Type.MAPLAYERDELETEFAILED, doomed.getFileMetadata().getId());
}

}
DataFile doomedAndMerged = ctxt.em().merge(doomed);
ctxt.em().remove(doomedAndMerged);
/**
Expand Down