Skip to content

Commit 4919535

Browse files
authored
Fix(block-editor) : Fixed removedAtttribute method on velocity request wrapper (dotCMS#32542)
Closes dotCMS#32522 ### Proposed Changes * This pull request updates the `removeAttribute` method from the `VelocityRequestWrapper` class to prevent the removal of attributes listed in the `SET_VALUE_BLACKLIST`. If an attribute is blacklisted, the method now exits without invoking the superclass method, otherwise the attribute is removed. * Added a new test suite in `VelocityRequestWrapperTest` to verify the behavior of `setAttribute` and `removeAttribute` methods: ensures blacklisted attributes cannot be set or removed and confirms that non-blacklisted attributes can be set and removed as expected. * With the attribute removal working as expected, the `refreshBlockEditorDataMap` method from the `StoryBlockAPIImpl` will work as expected in these lines: https://github.com/dotCMS/core/blob/484527afeea886a39d45473227d3340a7427dd20/dotCMS/src/main/java/com/dotcms/contenttype/business/StoryBlockAPIImpl.java#L451-L453 That attribute is removed to signal that there are no recursion levels when refreshing the data map for a contentlet referenced from a block editor field. When it wasn't removed, content referenced from the second block editor field wasn't refreshed as expected. ### Checklist - [x] Tests
1 parent 3b116a2 commit 4919535

File tree

4 files changed

+290
-10
lines changed

4 files changed

+290
-10
lines changed

dotCMS/src/main/java/com/dotcms/mock/request/MockAttributeRequest.java

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,4 +45,9 @@ public void setAttribute(String name, Object o) {
4545
attributes.put(name, o);
4646
}
4747

48+
@Override
49+
public void removeAttribute(String name) {
50+
attributes.remove(name);
51+
}
52+
4853
}

dotCMS/src/main/java/com/dotcms/rendering/velocity/viewtools/VelocityRequestWrapper.java

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -94,8 +94,11 @@ public String getRealPath(final String path) {
9494
}
9595

9696
@Override
97-
public void removeAttribute(String arg0) {
98-
// do nothing
97+
public void removeAttribute(String key) {
98+
if (SET_VALUE_BLACKLIST.contains(key)) {
99+
return;
100+
}
101+
super.removeAttribute(key);
99102
}
100103

101104
@Override
Lines changed: 140 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,140 @@
1+
package com.dotcms.rendering.velocity.viewtools;
2+
3+
import com.dotcms.UnitTestBase;
4+
import com.dotmarketing.util.Config;
5+
import com.liferay.portal.util.WebKeys;
6+
import org.junit.AfterClass;
7+
import org.junit.BeforeClass;
8+
import org.junit.Test;
9+
import org.junit.runner.RunWith;
10+
import org.mockito.Mock;
11+
import org.mockito.junit.MockitoJUnitRunner;
12+
13+
import javax.servlet.http.HttpServletRequest;
14+
15+
import static org.mockito.Mockito.*;
16+
17+
/**
18+
* Unit tests for {@link VelocityRequestWrapper} class.
19+
* Tests the setAttribute and removeAttribute methods with blacklisted and non-blacklisted attributes.
20+
*/
21+
@RunWith(MockitoJUnitRunner.class)
22+
public class VelocityRequestWrapperTest extends UnitTestBase {
23+
24+
@Mock
25+
private HttpServletRequest mockRequest;
26+
27+
private static boolean originalConfigValue;
28+
29+
/**
30+
* Sets up the config property before class loading to ensure SET_VALUE_BLACKLIST is initialized correctly.
31+
*/
32+
@BeforeClass
33+
public static void setUpClass() {
34+
// Store original config value to restore after all tests
35+
originalConfigValue = Config.getBooleanProperty("VELOCITY_PREVENT_SETTING_USER_ID", true);
36+
Config.setProperty("VELOCITY_PREVENT_SETTING_USER_ID", true);
37+
}
38+
39+
/**
40+
* Restores the original config value after all tests complete.
41+
*/
42+
@AfterClass
43+
public static void tearDownClass() {
44+
// Restore original config value
45+
Config.setProperty("VELOCITY_PREVENT_SETTING_USER_ID", originalConfigValue);
46+
}
47+
48+
/**
49+
* Tests that the VelocityRequestWrapper correctly prevents setting blacklisted attributes.
50+
* Specifically, it checks that USER_ID attributes is not set.
51+
*/
52+
@Test
53+
public void testSetAttribute_withBlacklistedUserIdAttribute_shouldNotSetAttribute() {
54+
final VelocityRequestWrapper wrapper = VelocityRequestWrapper.wrapVelocityRequest(mockRequest);
55+
56+
// When: setting a blacklisted attribute (USER_ID)
57+
wrapper.setAttribute(WebKeys.USER_ID, "testUserId");
58+
59+
// Then: setAttribute should not be called on the wrapped request
60+
verify(mockRequest, never()).setAttribute(WebKeys.USER_ID, "testUserId");
61+
}
62+
63+
/**
64+
* Tests that the VelocityRequestWrapper correctly prevents setting blacklisted attributes.
65+
* Specifically, it checks that USER attribute is not set.
66+
*/
67+
@Test
68+
public void testSetAttribute_withBlacklistedUserAttribute_shouldNotSetAttribute() {
69+
final VelocityRequestWrapper wrapper = VelocityRequestWrapper.wrapVelocityRequest(mockRequest);
70+
71+
// When: setting a blacklisted attribute (USER)
72+
wrapper.setAttribute(WebKeys.USER, "testUser");
73+
74+
// Then: setAttribute should not be called on the wrapped request
75+
verify(mockRequest, never()).setAttribute(WebKeys.USER, "testUser");
76+
}
77+
78+
/**
79+
* Tests that the VelocityRequestWrapper allows setting non-blacklisted attributes.
80+
* Specifically, it checks that a custom attribute is set correctly.
81+
*/
82+
@Test
83+
public void testSetAttribute_withNonBlacklistedAttribute_shouldSetAttribute() {
84+
final VelocityRequestWrapper wrapper = VelocityRequestWrapper.wrapVelocityRequest(mockRequest);
85+
final String testKey = "testAttribute";
86+
final String testValue = "testValue";
87+
88+
// When: setting a non-blacklisted attribute
89+
wrapper.setAttribute(testKey, testValue);
90+
91+
// Then: setAttribute should be called on the wrapped request
92+
verify(mockRequest, times(1)).setAttribute(testKey, testValue);
93+
}
94+
95+
/**
96+
* Tests that the VelocityRequestWrapper correctly prevents removing blacklisted attributes.
97+
* Specifically, it checks that USER_ID attribute is not removed.
98+
*/
99+
@Test
100+
public void testRemoveAttribute_withBlacklistedUserIdAttribute_shouldNotRemoveAttribute() {
101+
final VelocityRequestWrapper wrapper = VelocityRequestWrapper.wrapVelocityRequest(mockRequest);
102+
103+
// When: removing a blacklisted attribute (USER_ID)
104+
wrapper.removeAttribute(WebKeys.USER_ID);
105+
106+
// Then: removeAttribute should not be called on the wrapped request
107+
verify(mockRequest, never()).removeAttribute(WebKeys.USER_ID);
108+
}
109+
110+
/**
111+
* Tests that the VelocityRequestWrapper correctly prevents removing blacklisted attributes.
112+
* Specifically, it checks that USER attribute is not removed.
113+
*/
114+
@Test
115+
public void testRemoveAttribute_withBlacklistedUserAttribute_shouldNotRemoveAttribute() {
116+
final VelocityRequestWrapper wrapper = VelocityRequestWrapper.wrapVelocityRequest(mockRequest);
117+
118+
// When: removing a blacklisted attribute (USER)
119+
wrapper.removeAttribute(WebKeys.USER);
120+
121+
// Then: removeAttribute should not be called on the wrapped request
122+
verify(mockRequest, never()).removeAttribute(WebKeys.USER);
123+
}
124+
125+
/**
126+
* Tests that the VelocityRequestWrapper allows removing non-blacklisted attributes.
127+
* Specifically, it checks that a custom attribute is removed correctly.
128+
*/
129+
@Test
130+
public void testRemoveAttribute_withNonBlacklistedAttribute_shouldRemoveAttribute() {
131+
final VelocityRequestWrapper wrapper = VelocityRequestWrapper.wrapVelocityRequest(mockRequest);
132+
final String testKey = "testAttribute";
133+
134+
// When: removing a non-blacklisted attribute
135+
wrapper.removeAttribute(testKey);
136+
137+
// Then: removeAttribute should be called on the wrapped request
138+
verify(mockRequest, times(1)).removeAttribute(testKey);
139+
}
140+
}

dotcms-integration/src/test/java/com/dotcms/contenttype/business/StoryBlockAPITest.java

Lines changed: 140 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -10,14 +10,11 @@
1010
import com.dotcms.contenttype.model.type.ContentType;
1111
import com.dotcms.datagen.*;
1212
import com.dotcms.mock.request.MockAttributeRequest;
13-
import com.dotcms.mock.request.MockHttpRequestIntegrationTest;
14-
import com.dotcms.mock.response.MockHttpResponse;
15-
import com.dotcms.rendering.velocity.viewtools.content.util.ContentUtils;
13+
import com.dotcms.rendering.velocity.viewtools.VelocityRequestWrapper;
1614
import com.dotcms.util.CollectionsUtils;
1715
import com.dotcms.util.IntegrationTestInitService;
1816
import com.dotcms.util.JsonUtil;
1917
import com.dotmarketing.business.APILocator;
20-
import com.dotmarketing.db.LocalTransaction;
2118
import com.dotmarketing.exception.DotDataException;
2219
import com.dotmarketing.exception.DotSecurityException;
2320
import com.dotmarketing.portlets.contentlet.model.Contentlet;
@@ -29,24 +26,22 @@
2926
import com.fasterxml.jackson.core.JsonProcessingException;
3027
import com.liferay.util.StringPool;
3128
import com.tngtech.java.junit.dataprovider.DataProvider;
32-
import com.tngtech.java.junit.dataprovider.DataProviderRunner;
3329
import com.tngtech.java.junit.dataprovider.UseDataProvider;
3430
import io.vavr.control.Try;
3531
import org.junit.Assert;
3632
import org.junit.BeforeClass;
37-
import org.junit.Ignore;
3833
import org.junit.Test;
3934
import org.junit.runner.RunWith;
4035

4136
import javax.enterprise.context.ApplicationScoped;
4237
import javax.servlet.http.HttpServletRequest;
4338
import javax.servlet.http.HttpServletResponse;
39+
import java.io.IOException;
4440
import java.util.LinkedHashMap;
4541
import java.util.List;
4642
import java.util.Map;
4743
import java.util.Optional;
4844

49-
import static com.dotcms.util.CollectionsUtils.list;
5045
import static org.junit.Assert.*;
5146
import static org.mockito.Mockito.mock;
5247
import static org.mockito.Mockito.when;
@@ -435,7 +430,7 @@ public void test_get_dependencies_with_bad_content_value() {
435430
assertNotNull(contentletIdList);
436431
assertTrue(contentletIdList.isEmpty());
437432
}
438-
433+
439434
@Test
440435
public void test_get_refreshStoryBlockValueReferences_with_bad_content_value() {
441436

@@ -469,6 +464,143 @@ public void test_refreshStoryBlockValueReferences_with_json_value() {
469464

470465
}
471466

467+
/**
468+
* Method to test: {@link StoryBlockAPI#refreshReferences(Contentlet)}
469+
* Given Scenario: This will create 2 block contents, adds a rich content to each block content and retrieve the json.
470+
* ExpectedResult: The new json will contain the rich text data map for each block content.
471+
*/
472+
@Test
473+
public void test_refresh_references_multiple_blocks()
474+
throws DotDataException, DotSecurityException, JsonProcessingException, IOException {
475+
476+
ContentType storyBlockType = null;
477+
478+
final HttpServletRequest oldThreadRequest = HttpServletRequestThreadLocal.INSTANCE.getRequest();
479+
final HttpServletResponse oldThreadResponse = HttpServletResponseThreadLocal.INSTANCE.getResponse();
480+
481+
try {
482+
// 1) get the default language
483+
final Language defaultLanguage = APILocator.getLanguageAPI().getDefaultLanguage();
484+
485+
// 2) create 2 rich text contentlets with some initial values
486+
final ContentType contentTypeRichText = APILocator.getContentTypeAPI(APILocator.systemUser()).find("webPageContent");
487+
final Contentlet richTextContentlet1 = new ContentletDataGen(contentTypeRichText)
488+
.languageId(defaultLanguage.getId())
489+
.setProperty("title","Title1")
490+
.setProperty("body", TestDataUtils.BLOCK_EDITOR_DUMMY_CONTENT)
491+
.nextPersistedAndPublish();
492+
final Contentlet richTextContentlet2 = new ContentletDataGen(contentTypeRichText)
493+
.languageId(defaultLanguage.getId())
494+
.setProperty("title","Title2")
495+
.setProperty("body", TestDataUtils.BLOCK_EDITOR_DUMMY_CONTENT)
496+
.nextPersistedAndPublish();
497+
498+
// 3) create a StoryBlockField and a ContentType with it
499+
final Field storyBlockField = new FieldDataGen()
500+
.type(StoryBlockField.class)
501+
.name("StoryBlockTestField")
502+
.velocityVarName("storyBlockTestField")
503+
.next();
504+
storyBlockType = new ContentTypeDataGen().field(storyBlockField).nextPersisted();
505+
506+
// 4) create first block content with first rich content
507+
final Contentlet firstBlockContentlet = new ContentletDataGen(storyBlockType)
508+
.languageId(defaultLanguage.getId())
509+
.nextPersisted();
510+
final Contentlet firstBlockCheckout = ContentletDataGen.checkout(firstBlockContentlet);
511+
setBlockEditorField(firstBlockCheckout, storyBlockField, richTextContentlet1);
512+
final Contentlet firstBlockComplete = APILocator.getContentletAPI().checkin(
513+
firstBlockCheckout, APILocator.systemUser(), false);
514+
ContentletDataGen.publish(firstBlockComplete);
515+
516+
// 5) create second block content with second rich content
517+
final Contentlet secondBlockContentlet = new ContentletDataGen(storyBlockType)
518+
.languageId(defaultLanguage.getId())
519+
.nextPersisted();
520+
final Contentlet secondBlockCheckout = ContentletDataGen.checkout(secondBlockContentlet);
521+
setBlockEditorField(secondBlockCheckout, storyBlockField, richTextContentlet2);
522+
final Contentlet secondBlockComplete = APILocator.getContentletAPI().checkin(
523+
secondBlockCheckout, APILocator.systemUser(), false);
524+
ContentletDataGen.publish(secondBlockComplete);
525+
526+
// 6) now we have 2 block contents, each one with a rich content, we are going to refresh the references
527+
final HttpServletRequest attrRequest = new MockAttributeRequest(mock(HttpServletRequest.class));
528+
attrRequest.setAttribute("USER", APILocator.systemUser());
529+
attrRequest.setAttribute(WebKeys.PAGE_MODE_PARAMETER, PageMode.LIVE);
530+
attrRequest.setAttribute(WebKeys.HTMLPAGE_LANGUAGE, "1");
531+
532+
final HttpServletRequest request = VelocityRequestWrapper.wrapVelocityRequest(attrRequest);
533+
HttpServletRequestThreadLocal.INSTANCE.setRequest(request);
534+
535+
final HttpServletResponse response = mock(HttpServletResponse.class);
536+
HttpServletResponseThreadLocal.INSTANCE.setResponse(response);
537+
538+
// 7) verify first rich content
539+
final Contentlet firstBlockPublished = APILocator.getContentletAPI().find(
540+
firstBlockComplete.getInode(), APILocator.systemUser(), false);
541+
assertNotNull(firstBlockPublished);
542+
543+
final Map<?, ?> firstDataMap = getDataMap(firstBlockPublished, storyBlockField.variable());
544+
assertNotNull(firstDataMap);
545+
546+
assertEquals(richTextContentlet1.getIdentifier(), firstDataMap.get("identifier"));
547+
assertEquals(richTextContentlet1.getStringProperty("title"), firstDataMap.get("title"));
548+
549+
// 8) verify second rich content
550+
final Contentlet secondBlockPublished = APILocator.getContentletAPI().find(
551+
secondBlockComplete.getInode(), APILocator.systemUser(), false);
552+
assertNotNull(secondBlockPublished);
553+
554+
final Map<?, ?> secondDataMap = getDataMap(secondBlockPublished, storyBlockField.variable());
555+
assertNotNull(secondDataMap);
556+
557+
assertEquals(richTextContentlet2.getIdentifier(), secondDataMap.get("identifier"));
558+
assertEquals(richTextContentlet2.getStringProperty("title"), secondDataMap.get("title"));
559+
560+
} finally {
561+
HttpServletRequestThreadLocal.INSTANCE.setRequest(oldThreadRequest);
562+
HttpServletResponseThreadLocal.INSTANCE.setResponse(oldThreadResponse);
563+
if (storyBlockType != null) {
564+
ContentTypeDataGen.remove(storyBlockType);
565+
}
566+
}
567+
}
568+
569+
/**
570+
* Helper method to get the data map from the first Contentlet that is referenced in a StoryBlockField.
571+
* @param storyBlockContentlet The Contentlet that contains the StoryBlockField.
572+
* @param storyBlockField The StoryBlockField variable name.
573+
* @return A map containing the data from the referenced Contentlet, or null if not found.
574+
*/
575+
private Map<?, ?> getDataMap(final Contentlet storyBlockContentlet, final String storyBlockField)
576+
throws JsonProcessingException {
577+
578+
if (storyBlockContentlet == null || storyBlockField == null) return null;
579+
580+
final Object storyBlockValue = storyBlockContentlet.getStringProperty(storyBlockField);
581+
if (storyBlockValue == null) return null;
582+
583+
final Map<String, Object> blockEditorMap =
584+
APILocator.getStoryBlockAPI().toMap(storyBlockValue);
585+
if (blockEditorMap == null || blockEditorMap.isEmpty()) return null;
586+
587+
final List<?> contentsMap = (List<?>) blockEditorMap.get(StoryBlockAPI.CONTENT_KEY);
588+
if (contentsMap == null || contentsMap.isEmpty()) return null;
589+
590+
final Optional<?> firstContentletMap = contentsMap.stream()
591+
.filter(contentMap -> "dotContent".equals(
592+
((Map<?,?>)contentMap).get("type")))
593+
.findFirst();
594+
if (firstContentletMap.isEmpty()) return null;
595+
596+
final Map<?, ?> contentletMap = (Map<?, ?>) firstContentletMap.get();
597+
final Map<?, ?> attrsMap = (Map<?, ?>) contentletMap.get(StoryBlockAPI.ATTRS_KEY);
598+
if (attrsMap == null) return null;
599+
600+
return (Map<?, ?>) attrsMap.get(StoryBlockAPI.DATA_KEY);
601+
602+
}
603+
472604
/**
473605
* Method to test: {@link StoryBlockAPIImpl#refreshReferences(Contentlet)}
474606
* When:

0 commit comments

Comments
 (0)