This is an automated email from the ASF dual-hosted git repository. reschke pushed a commit to branch SLING-13287-1.x in repository https://gitbox.apache.org/repos/asf/sling-org-apache-sling-resourceresolver.git
commit ea049aaff8a4caed744ef2afe698d573639360cc Author: Julian Reschke <[email protected]> AuthorDate: Mon Aug 10 12:59:37 2026 +0200 SLING-13287: paging sanity checks - log type when not String (#226) --- pom.xml | 5 +- .../impl/mapping/PagedQueryIterator.java | 21 ++++- .../impl/mapping/PagedQueryIteratorTest.java | 45 +++++++++- .../resourceresolver/impl/mapping/TestLogger.java | 95 ++++++++++++++++++++++ 4 files changed, 159 insertions(+), 7 deletions(-) diff --git a/pom.xml b/pom.xml index f57a131e..c53cce4c 100644 --- a/pom.xml +++ b/pom.xml @@ -162,8 +162,9 @@ <scope>test</scope> </dependency> <dependency> - <groupId>org.slf4j</groupId> - <artifactId>slf4j-simple</artifactId> + <groupId>ch.qos.logback</groupId> + <artifactId>logback-classic</artifactId> + <version>1.6.1</version> <scope>test</scope> </dependency> <dependency> diff --git a/src/main/java/org/apache/sling/resourceresolver/impl/mapping/PagedQueryIterator.java b/src/main/java/org/apache/sling/resourceresolver/impl/mapping/PagedQueryIterator.java index 2081daa9..d98adb4c 100644 --- a/src/main/java/org/apache/sling/resourceresolver/impl/mapping/PagedQueryIterator.java +++ b/src/main/java/org/apache/sling/resourceresolver/impl/mapping/PagedQueryIterator.java @@ -24,6 +24,7 @@ import java.util.concurrent.TimeUnit; import org.apache.sling.api.resource.Resource; import org.apache.sling.api.resource.ResourceResolver; +import org.apache.sling.api.resource.ValueMap; import org.jetbrains.annotations.NotNull; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -82,29 +83,43 @@ public class PagedQueryIterator implements Iterator<Resource> { page += 1; } + private static String getDiagInformationWhenNotString(ValueMap valueMap, String propertyName) { + Object value = valueMap.get(propertyName); + if (value == null || value instanceof String[] || value instanceof String) { + // all good + return ""; + } else { + return " (type: '" + value.getClass() + "')"; + } + } + private Resource getNext() throws NoSuchElementException { Resource resource = it.next(); count += 1; - final String[] values = resource.getValueMap().get(propertyName, defaultValue); + + final ValueMap valueMap = resource.getValueMap(); + final String[] values = valueMap.get(propertyName, defaultValue); if (values.length > 0) { String value = values[0]; if (value.compareTo(lastKey) < 0) { log.warn( - "unexpected query result in page {}, property name '{}', got '{}', despite querying for > '{}'" + "unexpected query result in page {}, property name '{}', got '{}'{}, despite querying for > '{}'" + " (the async index may not yet reflect the current property value)", (page - 1), propertyName, value, + getDiagInformationWhenNotString(valueMap, propertyName), lastKey); } if (lastValue != null && value.compareTo(lastValue) < 0) { log.warn( - "unexpected query result in page {}, property name '{}', got '{}', last value was '{}'" + "unexpected query result in page {}, property name '{}', got '{}'{}, last value was '{}'" + " (the async index may not yet reflect the current property value)", (page - 1), propertyName, value, + getDiagInformationWhenNotString(valueMap, propertyName), lastValue); } diff --git a/src/test/java/org/apache/sling/resourceresolver/impl/mapping/PagedQueryIteratorTest.java b/src/test/java/org/apache/sling/resourceresolver/impl/mapping/PagedQueryIteratorTest.java index 24ac0777..33a013b8 100644 --- a/src/test/java/org/apache/sling/resourceresolver/impl/mapping/PagedQueryIteratorTest.java +++ b/src/test/java/org/apache/sling/resourceresolver/impl/mapping/PagedQueryIteratorTest.java @@ -22,7 +22,9 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; import java.util.Collections; +import java.util.Date; import java.util.Iterator; +import java.util.List; import java.util.Map; import java.util.function.Predicate; import java.util.stream.Collectors; @@ -31,12 +33,15 @@ import org.apache.sling.api.resource.QuerySyntaxException; import org.apache.sling.api.resource.Resource; import org.apache.sling.api.resource.ValueMap; import org.apache.sling.api.resource.path.Path; +import org.apache.sling.api.wrappers.ValueMapDecorator; +import org.apache.sling.api.wrappers.impl.ObjectConverter; import org.junit.Before; import org.junit.Test; import org.mockito.MockitoAnnotations; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.mock; @@ -98,14 +103,50 @@ public class PagedQueryIteratorTest extends AbstractMappingMapEntriesTest { checkResult(it, expected); } + @Test + public void testSimpleWrongType() { + try (TestLogger logger = TestLogger.create(PagedQueryIterator.class) + .contains("unexpected") + .start()) { + + String[] expected = new String[] {"a", "b", "c"}; + Collection<Resource> expectedResources = toResourceList(expected); + + Date oneMore = new Date(0); + ValueMap properties = new ValueMapDecorator(Map.of(PROPNAME, new Date[] {oneMore})); + Resource r = mock(Resource.class); + when(r.getValueMap()).thenReturn(properties); + + expectedResources.add(r); + + when(resourceResolver.findResources(eq("testSimpleWrongType"), eq("JCR-SQL2"))) + .thenReturn(expectedResources.iterator()); + PagedQueryIterator it = + new PagedQueryIterator("alias", PROPNAME, resourceResolver, "testSimpleWrongType", 2000); + + String[] expWithOneMore = Arrays.copyOf(expected, expected.length + 1); + // implementation detail: this assumes the way Sling converts Dates to Strings + expWithOneMore[expected.length] = ObjectConverter.convert(oneMore, String.class); + + checkResult(it, expWithOneMore); + + // implementation detail: assumes format of log message + List<String> logEntries = logger.stopAndGetLogs(); + assertTrue( + "Log should contain 'class [Ljava.util.Date;', but got: " + logEntries, + logEntries.toString().contains("class [Ljava.util.Date;")); + } + } + @Test public void testSimpleWrongResultAfterKey() { // SLING-13284: out-of-order results across page boundaries should not abort iteration String[] expected = new String[] {"x", "x", "a", "a"}; Collection<Resource> expectedResources = toResourceList(expected); - when(resourceResolver.findResources("testSimpleWrongOrder", "JCR-SQL2")) + when(resourceResolver.findResources("testSimpleWrongResultAfterKey", "JCR-SQL2")) .thenReturn(expectedResources.iterator()); - PagedQueryIterator it = new PagedQueryIterator("alias", PROPNAME, resourceResolver, "testSimpleWrongOrder", 1); + PagedQueryIterator it = + new PagedQueryIterator("alias", PROPNAME, resourceResolver, "testSimpleWrongResultAfterKey", 1); int count = 0; while (it.hasNext()) { it.next(); diff --git a/src/test/java/org/apache/sling/resourceresolver/impl/mapping/TestLogger.java b/src/test/java/org/apache/sling/resourceresolver/impl/mapping/TestLogger.java new file mode 100755 index 00000000..df2a9dca --- /dev/null +++ b/src/test/java/org/apache/sling/resourceresolver/impl/mapping/TestLogger.java @@ -0,0 +1,95 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.sling.resourceresolver.impl.mapping; + +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; + +import ch.qos.logback.classic.Logger; +import ch.qos.logback.classic.LoggerContext; +import ch.qos.logback.classic.spi.ILoggingEvent; +import ch.qos.logback.core.Appender; +import ch.qos.logback.core.AppenderBase; +import org.slf4j.LoggerFactory; + +// inspired by Oak's LogCustomizer +public class TestLogger implements AutoCloseable { + private final Appender<ILoggingEvent> customLogger; + + private final Logger delegate; + private String matchContainsMessage; + private final List<String> logs = Collections.synchronizedList(new ArrayList<>()); + + private TestLogger(Class<?> clazz) { + this.delegate = getLogger(clazz); + + this.customLogger = new AppenderBase<>() { + @Override + protected void append(ILoggingEvent e) { + String message = e.getFormattedMessage(); + if (matchContainsMessage == null || message.contains(matchContainsMessage)) { + logs.add(message); + } + } + }; + + this.customLogger.setContext((LoggerContext) LoggerFactory.getILoggerFactory()); + } + + public static TestLogger create(Class<?> clazz) { + return new TestLogger(clazz); + } + + public TestLogger start() { + if (delegate == null) { + throw new IllegalStateException(); + } + this.delegate.addAppender(this.customLogger); + this.customLogger.start(); + return this; + } + + public TestLogger contains(String matchContainsMessage) { + this.matchContainsMessage = matchContainsMessage; + return this; + } + + public List<String> stopAndGetLogs() { + if (this.delegate == null) { + throw new IllegalStateException(); + } + delegate.detachAppender(customLogger); + customLogger.stop(); + return logs; + } + + public void close() { + if (this.delegate == null) { + throw new IllegalStateException(); + } + delegate.detachAppender(customLogger); + customLogger.stop(); + logs.clear(); + } + + private static Logger getLogger(Class<?> clazz) { + return ((LoggerContext) LoggerFactory.getILoggerFactory()).getLogger(clazz); + } +}
