Skip to content

Commit

Permalink
SITES-24155 - Content fragment list order by error (#2842)
Browse files Browse the repository at this point in the history 
* added collator based sorting on the query for content fragments
* extended unit tests
  • Loading branch information
@LSantha
LSantha authored Sep 3, 2024
1 parent 37aee59 commit 099ecbd
Show file tree
Hide file tree
Showing 3 changed files with 97 additions and 8 deletions.
47 changes: 40 additions & 7 deletions ...be/cq/wcm/core/components/internal/models/v1/contentfragment/ContentFragmentListImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,23 +15,25 @@
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~*/
package com.adobe.cq.wcm.core.components.internal.models.v1.contentfragment;

import java.text.Collator;
import java.util.*;

import javax.annotation.PostConstruct;
import javax.jcr.RepositoryException;
import javax.jcr.Session;
import javax.jcr.query.Row;

import com.day.cq.search.eval.AbstractPredicateEvaluator;
import com.day.cq.search.eval.EvaluationContext;
import com.day.cq.wcm.api.Page;
import org.apache.commons.lang3.StringUtils;
import org.apache.sling.api.SlingHttpServletRequest;
import org.apache.sling.api.resource.Resource;
import org.apache.sling.api.resource.ResourceResolver;
import org.apache.sling.models.annotations.Default;
import org.apache.sling.models.annotations.Exporter;
import org.apache.sling.models.annotations.Model;
import org.apache.sling.models.annotations.injectorspecific.InjectionStrategy;
import org.apache.sling.models.annotations.injectorspecific.OSGiService;
import org.apache.sling.models.annotations.injectorspecific.Self;
import org.apache.sling.models.annotations.injectorspecific.SlingObject;
import org.apache.sling.models.annotations.injectorspecific.ValueMapValue;
import org.apache.sling.models.annotations.injectorspecific.*;
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;
import org.slf4j.Logger;
Expand Down Expand Up @@ -63,7 +65,6 @@
)
@Exporter(name = ExporterConstants.SLING_MODEL_EXPORTER_NAME, extensions = ExporterConstants.SLING_MODEL_EXTENSION)
public class ContentFragmentListImpl extends AbstractComponentImpl implements ContentFragmentList {

private static final Logger LOG = LoggerFactory.getLogger(ContentFragmentListImpl.class);

public static final String RESOURCE_TYPE_V1 = "core/wcm/components/contentfragmentlist/v1/contentfragmentlist";
Expand All @@ -72,6 +73,7 @@ public class ContentFragmentListImpl extends AbstractComponentImpl implements Co
public static final String DEFAULT_DAM_PARENT_PATH = "/content/dam";

public static final int DEFAULT_MAX_ITEMS = -1;
public static final String CF_COMPARATOR_REF = "ContentFragmentComparatorRef";

@Self(injectionStrategy = InjectionStrategy.REQUIRED)
private SlingHttpServletRequest slingHttpServletRequest;
Expand Down Expand Up @@ -110,6 +112,9 @@ public class ContentFragmentListImpl extends AbstractComponentImpl implements Co
@Default(values = Predicate.SORT_ASCENDING)
private String sortOrder;

@ScriptVariable
protected Page currentPage;

private final List<DAMContentFragment> items = new ArrayList<>();

@PostConstruct
Expand Down Expand Up @@ -144,7 +149,7 @@ private void initModel() {
queryParameterMap.put("1_property.value", modelPath);

if (StringUtils.isNotEmpty(orderBy)) {
queryParameterMap.put("orderby", "@" + orderBy);
queryParameterMap.put("orderby", CF_COMPARATOR_REF);
if (StringUtils.isNotEmpty(sortOrder)) {
queryParameterMap.put("orderby.sort", sortOrder);
}
Expand All @@ -169,6 +174,11 @@ private void initModel() {
PredicateGroup predicateGroup = PredicateGroup.create(queryParameterMap);
Query query = queryBuilder.createQuery(predicateGroup, session);

if (StringUtils.isNotEmpty(orderBy)) {
Locale locale = currentPage != null ? currentPage.getLanguage(false) : Locale.getDefault();
query.registerPredicateEvaluator(CF_COMPARATOR_REF, new ContentFragmentPredicateEvaluator(locale));
}

SearchResult searchResult = query.getResult();

LOG.debug("Query statement: '{}'", searchResult.getQueryStatement());
Expand Down Expand Up @@ -209,4 +219,27 @@ public Collection<DAMContentFragment> getListItems() {
public String getExportedType() {
return slingHttpServletRequest.getResource().getResourceType();
}

class ContentFragmentPredicateEvaluator extends AbstractPredicateEvaluator {
private final Comparator<String> stringComparator;

public ContentFragmentPredicateEvaluator(Locale locale) {
Collator collator = Collator.getInstance(locale);
this.stringComparator = Comparator.nullsLast(collator);
}

@Override
public Comparator<Row> getOrderByComparator(Predicate predicate, EvaluationContext context) {
return (row1, row2) -> stringComparator.compare(getString(row1), getString(row2));
}

private String getString(Row row) {
try {
String str = row.getNode().getProperty(orderBy).getString();
return StringUtils.isBlank(str) ? null : str;
} catch (RepositoryException e) {
return null;
}
}
}
}
8 changes: 8 additions & 0 deletions ...q/wcm/core/components/internal/models/v1/contentfragment/AbstractContentFragmentTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,9 @@

import java.io.ByteArrayInputStream;
import java.nio.charset.StandardCharsets;
import java.util.Locale;

import com.day.cq.wcm.api.Page;
import org.apache.sling.api.resource.Resource;
import org.apache.sling.api.resource.ResourceResolver;
import org.apache.sling.api.resource.ValueMap;
Expand All @@ -33,7 +35,9 @@
import com.day.cq.search.QueryBuilder;
import io.wcm.testing.mock.aem.junit5.AemContext;

import static org.mockito.ArgumentMatchers.anyBoolean;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

public abstract class AbstractContentFragmentTest<T> {

Expand Down Expand Up @@ -148,6 +152,10 @@ T getModelInstanceUnderTest(String resourceName) {
slingBindings.put(SlingBindings.RESOLVER, resourceResolver);
slingBindings.put(SlingBindings.RESOURCE, resource);
slingBindings.put(WCMBindings.PROPERTIES, resource.adaptTo(ValueMap.class));
Page currentPage = mock(Page.class);
when(currentPage.getLanguage(anyBoolean())).thenReturn(Locale.getDefault());
when(currentPage.getContentResource()).thenReturn(resource);
slingBindings.put(WCMBindings.CURRENT_PAGE, currentPage);
httpServletRequest.setAttribute(SlingBindings.class.getName(), slingBindings);
httpServletRequest.setContextPath(CONTEXT_PATH);
return httpServletRequest.adaptTo(getClassType());
Expand Down
50 changes: 49 additions & 1 deletion ...q/wcm/core/components/internal/models/v1/contentfragment/ContentFragmentListImplTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,19 @@
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~*/
package com.adobe.cq.wcm.core.components.internal.models.v1.contentfragment;

import java.util.ArrayList;
import java.util.Collections;
import java.util.Comparator;
import java.util.HashMap;
import java.util.Iterator;
import java.util.Locale;
import java.util.Map;
import javax.jcr.Node;
import javax.jcr.Property;
import javax.jcr.Session;
import javax.jcr.query.Row;

import com.day.cq.search.eval.PredicateEvaluator;
import org.apache.sling.api.resource.Resource;
import org.apache.sling.api.resource.ResourceResolver;
import org.junit.jupiter.api.BeforeEach;
Expand All @@ -39,6 +47,7 @@

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.mockito.ArgumentMatchers.anyString;
import static org.mockito.Mockito.when;

@ExtendWith(AemContextExtension.class)
Expand Down Expand Up @@ -166,7 +175,7 @@ void verifyQueryBuilderInteractionWhenOrderByIsGiven() {
"property", "jcr:content/data/cq:model",
"value", "foobar"));
expectedPredicates.put("orderby", ImmutableMap.of(
"orderby", "@main",
"orderby", ContentFragmentListImpl.CF_COMPARATOR_REF,
"sort", "desc"));


Expand All @@ -177,6 +186,45 @@ void verifyQueryBuilderInteractionWhenOrderByIsGiven() {
verifyPredicateGroup(expectedPredicates, DEFAULT_NO_MAX_LIMIT_SET);
}

@Test
void verifyPredicateEvaluator() {
// GIVEN
// WHEN
ContentFragmentListImpl underTest = (ContentFragmentListImpl) getModelInstanceUnderTest(MODEL_ORDER_BY);
Locale locale = Locale.US;

// THEN
PredicateEvaluator pe = underTest.new ContentFragmentPredicateEvaluator(locale);
Comparator<Row> comparator = pe.getOrderByComparator(null, null);
assertNotNull(comparator);

ArrayList<Row> rows = new ArrayList<>();
Row r1 = createRow("abc");
rows.add(r1);
Row r2 = createRow("def");
rows.add(r2);
Row r3 = createRow("ábc");
rows.add(r3);
Collections.sort(rows, comparator);
assertEquals(r1, rows.get(0));
assertEquals(r3, rows.get(1));
assertEquals(r2, rows.get(2));
}

private Row createRow(String value) {
Row row = Mockito.mock(Row.class);
try {
Node node = Mockito.mock(Node.class);
Property property = Mockito.mock(Property.class);
when(property.getString()).thenReturn(value);
when(node.getProperty(anyString())).thenReturn(property);
when(row.getNode()).thenReturn(node);
} catch (Exception e) {
// ignore
}
return row;
}

@Test
void verifyLeakingResourceResolverIsClosed() {
// GIVEN
Expand Down

0 comments on commit 099ecbd

Please sign in to comment.