Skip to content

Commit c6a28fa

Browse files
labkey-adamlabkey-adam
committed
Check for orphans before deleting containers
1 parent 9ccf24e commit c6a28fa

4 files changed

Lines changed: 62 additions & 65 deletions

File tree

‎api/src/org/labkey/api/attachments/AttachmentService.java‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -140,6 +140,8 @@ static AttachmentService get()
140140

141141
HttpView<?> getFindAttachmentParentsView();
142142

143+
void detectOrphans(Container c);
144+
143145
class DuplicateFilenameException extends IOException implements SkipMothershipLogging
144146
{
145147
private final List<String> _errors = new ArrayList<>();

‎api/src/org/labkey/api/data/ContainerManager.java‎

Lines changed: 21 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@
3838
import org.labkey.api.admin.FolderWriterImpl;
3939
import org.labkey.api.admin.StaticLoggerGetter;
4040
import org.labkey.api.attachments.AttachmentParent;
41+
import org.labkey.api.attachments.AttachmentService;
4142
import org.labkey.api.audit.AuditLogService;
4243
import org.labkey.api.audit.AuditTypeEvent;
4344
import org.labkey.api.audit.provider.ContainerAuditProvider;
@@ -422,6 +423,7 @@ public static Container createContainerFromTemplate(Container parent, String nam
422423

423424
// import objects into the target folder
424425
XmlObject folderXml = vf.getXmlBean("folder.xml");
426+
425427
if (folderXml instanceof FolderDocument folderDoc)
426428
{
427429
FolderImportContext importCtx = new FolderImportContext(user, c, folderDoc, null, new StaticLoggerGetter(LogManager.getLogger(FolderImporterImpl.class)), vf);
@@ -1539,7 +1541,7 @@ else if (hasAncestryRead)
15391541
}
15401542

15411543
if (!addFolder)
1542-
LOG.debug("isNavAccessOpen restriction: \"" + f.getPath() + "\"");
1544+
LOG.debug("isNavAccessOpen restriction: \"{}\"", f.getPath());
15431545
}
15441546

15451547
if (addFolder)
@@ -1915,7 +1917,7 @@ private static boolean delete(final Container c, User user, @Nullable String com
19151917
throw new IllegalStateException("Container not flagged as being deleted: " + c.getPath());
19161918
}
19171919

1918-
LOG.debug("Starting container delete for " + c.getContainerNoun(true) + " " + c.getPath());
1920+
LOG.debug("Starting container delete for {} {}", c.getContainerNoun(true), c.getPath());
19191921

19201922
// Tell the search indexer to drop work for the container that's about to be deleted
19211923
SearchService.get().purgeForContainer(c);
@@ -1942,6 +1944,8 @@ private static boolean delete(final Container c, User user, @Nullable String com
19421944
setContainerTabDeleted(c.getParent(), c.getName(), c.getParent().getFolderType().getName());
19431945
}
19441946

1947+
AttachmentService.get().detectOrphans(c);
1948+
19451949
fireDeleteContainer(c, user);
19461950

19471951
SqlExecutor sqlExecutor = new SqlExecutor(CORE.getSchema());
@@ -1981,11 +1985,11 @@ private static boolean delete(final Container c, User user, @Nullable String com
19811985
boolean success = CORE.getSchema().getScope().executeWithRetry(tryDeleteContainer);
19821986
if (success)
19831987
{
1984-
LOG.debug("Completed container delete for " + c.getContainerNoun(true) + " " + c.getPath());
1988+
LOG.debug("Completed container delete for {} {}", c.getContainerNoun(true), c.getPath());
19851989
}
19861990
else
19871991
{
1988-
LOG.warn("Failed to delete container: " + c.getPath());
1992+
LOG.warn("Failed to delete container: {}", c.getPath());
19891993
}
19901994
return success;
19911995
}
@@ -2025,13 +2029,13 @@ public static void deleteAll(Container root, User user, @Nullable String comment
20252029
if (!hasTreePermission(root, user, DeletePermission.class))
20262030
throw new UnauthorizedException("You don't have delete permissions to all folders");
20272031

2028-
LOG.debug("Starting container (and children) delete for " + root.getContainerNoun(true) + " " + root.getPath());
2032+
LOG.debug("Starting container (and children) delete for {} {}", root.getContainerNoun(true), root.getPath());
20292033
Set<Container> depthFirst = getAllChildrenDepthFirst(root);
20302034
depthFirst.add(root);
20312035

20322036
delete(depthFirst, user, comment);
20332037

2034-
LOG.debug("Completed container (and children) delete for " + root.getContainerNoun(true) + " " + root.getPath());
2038+
LOG.debug("Completed container (and children) delete for {} {}", root.getContainerNoun(true), root.getPath());
20352039
}
20362040

20372041
public static void deleteAll(Container root, User user) throws UnauthorizedException
@@ -2426,7 +2430,7 @@ protected static void fireCreateContainer(Container c, User user, @Nullable Stri
24262430
}
24272431
catch (Throwable t)
24282432
{
2429-
LOG.error("fireCreateContainer for " + cl.getClass().getName(), t);
2433+
LOG.error("fireCreateContainer for {}", cl.getClass().getName(), t);
24302434
}
24312435
}
24322436
}
@@ -2490,7 +2494,7 @@ public static void firePropertyChangeEvent(ContainerPropertyChangeEvent evt)
24902494
}
24912495
catch (Throwable t)
24922496
{
2493-
LOG.error("firePropertyChangeEvent for " + l.getClass().getName(), t);
2497+
LOG.error("firePropertyChangeEvent for {}", l.getClass().getName(), t);
24942498
}
24952499
}
24962500
}
@@ -2523,8 +2527,8 @@ public static Container createDefaultSupportContainer()
25232527
// create a "support" container. Admins can do anything,
25242528
// Users can read/write, Guests can read.
25252529
return bootstrapContainer(DEFAULT_SUPPORT_PROJECT_PATH,
2526-
RoleManager.getRole(AuthorRole.class),
2527-
RoleManager.getRole(ReaderRole.class)
2530+
RoleManager.getRole(AuthorRole.class),
2531+
RoleManager.getRole(ReaderRole.class)
25282532
);
25292533
}
25302534

@@ -2693,7 +2697,7 @@ public static Container bootstrapContainer(String path, @NotNull Role userRole,
26932697

26942698
if (c == null)
26952699
{
2696-
LOG.debug("Creating new container for path '" + path + "'");
2700+
LOG.debug("Creating new container for path '{}'", path);
26972701
newContainer = true;
26982702
c = ensureContainer(path, user);
26992703
}
@@ -2710,7 +2714,7 @@ public static Container bootstrapContainer(String path, @NotNull Role userRole,
27102714

27112715
if (newContainer || 0 == policyCount.intValue())
27122716
{
2713-
LOG.debug("Setting permissions for '" + path + "'");
2717+
LOG.debug("Setting permissions for '{}'", path);
27142718
MutableSecurityPolicy policy = new MutableSecurityPolicy(c);
27152719
policy.addRoleAssignment(SecurityManager.getGroup(Group.groupUsers), userRole);
27162720
if (guestRole != null)
@@ -2881,25 +2885,25 @@ public void testFolderType()
28812885

28822886
private void testOneFolderType(FolderType folderType)
28832887
{
2884-
LOG.info("testOneFolderType(" + folderType.getName() + "): creating container");
2888+
LOG.info("testOneFolderType({}): creating container", folderType.getName());
28852889
Container newFolder = createContainer(_testRoot, "folderTypeTest", TestContext.get().getUser());
28862890
FolderType ft = newFolder.getFolderType();
28872891
assertEquals(FolderType.NONE, ft);
28882892

28892893
Container newFolderFromCache = getForId(newFolder.getId());
28902894
assertNotNull(newFolderFromCache);
28912895
assertEquals(FolderType.NONE, newFolderFromCache.getFolderType());
2892-
LOG.info("testOneFolderType(" + folderType.getName() + "): setting folder type");
2896+
LOG.info("testOneFolderType({}): setting folder type", folderType.getName());
28932897
newFolder.setFolderType(folderType, TestContext.get().getUser());
28942898

28952899
newFolderFromCache = getForId(newFolder.getId());
28962900
assertNotNull(newFolderFromCache);
28972901
assertEquals(newFolderFromCache.getFolderType().getName(), folderType.getName());
28982902
assertEquals(newFolderFromCache.getFolderType().getDescription(), folderType.getDescription());
28992903

2900-
LOG.info("testOneFolderType(" + folderType.getName() + "): deleteAll");
2904+
LOG.info("testOneFolderType({}): deleteAll", folderType.getName());
29012905
deleteAll(newFolder, TestContext.get().getUser()); // There might be subfolders because of container tabs
2902-
LOG.info("testOneFolderType(" + folderType.getName() + "): deleteAll complete");
2906+
LOG.info("testOneFolderType({}): deleteAll complete", folderType.getName());
29032907
Container deletedContainer = getForId(newFolder.getId());
29042908

29052909
if (deletedContainer != null)
@@ -2946,7 +2950,7 @@ private static void logNode(MultiValuedMap<String, String> mm, String name, int
29462950

29472951
for (String childName : nodes)
29482952
{
2949-
LOG.debug(StringUtils.repeat(" ", offset) + childName);
2953+
LOG.debug("{}{}", StringUtils.repeat(" ", offset), childName);
29502954
logNode(mm, childName, offset + 1);
29512955
}
29522956
}

‎core/src/org/labkey/core/attachment/AttachmentContainerListener.java‎

Lines changed: 0 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -1,61 +1,16 @@
11
package org.labkey.core.attachment;
22

3-
import org.apache.logging.log4j.Logger;
4-
import org.labkey.api.collections.CsvSet;
5-
import org.labkey.api.data.CompareType;
63
import org.labkey.api.data.Container;
74
import org.labkey.api.data.ContainerManager.ContainerListener;
85
import org.labkey.api.data.CoreSchema;
9-
import org.labkey.api.data.SimpleFilter;
10-
import org.labkey.api.data.TableInfo;
11-
import org.labkey.api.data.TableSelector;
12-
import org.labkey.api.query.FieldKey;
136
import org.labkey.api.security.User;
14-
import org.labkey.api.settings.AppProps;
157
import org.labkey.api.util.ContainerUtil;
16-
import org.labkey.api.util.StringUtilsLabKey;
17-
import org.labkey.api.util.logging.LogHelper;
18-
19-
import java.util.List;
20-
import java.util.stream.Collectors;
218

229
public class AttachmentContainerListener implements ContainerListener
2310
{
24-
private static final Logger LOG = LogHelper.getLogger(AttachmentContainerListener.class, "Reporting orphaned attachments");
25-
26-
private record Orphan(String documentName, String parentType){}
27-
2811
@Override
2912
public void containerDeleted(Container c, User user)
3013
{
31-
TableInfo table = CoreSchema.getInstance().getTableInfoDocuments();
32-
// Log orphaned attachments in this container, but in dev mode only, since this is for our testing. Also, we
33-
// don't yet offer a way to delete orphaned attachments via the UI, so it's not helpful to inform admins.
34-
if (AppProps.getInstance().isDevMode())
35-
{
36-
// Find all attachments in this container that don't use the container itself as the parent (since those
37-
// are the responsibility of this container listener).
38-
SimpleFilter filter = new SimpleFilter(FieldKey.fromParts("Container"), c.getId());
39-
filter.addCondition(FieldKey.fromParts("Parent"), c.getId(), CompareType.NEQ);
40-
List<Orphan> orphans = new TableSelector(table, new CsvSet("DocumentName, ParentType"), filter, null).getArrayList(Orphan.class);
41-
if (!orphans.isEmpty())
42-
{
43-
LOG.error("Found {} in this container, which likely indicates a problem with a delete method or a container listener.", StringUtilsLabKey.pluralize(orphans.size(), "orphaned attachment"));
44-
45-
final String message;
46-
if (orphans.size() > 20)
47-
{
48-
orphans = orphans.subList(0, 20);
49-
message = "The first 20";
50-
}
51-
else
52-
{
53-
message = "All";
54-
}
55-
56-
LOG.error("{} detected orphans are listed below:\n{}", message, orphans.stream().map(Record::toString).collect(Collectors.joining("\n")));
57-
}
58-
}
5914
ContainerUtil.purgeTable(CoreSchema.getInstance().getTableInfoDocuments(), c, null);
6015
AttachmentCache.removeAttachments(c);
6116
}

‎core/src/org/labkey/core/attachment/AttachmentServiceImpl.java‎

Lines changed: 39 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
import org.apache.commons.io.IOUtils;
2222
import org.apache.commons.lang3.StringUtils;
2323
import org.apache.commons.lang3.Strings;
24+
import org.apache.logging.log4j.Logger;
2425
import org.jetbrains.annotations.NotNull;
2526
import org.jetbrains.annotations.Nullable;
2627
import org.junit.Assert;
@@ -37,6 +38,7 @@
3738
import org.labkey.api.audit.AuditLogService;
3839
import org.labkey.api.audit.provider.FileSystemAuditProvider;
3940
import org.labkey.api.collections.CaseInsensitiveHashSet;
41+
import org.labkey.api.collections.CsvSet;
4042
import org.labkey.api.collections.LabKeyCollectors;
4143
import org.labkey.api.collections.Sets;
4244
import org.labkey.api.data.ColumnInfo;
@@ -79,7 +81,6 @@
7981
import org.labkey.api.security.permissions.Permission;
8082
import org.labkey.api.settings.AppProps;
8183
import org.labkey.api.test.TestWhen;
82-
import org.labkey.api.util.ContainerUtil;
8384
import org.labkey.api.util.FileStream;
8485
import org.labkey.api.util.FileUtil;
8586
import org.labkey.api.util.GUID;
@@ -91,8 +92,10 @@
9192
import org.labkey.api.util.Path;
9293
import org.labkey.api.util.ResponseHelper;
9394
import org.labkey.api.util.ResultSetUtil;
95+
import org.labkey.api.util.StringUtilsLabKey;
9496
import org.labkey.api.util.TestContext;
9597
import org.labkey.api.util.URLHelper;
98+
import org.labkey.api.util.logging.LogHelper;
9699
import org.labkey.api.view.ActionURL;
97100
import org.labkey.api.view.HttpView;
98101
import org.labkey.api.view.JspView;
@@ -136,9 +139,11 @@
136139
import java.util.Objects;
137140
import java.util.Set;
138141
import java.util.TreeSet;
142+
import java.util.stream.Collectors;
139143

140144
public class AttachmentServiceImpl implements AttachmentService
141145
{
146+
private static final Logger LOG = LogHelper.getLogger(AttachmentServiceImpl.class, "");
142147
private static final String UPLOAD_LOG = ".upload.log";
143148
private static final Map<String, AttachmentParentType> ATTACHMENT_TYPE_MAP = new HashMap<>();
144149
private static final Set<String> ATTACHMENT_COLUMNS = Set.of("Parent", "Container", "DocumentName", "DocumentSize", "DocumentType", "Created", "CreatedBy", "LastIndexed");
@@ -177,7 +182,6 @@ public void download(HttpServletResponse response, AttachmentParent parent, Stri
177182
}
178183
}
179184

180-
181185
@Override
182186
public void download(HttpServletResponse response, AttachmentParent parent, String filename, boolean inlineIfPossible) throws ServletException, IOException
183187
{
@@ -994,7 +998,6 @@ public void writeDocument(DocumentWriter writer, AttachmentParent parent, String
994998
writeDocument(writer, parent, name, null, asAttachment);
995999
}
9961000

997-
9981001
@Override
9991002
@NotNull
10001003
public InputStream getInputStream(AttachmentParent parent, String name) throws FileNotFoundException
@@ -1085,6 +1088,39 @@ public int available()
10851088
}
10861089
}
10871090

1091+
private record Orphan(String documentName, String parentType){}
1092+
1093+
@Override
1094+
public void detectOrphans(Container c)
1095+
{
1096+
TableInfo table = CoreSchema.getInstance().getTableInfoDocuments();
1097+
// Log orphaned attachments in this container, but in dev mode only, since this is for our testing. Also, we
1098+
// don't yet offer a way to delete orphaned attachments via the UI, so it's not helpful to inform admins.
1099+
if (AppProps.getInstance().isDevMode())
1100+
{
1101+
// Find all attachments in this container that don't use the container itself as the parent (since those
1102+
// are the responsibility of this container listener).
1103+
SimpleFilter filter = new SimpleFilter(FieldKey.fromParts("Container"), c.getId());
1104+
List<Orphan> orphans = new TableSelector(table, new CsvSet("DocumentName, ParentType"), filter, null).getArrayList(Orphan.class);
1105+
if (!orphans.isEmpty())
1106+
{
1107+
LOG.error("Found {} in this container, which likely indicates a problem with a delete method or a container listener.", StringUtilsLabKey.pluralize(orphans.size(), "orphaned attachment"));
1108+
1109+
final String message;
1110+
if (orphans.size() > 20)
1111+
{
1112+
orphans = orphans.subList(0, 20);
1113+
message = "The first 20";
1114+
}
1115+
else
1116+
{
1117+
message = "All";
1118+
}
1119+
1120+
LOG.error("{} detected orphans are listed below:\n{}", message, orphans.stream().map(Record::toString).collect(Collectors.joining("\n")));
1121+
}
1122+
}
1123+
}
10881124

10891125
private CoreSchema coreTables()
10901126
{

0 commit comments

Comments
 (0)