From c7e4c5c8dcfafdad49bb1ab9dc2c007d5f43c25b Mon Sep 17 00:00:00 2001 From: Jose Castro Date: Fri, 22 Mar 2024 12:18:48 -0600 Subject: [PATCH] fix(sites): Unable to Delete Site with Content Types and Contentlet #27862 (#28044) * fix(sites) #27862 : Unable to Delete Site with Content Types and Contentlet * Testing code change in Integration Test. * Adding Code Review feedback form Daniel Silva. Updating Integration Test. --- .../business/ESContentletAPIImpl.java | 68 ++++--- .../contenttype/business/ContentTypeAPI.java | 51 +++-- .../business/ContentTypeAPIImpl.java | 62 ++++-- .../contentlet/business/HostFactoryImpl.java | 186 +++++++++++------- .../templates/business/TemplateAPIImpl.java | 100 +++++++--- .../WEB-INF/messages/Language.properties | 2 +- .../WEB-INF/messages/Language_es.properties | 2 +- .../test/ContentTypeAPIImplTest.java | 33 +++- 8 files changed, 352 insertions(+), 152 deletions(-) diff --git a/dotCMS/src/main/java/com/dotcms/content/elasticsearch/business/ESContentletAPIImpl.java b/dotCMS/src/main/java/com/dotcms/content/elasticsearch/business/ESContentletAPIImpl.java index 3573ee564dd2..f90f2719f905 100644 --- a/dotCMS/src/main/java/com/dotcms/content/elasticsearch/business/ESContentletAPIImpl.java +++ b/dotCMS/src/main/java/com/dotcms/content/elasticsearch/business/ESContentletAPIImpl.java @@ -31,6 +31,7 @@ import com.dotcms.contenttype.transform.contenttype.ContentTypeTransformer; import com.dotcms.contenttype.transform.contenttype.StructureTransformer; import com.dotcms.contenttype.transform.field.LegacyFieldTransformer; +import com.dotcms.exception.ExceptionUtil; import com.dotcms.notifications.bean.NotificationLevel; import com.dotcms.publisher.business.DotPublisherException; import com.dotcms.publisher.business.PublisherAPI; @@ -2416,25 +2417,24 @@ public boolean delete(final Contentlet contentlet, final User user, final Optional deleteOpt = this.checkAndRunDeleteAsWorkflow(contentlet, user, respectFrontendRoles); if (deleteOpt.isPresent()) { - - Logger.info(this, "A Workflow has been ran instead of delete the contentlet: " + - contentlet.getIdentifier()); - + Logger.info(this, String.format("A delete workflow has been used to delete Contentlet ID " + + "'%s' instead of calling the APIs", contentlet.getIdentifier())); return deleteOpt.get(); } - boolean deleted = false; + boolean deleted; final List contentlets = new ArrayList<>(); contentlets.add(contentlet); try { - deleted = delete(contentlets, user, respectFrontendRoles); + boolean isSite = contentlet.isHost(); + deleted = this.deleteContentlets(contentlets, user, respectFrontendRoles, isSite); HibernateUtil.addCommitListener (() -> this.localSystemEventsAPI.notify( - new ContentletDeletedEvent(contentlet, user))); - } catch (DotDataException | DotSecurityException e) { - - logContentletActivity(contentlets, "Error Deleting Content", user); + new ContentletDeletedEvent<>(contentlet, user))); + } catch (final DotDataException | DotSecurityException e) { + this.logContentletActivity(contentlets, String.format("Error deleting content with ID " + + "'%s': %s", contentlet.getIdentifier(), ExceptionUtil.getErrorMessage(e)), user); throw e; } @@ -2953,10 +2953,12 @@ private void backupDestroyedContentlets(final List contentlets, fina * any of the specified contentlets is not archived, an exception will be thrown. If there's * only one language for a given contentlet, the object will be destroyed. * - * @param contentlets - The list of contentlets that will be deleted. - * @param user - The {@link User} performing this action. - * @param respectFrontendRoles - - * @param isDeletingAHost - If the code calling this method is trying to delete a given + * @param contentlets The list of contentlets that will be deleted. + * @param user The {@link User} performing this action. + * @param respectFrontendRoles If the User executing this action has the front-end role, or if + * front-end roles must be validated against this user, set to + * {@code true}. + * @param isDeletingAHost If the code calling this method is trying to delete a given * Site (host), set to {@code true}. Otherwise, set to * {@code false}. * @return If the contentlets were successfully deleted, returns {@code true}. Otherwise, @@ -2974,21 +2976,20 @@ private boolean deleteContentlets(final List contentlets, final User boolean noErrors = true; - if (contentlets == null || contentlets.size() == 0) { - - Logger.info(this, "No contents passed to delete so returning"); + if (UtilMethods.isNotSet(contentlets)) { + Logger.info(this, "No contents passed to delete"); noErrors = false; return noErrors; } - this.logContentletActivity(contentlets, "Deleting Content", user); + this.logContentletActivity(contentlets, "Deleting Contents", user); final List filteredContentlets = this.validateAndFilterContentletsToDelete( contentlets, user, respectFrontendRoles); if (filteredContentlets.size() != contentlets.size()) { - this.logContentletActivity(contentlets, "Error Deleting Content", user); + this.logContentletActivity(contentlets, "Error Deleting Contents.", user); throw new DotSecurityException("User: " + (user != null ? user.getUserId() : "Unknown") + " does not have permission to delete some or all of the contentlets"); } @@ -3000,21 +3001,36 @@ private boolean deleteContentlets(final List contentlets, final User contentletIdentifiers.add(contentlet.getIdentifier()); } - AdminLogger.log(this.getClass(), "delete", - "User trying to delete the following contents: " + - contentletIdentifiers.toString(), user); + AdminLogger.log(this.getClass(), "delete", "User trying to delete the following contents: " + + contentletIdentifiers, user); - final HashSet deletedIdentifiers = new HashSet(); - final Iterator contentletIterator = filteredContentlets.iterator(); - while (contentletIterator.hasNext()) { + final HashSet deletedIdentifiers = new HashSet<>(); + for (final Contentlet filteredContentlet : filteredContentlets) { this.deleteContentlet(contentlets, user, isDeletingAHost, - deletedIdentifiers, contentletIterator.next()); + deletedIdentifiers, filteredContentlet); } return noErrors; } + /** + * Deletes the specified list of Contentlets. + * + * @param contentlets The complete list of {@link Contentlet} objects to delete. + * @param user The {@link User} that is trying to delete such Contentlets. + * @param isDeletingAHost If the Contentlet being deleted is a Site, set this to {@code true} + * so that the "destroy" method is called directly, with no + * validations. + * @param deletedIdentifiers A list that keeps track of the Identifiers that have already been + * deleted. + * @param contentletToDelete The actual {@link Contentlet} that is being deleted. + * + * @throws DotDataException The specified {@link User} does not have the required + * permissions to perform this action. + * @throws DotSecurityException An error occurred when deleting the information from the + * database. + */ private void deleteContentlet(final List contentlets, final User user, final boolean isDeletingAHost, final HashSet deletedIdentifiers, final Contentlet contentletToDelete) diff --git a/dotCMS/src/main/java/com/dotcms/contenttype/business/ContentTypeAPI.java b/dotCMS/src/main/java/com/dotcms/contenttype/business/ContentTypeAPI.java index 108b84934a2f..2a108dc709b6 100644 --- a/dotCMS/src/main/java/com/dotcms/contenttype/business/ContentTypeAPI.java +++ b/dotCMS/src/main/java/com/dotcms/contenttype/business/ContentTypeAPI.java @@ -1,5 +1,6 @@ package com.dotcms.contenttype.business; +import com.dotcms.contenttype.exception.NotFoundInDbException; import com.dotcms.contenttype.model.field.Field; import com.dotcms.contenttype.model.field.FieldVariable; import com.dotcms.contenttype.model.type.BaseContentType; @@ -53,23 +54,47 @@ public interface ContentTypeAPI { "container", "template", "user", "calendarEvent"); /** - * Deletes the given Content Type - * - * @param st Content Type that will be deleted - * @throws DotSecurityException The user does not have permissions to perform this action. - * @throws DotDataException Error occurred when performing the action. + * Deletes the specified Content Type. By default, the process that actually deletes it is + * asynchronous, meaning that a separate thread/transaction takes care of it. You can change this + * behavior by updating the value of the {@link ContentTypeAPIImpl#DELETE_CONTENT_TYPE_ASYNC} + * property to {@code true}. + * + * @param contentType The {@link ContentType} being deleted. + * + * @throws DotSecurityException The User accessing this API does not have the required + * permissions to perform this action. + * @throws DotDataException An error occurred when interacting with the database. */ - void delete(ContentType st) throws DotSecurityException, DotDataException; + void delete(final ContentType contentType) throws DotSecurityException, DotDataException; /** - * Find a Content Type given the inode - * - * @param inodeOrVar Either the Inode or the Velocity var name representing the Structure to find - * @return Content Type Object - * @throws DotSecurityException The user does not have permissions to perform this action. - * @throws DotDataException Error occurred when performing the action. + * Deletes the specified Content Type. By default, the process that actually deletes it is + * asynchronous, meaning that a separate thread/transaction takes care of it. With this + * implementation, you can force dotCMS to wait for the deletion process to be over before + * moving on. Therefore, given the implications related to performance, this method must be + * used carefully + * + * @param contentType The {@link ContentType} being deleted. + * + * @throws DotSecurityException The User accessing this API does not have the required + * permissions to perform this action. + * @throws DotDataException An error occurred when interacting with the database. + */ + void deleteSync(final ContentType contentType) throws DotSecurityException, DotDataException; + + /** + * Returns the Content Type that matches the specified Inode or Velocity Variable Name. + * + * @param inodeOrVar Either the Inode or the Velocity var name representing the Content Type to + * find. + * + * @return The {@link ContentType} that was requested. + * + * @throws DotSecurityException The user does not have permissions to perform this action. + * @throws DotDataException Error occurred when performing the action. + * @throws NotFoundInDbException The Content Type was not found in the database. */ - ContentType find(String inodeOrVar) throws DotSecurityException, DotDataException; + ContentType find(final String inodeOrVar) throws DotSecurityException, DotDataException; /** * Returns a list of Content Types based on the specified list of Velocity Variable Names. If one or more Velocity diff --git a/dotCMS/src/main/java/com/dotcms/contenttype/business/ContentTypeAPIImpl.java b/dotCMS/src/main/java/com/dotcms/contenttype/business/ContentTypeAPIImpl.java index 613ee97c3410..8353f24beccd 100644 --- a/dotCMS/src/main/java/com/dotcms/contenttype/business/ContentTypeAPIImpl.java +++ b/dotCMS/src/main/java/com/dotcms/contenttype/business/ContentTypeAPIImpl.java @@ -118,29 +118,45 @@ public ContentTypeAPIImpl(User user, boolean respectFrontendRoles) { APILocator.getPermissionAPI(), APILocator.getContentTypeFieldAPI(), APILocator.getLocalSystemEventsAPI()); } + @Override + public void delete(final ContentType contentType) throws DotSecurityException, DotDataException { + this.delete(contentType, true); + } + + @Override + public void deleteSync(final ContentType contentType) throws DotSecurityException, DotDataException { + this.delete(contentType, false); + } + /** - * Content-Type Delete Entry point - * @param type Content Type that will be deleted - * @throws DotSecurityException - * @throws DotDataException + * Deletes the specified Content Type, either in the same database transaction or as a separate + * process. + * + * @param contentType The {@link ContentType} being deleted. + * @param async If the deletion process should be executed asynchronously -- i.e.; in a + * separate process, set this to {@code true}. + * + * @throws DotSecurityException The specified User does not have edition permissions on this + * Content Type. + * @throws DotDataException An error occurred when interacting with the database. */ - @Override - public void delete(ContentType type) throws DotSecurityException, DotDataException { - if (!contentTypeCanBeDeleted(type)) { - Logger.warn(this, "Content Type " + type.name() - + " cannot be deleted because it is referenced by other content types"); + private void delete(final ContentType contentType, final boolean async) throws DotSecurityException, DotDataException { + if (!contentTypeCanBeDeleted(contentType)) { + Logger.warn(this, String.format("Content Type '%s' does not exist", contentType.name())); return; } - - final boolean asyncDelete = Config.getBooleanProperty(DELETE_CONTENT_TYPE_ASYNC, true);; - final boolean asyncDeleteWithJob = Config.getBooleanProperty(DELETE_CONTENT_TYPE_ASYNC_WITH_JOB, true); + boolean asyncDelete = async; + boolean asyncDeleteWithJob = Config.getBooleanProperty(DELETE_CONTENT_TYPE_ASYNC_WITH_JOB, true); + if (async) { + asyncDelete = Config.getBooleanProperty(DELETE_CONTENT_TYPE_ASYNC, true); + } if (!asyncDelete) { - Logger.debug(this, String.format(" Content type (%s) will be deleted sequentially.", type.name())); - transactionalDelete(type); + Logger.debug(this, () -> String.format("Content Type '%s' will be deleted synchronously", contentType.name())); + this.transactionalDelete(contentType); } else { //We make a copy to hold all the contentlets that will be deleted asynchronously and then dispose the original one - triggerAsyncDelete(type, asyncDeleteWithJob); + this.triggerAsyncDelete(contentType, asyncDeleteWithJob); } } @@ -209,8 +225,20 @@ private void transactionalDelete(ContentType type) throws DotDataException { HibernateUtil.addCommitListener(() -> localSystemEventsAPI.notify(new ContentTypeDeletedEvent(type.variable()))); } - boolean contentTypeCanBeDeleted(ContentType type) throws DotDataException, DotSecurityException { - + /** + * Verifies whether the {@link User} calling this method has the required {@code EDIT} + * permissions to delete the specified Content Type or not. + * + * @param type The {@link ContentType} to be deleted. + * + * @return If the {@link User} has the required permissions to delete the specified Content + * Type, returns {@code true}. + * + * @throws DotDataException An error occurred when accessing the database. + * @throws DotSecurityException The specified User does not have the necessary permissions to + * perform this action. + */ + protected boolean contentTypeCanBeDeleted(ContentType type) throws DotDataException, DotSecurityException { if (null == type.id()) { throw new DotDataException("ContentType must have an id set"); } diff --git a/dotCMS/src/main/java/com/dotmarketing/portlets/contentlet/business/HostFactoryImpl.java b/dotCMS/src/main/java/com/dotmarketing/portlets/contentlet/business/HostFactoryImpl.java index 79b4e46cccd2..752bd6ca413b 100644 --- a/dotCMS/src/main/java/com/dotmarketing/portlets/contentlet/business/HostFactoryImpl.java +++ b/dotCMS/src/main/java/com/dotmarketing/portlets/contentlet/business/HostFactoryImpl.java @@ -8,6 +8,7 @@ import com.dotcms.contenttype.model.type.ContentType; import com.dotcms.contenttype.model.type.ContentTypeBuilder; import com.dotcms.contenttype.transform.contenttype.StructureTransformer; +import com.dotcms.exception.ExceptionUtil; import com.dotcms.notifications.bean.NotificationLevel; import com.dotcms.notifications.bean.NotificationType; import com.dotcms.util.ConversionUtils; @@ -67,7 +68,7 @@ */ public class HostFactoryImpl implements HostFactory { - private HostCache siteCache = CacheLocator.getHostCache(); + private final HostCache siteCache; private ContentletFactory contentFactory; private ContentletAPI contentletAPI; @@ -396,32 +397,44 @@ public Host DBSearch(final String id, final boolean respectFrontendRoles) throws return site; } + /** + * This method is called inside a separate thread and takes care of deleting the specified Site + * in a process in the background. Once it's done, a notification will be generated as well, + * which is particularly useful to the UI layer. + * + * @param site The {@link Host} object to be deleted. + * @param user The {@link User} object that is requesting the deletion. + * @param respectFrontendRoles If the User executing this action has the front-end role, or if + * front-end roles must be validated against this user, set to + * {@code true}. + * + * @return If the Site was deleted successfully, returns {@code true}. + * + * @throws DotRuntimeException The specified Site failed to be deleted. + */ @WrapInTransaction - private Boolean innerDeleteHost(final Host site, final User user, final boolean respectFrontendRoles) { + private Boolean innerDeleteSite(final Host site, final User user, final boolean respectFrontendRoles) { try { - deleteHost(site, user, respectFrontendRoles); + deleteSite(site, user, respectFrontendRoles); HibernateUtil.addCommitListener (() -> generateNotification(site, user)); - } catch (Exception e) { - // send notification + } catch (final Exception e) { try { - APILocator.getNotificationAPI().generateNotification( new I18NMessage("notification.hostapi.delete.error.title"), // title = Host Notification - new I18NMessage("notifications_host_deletion_error", site.getHostname(), e.getMessage()), + new I18NMessage("notifications_host_deletion_error", site.getHostname(), ExceptionUtil.getErrorMessage(e)), null, // no actions NotificationLevel.ERROR, NotificationType.GENERIC, user.getUserId(), user.getLocale() ); - } catch (final DotDataException e1) { Logger.error(HostAPIImpl.class, String.format("An error occurred when saving Site Deletion " + - "Notification for site '%s': %s", site, e.getMessage()), e); + "Notification for site '%s': %s", site, ExceptionUtil.getErrorMessage(e)), e); } final String errorMsg = String.format("An error occurred when User '%s' tried to delete Site " + - "'%s': %s", user.getUserId(), site, e.getMessage()); + "'%s': %s", user.getUserId(), site, ExceptionUtil.getErrorMessage(e)); Logger.error(HostAPIImpl.class, errorMsg, e); throw new DotRuntimeException(errorMsg, e); } @@ -447,84 +460,112 @@ private void generateNotification(final Host site, final User user) { } } - public void deleteHost(final Host site, final User user, final boolean respectFrontendRoles) throws Exception { - if(site != null){ + /** + * Deletes the specified Site from the content repository. There are a lot of different objects + * that must be deleted when a Site is removed, such as Menu Links, Contentlets, Folders, and so + * on. This means that the deletion process is quite complex and may take a very long time to + * finish. + * + * @param site The {@link Host} object to be deleted. + * @param user The {@link User} object that is requesting the deletion. + * @param respectFrontendRoles If the User executing this action has the front-end role, or if + * front-end roles must be validated against this user, set to + * {@code true}. + * + * @throws Exception An error occurred when deleting the specified Site. + */ + public void deleteSite(final Host site, final User user, final boolean respectFrontendRoles) throws Exception { + if (null == site || UtilMethods.isNotSet(site.getIdentifier())) { + return; + } else { siteCache.remove(site); } - + Logger.info(this, "======================================================================"); + Logger.info(this, String.format(" Start deleting Site '%s' ...", site.getHostname())); + Logger.info(this, "======================================================================"); final DotConnect dc = new DotConnect(); - // Remove Links - MenuLinkAPI linkAPI = APILocator.getMenuLinkAPI(); - List links = linkAPI.findLinks(user, true, null, site.getIdentifier(), null, null, null, 0, -1, null); - for (Link link : links) { + final MenuLinkAPI linkAPI = APILocator.getMenuLinkAPI(); + final List links = linkAPI.findLinks(user, true, null, site.getIdentifier(), null, null, null, 0, -1, null); + Logger.info(this, String.format("-> (Step 1/14) Deleting %d Menu Links from Site '%s'", links.size(), site.getHostname())); + for (final Link link : links) { linkAPI.delete(link, user, respectFrontendRoles); } - // Remove Contentlet - ContentletAPI contentAPI = APILocator.getContentletAPI(); + Logger.info(this, String.format("-> (Step 2/14) Deleting all Contentlets from Site '%s'", site.getHostname())); + final ContentletAPI contentAPI = APILocator.getContentletAPI(); contentAPI.deleteByHost(site, APILocator.systemUser(), respectFrontendRoles); - // Remove Folders - FolderAPI folderAPI = APILocator.getFolderAPI(); - List folders = folderAPI.findFoldersByHost(site, user, respectFrontendRoles); - for (Folder folder : folders) { + final FolderAPI folderAPI = APILocator.getFolderAPI(); + final List folders = folderAPI.findFoldersByHost(site, user, respectFrontendRoles); + Logger.info(this, String.format("-> (Step 3/14) Deleting %d Folders from Site '%s'", folders.size(), site.getHostname())); + for (final Folder folder : folders) { folderAPI.delete(folder, user, respectFrontendRoles); } - // Remove Templates - TemplateAPI templateAPI = APILocator.getTemplateAPI(); - List