From 824106f91f5972e25a6cc2e14248d123c95c1823 Mon Sep 17 00:00:00 2001 From: Valentino Giardino <77643678+valentinogiardino@users.noreply.github.com> Date: Fri, 8 Nov 2024 09:19:35 -0300 Subject: [PATCH] fix(variantWebApi) dotAsset localization issue for non default language #29851 (#30587) ### Proposed Changes * Updated the `isFileFallback` method to account for content in non-default languages. - Added a conditional check for `DOTASSET` types and ensured fallback behavior respects language settings via `LanguageAPI`. * Updated the `inodePath` method in `ShortyServlet` to implement language fallback logic for contentlets with `ImageField` or `FileField` when the associated content version is missing in a specified language. ### Checklist - [x] Tests - [x] Translations - [x] Security Implications Contemplated ### Additional Info **Context**: This fix resolves an issue where content with image fields in a secondary language did not display the associated image as expected. The modification ensures that dotAsset images can fall back to the default language if they are not available in the specified language, preventing unexpected behaviors when creating or editing multilingual pages. ### Screenshots #### Current behavior (issue) https://github.com/user-attachments/assets/81c0ae60-a5d5-4046-a77b-87dcd1195cf3 #### Behavior after the fix https://github.com/user-attachments/assets/f9f44e12-9e1c-484c-b496-32efbd3c9c01 --- .../business/web/VariantWebAPIImpl.java | 2 +- .../dotmarketing/servlets/ShortyServlet.java | 87 +++++++++++++---- .../com/dotcms/datagen/TestDataUtils.java | 6 ++ .../web/VariantWebAPIImplIntegrationTest.java | 29 +++++- .../ShortyServletAndTitleImageTest.java | 94 +++++++++++++++---- 5 files changed, 176 insertions(+), 42 deletions(-) diff --git a/dotCMS/src/main/java/com/dotcms/variant/business/web/VariantWebAPIImpl.java b/dotCMS/src/main/java/com/dotcms/variant/business/web/VariantWebAPIImpl.java index bf386e9e9831..5a83c9088ce9 100644 --- a/dotCMS/src/main/java/com/dotcms/variant/business/web/VariantWebAPIImpl.java +++ b/dotCMS/src/main/java/com/dotcms/variant/business/web/VariantWebAPIImpl.java @@ -253,7 +253,7 @@ private static boolean isWidgetFallback(ContentType type) { } private static boolean isFileFallback(ContentType type) { - return type.baseType() == BaseContentType.FILEASSET + return (type.baseType() == BaseContentType.FILEASSET || type.baseType() == BaseContentType.DOTASSET) && APILocator.getLanguageAPI().canDefaultFileToDefaultLanguage(); } diff --git a/dotCMS/src/main/java/com/dotmarketing/servlets/ShortyServlet.java b/dotCMS/src/main/java/com/dotmarketing/servlets/ShortyServlet.java index f17548d4534a..568a31ded0c3 100644 --- a/dotCMS/src/main/java/com/dotmarketing/servlets/ShortyServlet.java +++ b/dotCMS/src/main/java/com/dotmarketing/servlets/ShortyServlet.java @@ -499,7 +499,20 @@ private boolean isValidRequest(final HttpServletRequest request, return true; } - + + /** + * Resolves and builds the appropriate file path for a contentlet's field. + * This method handles both regular fields and special cases for image/file fields, + * including language fallback logic when necessary. + * + * @param contentlet The contentlet whose field path needs to be resolved + * @param tryField The name of the field to try to resolve + * @param live Whether to use the live version (true) or working version (false) + * @return A string representing the path to the field's content + * @throws DotStateException If there's an issue with the contentlet's state + * @throws DotDataException If there's an error accessing the data + * @throws DotSecurityException If there's a security violation + */ protected final String inodePath(final Contentlet contentlet, final String tryField, final boolean live) @@ -508,35 +521,73 @@ protected final String inodePath(final Contentlet contentlet, final Optional fieldOpt = resolveField(contentlet, tryField); if (fieldOpt.isEmpty()) { - return "/" + contentlet.getInode() + "/" + FILE_ASSET_DEFAULT; + return buildFieldPath(contentlet, FILE_ASSET_DEFAULT); } final Field field = fieldOpt.get(); if (field instanceof ImageField || field instanceof FileField) { final String relatedImageId = contentlet.getStringProperty(field.variable()); - final Optional contentletVersionInfo = + Optional contentletVersionInfo = this.versionableAPI.getContentletVersionInfo(relatedImageId, contentlet.getLanguageId()); - if (contentletVersionInfo.isPresent()) { - final String inode = live ? contentletVersionInfo.get().getLiveInode() - : contentletVersionInfo.get().getWorkingInode(); - - final Contentlet imageContentlet = APILocator.getContentletAPI() - .find(inode, APILocator.systemUser(), false); - - validateContentlet(imageContentlet, live, inode); - - final String fieldVar = imageContentlet.isDotAsset() ? - DotAssetContentType.ASSET_FIELD_VAR : FILE_ASSET_DEFAULT; + if (contentletVersionInfo.isEmpty() && shouldFallbackToDefaultLanguage(contentlet)) { + // Try finding the contentlet version with the default language ID + Logger.info(this, "No contentlet version found for identifier " + relatedImageId + " in language " + contentlet.getLanguageId() + ", trying default language."); + contentletVersionInfo = this.versionableAPI.getContentletVersionInfo(relatedImageId,APILocator.getLanguageAPI().getDefaultLanguage().getId()); + } - return new StringBuilder(StringPool.FORWARD_SLASH).append(inode) - .append(StringPool.FORWARD_SLASH).append(fieldVar).toString(); + if (contentletVersionInfo.isPresent()) { + Logger.debug(this, "Contentlet version found for identifier: " + relatedImageId); + final Contentlet imageContentlet = getImageContentlet(contentletVersionInfo.get(), live); + validateContentlet(imageContentlet, live, imageContentlet.getInode()); + final String fieldVar = imageContentlet.isDotAsset() ? DotAssetContentType.ASSET_FIELD_VAR : FILE_ASSET_DEFAULT; + return buildFieldPath(imageContentlet, fieldVar); } + Logger.debug(this, "No contentlet version found for identifier: " + relatedImageId + ", returning path based on original contentlet inode: " + contentlet.getInode()); } + return buildFieldPath(contentlet, field.variable()); + } + + /** + * Determines whether the system should attempt to fallback to the default language + * for the given contentlet. This is used when content is not found in the + * contentlet's original language. + * + * @param contentlet The contentlet to check for language fallback eligibility + * @return true if the system should attempt to use the default language, false otherwise + */ + private boolean shouldFallbackToDefaultLanguage(final Contentlet contentlet) { + return APILocator.getLanguageAPI().canDefaultFileToDefaultLanguage() && + APILocator.getLanguageAPI().getDefaultLanguage().getId() != contentlet.getLanguageId(); + } + + /** + * Retrieves the appropriate contentlet version (live or working) for an image + * based on the provided version info. + * + * @param versionInfo The version information for the contentlet + * @param live Whether to retrieve the live version (true) or working version (false) + * @return The requested version of the contentlet + * @throws DotDataException If there's an error accessing the data + * @throws DotSecurityException If there's a security violation + */ + private Contentlet getImageContentlet(final ContentletVersionInfo versionInfo, final boolean live) + throws DotDataException, DotSecurityException { + final String inode = live ? versionInfo.getLiveInode() : versionInfo.getWorkingInode(); + return APILocator.getContentletAPI().find(inode, APILocator.systemUser(), false); + } - return new StringBuilder(StringPool.FORWARD_SLASH).append(contentlet.getInode()) - .append(StringPool.FORWARD_SLASH).append(field.variable()).toString(); + /** + * Constructs a standardized field path for a contentlet and field variable. + * The path format is: /[contentlet-inode]/[field-variable] + * + * @param contentlet The contentlet for which to build the path + * @param fieldVar The field variable to append to the path + * @return A formatted path string + */ + private String buildFieldPath(final Contentlet contentlet, final String fieldVar) { + return StringPool.FORWARD_SLASH + contentlet.getInode() + StringPool.FORWARD_SLASH + fieldVar; } private void validateContentlet(final Contentlet contentlet, final boolean live, final String inode) throws DotDataException { diff --git a/dotcms-integration/src/test/java/com/dotcms/datagen/TestDataUtils.java b/dotcms-integration/src/test/java/com/dotcms/datagen/TestDataUtils.java index b508989796d3..6cc236ab2ff7 100644 --- a/dotcms-integration/src/test/java/com/dotcms/datagen/TestDataUtils.java +++ b/dotcms-integration/src/test/java/com/dotcms/datagen/TestDataUtils.java @@ -660,6 +660,12 @@ public static Contentlet getDotAssetLikeContentlet(final Treeable hostOrFolder) } + @WrapInTransaction + public static Contentlet getDotAssetLikeContentlet(final boolean perist, final long languageId) { + Contentlet dotAssetContentlet = getDotAssetLikeContentlet(languageId, new FolderDataGen().nextPersisted()); + return perist ? ContentletDataGen.publish(dotAssetContentlet) : dotAssetContentlet; + } + @WrapInTransaction public static Contentlet getDotAssetLikeContentlet(long languageId, final Treeable hostOrFolder) { diff --git a/dotcms-integration/src/test/java/com/dotcms/variant/business/web/VariantWebAPIImplIntegrationTest.java b/dotcms-integration/src/test/java/com/dotcms/variant/business/web/VariantWebAPIImplIntegrationTest.java index 2cbaa2bc8803..e9d8b5d6ebbd 100644 --- a/dotcms-integration/src/test/java/com/dotcms/variant/business/web/VariantWebAPIImplIntegrationTest.java +++ b/dotcms-integration/src/test/java/com/dotcms/variant/business/web/VariantWebAPIImplIntegrationTest.java @@ -2,16 +2,17 @@ import static org.junit.Assert.assertEquals; +import com.dotcms.JUnit4WeldRunner; import com.dotcms.api.web.HttpServletRequestThreadLocal; -import com.dotcms.datagen.LanguageDataGen; -import com.dotcms.datagen.TestDataUtils; -import com.dotcms.datagen.VariantDataGen; +import com.dotcms.datagen.*; import com.dotcms.mock.request.MockSession; import com.dotcms.util.IntegrationTestInitService; import com.dotcms.variant.VariantAPI; import com.dotcms.variant.model.Variant; import com.dotmarketing.business.APILocator; import com.dotmarketing.business.web.WebAPILocator; + +import javax.enterprise.context.ApplicationScoped; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpSession; @@ -21,10 +22,10 @@ import com.dotmarketing.portlets.languagesmanager.model.Language; import com.dotmarketing.util.PageMode; import com.liferay.portal.model.User; -import net.bytebuddy.utility.RandomString; import org.apache.commons.lang.RandomStringUtils; import org.junit.BeforeClass; import org.junit.Test; +import org.junit.runner.RunWith; import static org.junit.Assert.assertNotNull; import static org.mockito.Mockito.mock; @@ -33,6 +34,8 @@ import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; +@ApplicationScoped +@RunWith(JUnit4WeldRunner.class) public class VariantWebAPIImplIntegrationTest { @BeforeClass @@ -273,4 +276,22 @@ public void test_getContentletVersionInfoByFallback_should_get_default_content() assertNotNull(cvi); } + + + /** + * Method to test {@link VariantWebAPIImpl#getContentletVersionInfoByFallback(long, String, PageMode, User, boolean)} + * When: The dotAsset contentlet does not have a version for the language + * Should: Return the default contentlet + * @throws DotDataException + */ + @Test + public void test_getContentletVersionInfoByFallback_should_get_default_content_for_dotAsset() throws DotDataException { + final Contentlet content = TestDataUtils.getDotAssetLikeContentlet(true, APILocator.getLanguageAPI().getDefaultLanguage().getId()); + final Language language = new LanguageDataGen().nextPersisted(); + + final VariantWebAPI variantWebAPI = WebAPILocator.getVariantWebAPI(); + ContentletVersionInfo cvi = variantWebAPI.getContentletVersionInfoByFallback(language.getId(), content.getIdentifier(), PageMode.LIVE, APILocator.getUserAPI().getAnonymousUser()); + + assertNotNull(cvi); + } } \ No newline at end of file diff --git a/dotcms-integration/src/test/java/com/dotmarketing/servlets/ShortyServletAndTitleImageTest.java b/dotcms-integration/src/test/java/com/dotmarketing/servlets/ShortyServletAndTitleImageTest.java index 692974a39573..e3f8a8cdfb0d 100644 --- a/dotcms-integration/src/test/java/com/dotmarketing/servlets/ShortyServletAndTitleImageTest.java +++ b/dotcms-integration/src/test/java/com/dotmarketing/servlets/ShortyServletAndTitleImageTest.java @@ -9,12 +9,12 @@ import java.util.Optional; -import com.dotcms.concurrent.DotSubmitter; -import com.dotmarketing.exception.DotDataException; -import com.dotmarketing.exception.DotSecurityException; -import com.dotmarketing.image.focalpoint.FocalPoint; +import com.dotcms.contenttype.model.type.DotAssetContentType; +import com.dotcms.datagen.*; +import com.dotcms.junit.CustomDataProviderRunner; + +import com.dotmarketing.portlets.languagesmanager.model.Language; import com.tngtech.java.junit.dataprovider.DataProvider; -import com.tngtech.java.junit.dataprovider.DataProviderRunner; import com.tngtech.java.junit.dataprovider.UseDataProvider; import org.junit.BeforeClass; import org.junit.Test; @@ -23,9 +23,6 @@ import com.dotcms.contenttype.model.field.ImmutableBinaryField; import com.dotcms.contenttype.model.field.ImmutableImageField; import com.dotcms.contenttype.model.type.ContentType; -import com.dotcms.datagen.ContentTypeDataGen; -import com.dotcms.datagen.FileAssetDataGen; -import com.dotcms.datagen.FolderDataGen; import com.dotcms.util.IntegrationTestInitService; import com.dotmarketing.beans.Host; import com.dotmarketing.business.APILocator; @@ -38,10 +35,12 @@ import com.liferay.portal.model.User; import org.junit.runner.RunWith; +import javax.enterprise.context.ApplicationScoped; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; -@RunWith(DataProviderRunner.class) +@ApplicationScoped +@RunWith(CustomDataProviderRunner.class) public class ShortyServletAndTitleImageTest { @@ -266,8 +265,6 @@ public void test_ShortyServlet_Returns_Proper_Field() throws Exception{ path.contains("/" + cvi.get().getLiveInode())); assertTrue(path.contains("/fileAsset")); - - // null out image field @@ -284,14 +281,73 @@ public void test_ShortyServlet_Returns_Proper_Field() throws Exception{ assertTrue("shorty points to the inode", path.contains("/" + contentlet.getInode())); assertTrue("titleImage points to binary3", path.contains("/" + binary3.variable())); - - - - - - - - + } + + + + + + /** + * Method to test: {@link ShortyServlet#inodePath(Contentlet, String, boolean)} + * Given Scenario: A contentlet with an image field referencing a dot asset is created in a secondary language, + * while the actual asset only exists in the default language + * ExpectedResult: The servlet should: + * - Detect that the asset doesn't exist in the secondary language + * - Fall back to the default language version + * - Return the correct path for both live and working versions + * - Include the correct field variable (DotAssetContentType.ASSET_FIELD_VAR) + */ + @Test + public void test_inodePath_ImageField_ShouldFallbackToDefaultLanguage() throws Exception { + // Initialize servlet + final ShortyServlet servlet = new ShortyServlet(); + + // Create and publish dot asset in default language + Contentlet dotAssset = TestDataUtils.getDotAssetLikeContentlet(true, + APILocator.getLanguageAPI().getDefaultLanguage().getId()); + APILocator.getContentletAPI().publish(dotAssset, user, false); + + // Create content type with image field + final ContentType contentType = new ContentTypeDataGen().nextPersisted(); + Field imageField = ImmutableImageField.builder() + .contentTypeId(contentType.id()) + .name("image") + .variable("image").build(); + imageField = APILocator.getContentTypeFieldAPI().save(imageField, user); + + // Setup languages + final Language defaultLanguage = APILocator.getLanguageAPI().getDefaultLanguage(); + final Language language2 = new LanguageDataGen().nextPersisted(); + + // Create contentlet in secondary language + Contentlet contentlet = new Contentlet(); + contentlet.setContentTypeId(contentType.id()); + contentlet.setStringProperty(imageField, dotAssset.getIdentifier()); + contentlet.setLanguageId(language2.getId()); + contentlet = APILocator.getContentletAPI().checkin(contentlet, user, false); + + // Verify asset doesn't exist in secondary language + Optional cvi = APILocator.getVersionableAPI() + .getContentletVersionInfo(contentlet.getStringProperty(imageField.variable()), + language2.getId()); + assertTrue(cvi.isEmpty()); + + // Verify asset exists in default language + Optional cvi2 = APILocator.getVersionableAPI() + .getContentletVersionInfo(contentlet.getStringProperty(imageField.variable()), + defaultLanguage.getId()); + assertTrue(cvi2.isPresent()); + assertEquals(cvi2.get().getLang(), defaultLanguage.getId()); + + // Verify live version path resolution + String path = servlet.inodePath(contentlet, imageField.variable(), true); + assertTrue("shorty points to the asset live inode", path.contains("/" + cvi2.get().getLiveInode())); + assertTrue(path.contains("/" + DotAssetContentType.ASSET_FIELD_VAR)); + + // Verify working version path resolution + path = servlet.inodePath(contentlet, imageField.variable(), false); + assertTrue("shorty points to the asset working inode", path.contains("/" + cvi2.get().getWorkingInode())); + assertTrue(path.contains("/" + DotAssetContentType.ASSET_FIELD_VAR)); } /**