From fcc0050bdd35e8570757c87e50ca1874de1868db Mon Sep 17 00:00:00 2001 From: Martin Kouba Date: Wed, 13 Sep 2023 15:00:19 +0200 Subject: [PATCH] Qute: type pollution fixes - turn ResultNode in an abstract class - restrict the support of CompletionStage by default; i.e. only consider CompletableFuture and CompletedStage - in some cases intentionally use "instanceof" to test interfaces as the last resort in order to mitigate the type pollution - also make Qute's version of LazyValue Loom-friendly --- .../quarkus/qute/CompletionStageSupport.java | 33 ++++++++++++++++ .../java/io/quarkus/qute/EvaluatorImpl.java | 26 ++----------- .../java/io/quarkus/qute/ExpressionNode.java | 19 ++++++---- .../java/io/quarkus/qute/FieldAccessor.java | 8 +--- .../java/io/quarkus/qute/GetterAccessor.java | 8 +--- .../main/java/io/quarkus/qute/LazyValue.java | 20 +++++++--- .../io/quarkus/qute/LoopSectionHelper.java | 38 +++++++++++++------ .../java/io/quarkus/qute/MultiResultNode.java | 2 +- .../main/java/io/quarkus/qute/ResultNode.java | 6 +-- .../io/quarkus/qute/SingleResultNode.java | 2 +- .../main/java/io/quarkus/qute/TextNode.java | 2 +- 11 files changed, 97 insertions(+), 67 deletions(-) create mode 100644 independent-projects/qute/core/src/main/java/io/quarkus/qute/CompletionStageSupport.java diff --git a/independent-projects/qute/core/src/main/java/io/quarkus/qute/CompletionStageSupport.java b/independent-projects/qute/core/src/main/java/io/quarkus/qute/CompletionStageSupport.java new file mode 100644 index 0000000000000..5e1242cd16c72 --- /dev/null +++ b/independent-projects/qute/core/src/main/java/io/quarkus/qute/CompletionStageSupport.java @@ -0,0 +1,33 @@ +package io.quarkus.qute; + +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.CompletionStage; + +import io.smallrye.mutiny.operators.AbstractUni; + +class CompletionStageSupport { + + static final String SYSTEM_PROPERTY = "quarkus.qute.unrestricted-completion-stage-support"; + + /** + * {@code true} if any {@link CompletionStage} implementation is supported. {@code false} if only {@link CompletableFuture} + * and {@link CompletedStage} are considered in the API. + */ + static final boolean UNRESTRICTED = Boolean.getBoolean(SYSTEM_PROPERTY); + + @SuppressWarnings("unchecked") + static CompletionStage toCompletionStage(Object result) { + // Note that we intentionally use "instanceof" to test interfaces as the last resort in order to mitigate the "type pollution" + // See https://github.com/RedHatPerf/type-pollution-agent for more information + if (result instanceof CompletableFuture) { + return (CompletableFuture) result; + } else if (result instanceof CompletedStage) { + return (CompletedStage) result; + } else if (result instanceof AbstractUni) { + return ((AbstractUni) result).subscribeAsCompletionStage(); + } else if (UNRESTRICTED && result instanceof CompletionStage) { + return (CompletionStage) result; + } + return CompletedStage.of(result); + } +} diff --git a/independent-projects/qute/core/src/main/java/io/quarkus/qute/EvaluatorImpl.java b/independent-projects/qute/core/src/main/java/io/quarkus/qute/EvaluatorImpl.java index d152524b1253b..5171f7372476e 100644 --- a/independent-projects/qute/core/src/main/java/io/quarkus/qute/EvaluatorImpl.java +++ b/independent-projects/qute/core/src/main/java/io/quarkus/qute/EvaluatorImpl.java @@ -8,7 +8,6 @@ import java.util.List; import java.util.Map; import java.util.Map.Entry; -import java.util.concurrent.CompletableFuture; import java.util.concurrent.CompletionStage; import org.jboss.logging.Logger; @@ -16,7 +15,6 @@ import io.quarkus.qute.Expression.Part; import io.quarkus.qute.ExpressionImpl.PartImpl; import io.quarkus.qute.Results.NotFound; -import io.smallrye.mutiny.operators.AbstractUni; class EvaluatorImpl implements Evaluator { @@ -78,7 +76,7 @@ public CompletionStage evaluate(Expression expression, ResolutionContext // Very often a single matching resolver will be found return matching[0].resolve(context).thenCompose(r -> (parts.size() > 1) ? resolveReference(false, r, parts, resolutionContext, expression, 1) - : toCompletionStage(r)); + : CompletionStageSupport.toCompletionStage(r)); } else { // Multiple namespace resolvers match return resolveNamespace(context, resolutionContext, parts, matching, 0, expression); @@ -113,7 +111,7 @@ private CompletionStage resolveNamespace(EvalContext context, Resolution } else if (parts.size() > 1) { return resolveReference(false, r, parts, resolutionContext, expression, 1); } else { - return toCompletionStage(r); + return CompletionStageSupport.toCompletionStage(r); } }); } @@ -144,7 +142,7 @@ private CompletionStage resolve(EvalContextImpl evalContext, Iterator resolve(EvalContextImpl evalContext, Iterator toCompletionStage(Object result) { - // Note that we intentionally avoid "result instanceof Uni"; see https://github.com/RedHatPerf/type-pollution-agent - // Unfortunatelly, we can't get rid of "result instanceof CompletionStage" so we at least try to test CompletableFuture and CompletedStage first - if (result instanceof CompletableFuture) { - return (CompletableFuture) result; - } else if (result instanceof CompletedStage) { - return (CompletedStage) result; - } else if (result instanceof AbstractUni) { - return ((AbstractUni) result).subscribeAsCompletionStage(); - } else if (result instanceof CompletionStage) { - return (CompletionStage) result; - } - return CompletedStage.of(result); - } - private TemplateException propertyNotFound(Object result, Expression expression) { String propertyMessage; if (result instanceof NotFound) { diff --git a/independent-projects/qute/core/src/main/java/io/quarkus/qute/ExpressionNode.java b/independent-projects/qute/core/src/main/java/io/quarkus/qute/ExpressionNode.java index 0311ce8bd407a..3cc1425adbf9b 100644 --- a/independent-projects/qute/core/src/main/java/io/quarkus/qute/ExpressionNode.java +++ b/independent-projects/qute/core/src/main/java/io/quarkus/qute/ExpressionNode.java @@ -2,15 +2,15 @@ import java.util.Collections; import java.util.List; +import java.util.concurrent.CompletableFuture; import java.util.concurrent.CompletionStage; -import java.util.function.Function; import org.jboss.logging.Logger; /** * This node holds a single expression such as {@code foo.bar}. */ -class ExpressionNode implements TemplateNode, Function> { +class ExpressionNode implements TemplateNode { private static final Logger LOG = Logger.getLogger("io.quarkus.qute.nodeResolve"); @@ -18,12 +18,14 @@ class ExpressionNode implements TemplateNode, Function resolve(ResolutionContext context) { if (traceLevel) { LOG.tracef("Resolve {%s} started:%s", expression.toOriginalString(), expression.getOrigin()); } - return context.evaluate(expression).thenCompose(this); + return context.evaluate(expression).thenCompose(this::toResultNode); } - @Override - public CompletionStage apply(Object result) { + CompletionStage toResultNode(Object result) { if (traceLevel) { LOG.tracef("Resolve {%s} completed:%s", expression.toOriginalString(), expression.getOrigin()); } if (result instanceof ResultNode) { return CompletedStage.of((ResultNode) result); - } else if (result instanceof CompletionStage) { - return ((CompletionStage) result).thenCompose(this); + } else if (result instanceof CompletableFuture) { + return (CompletableFuture) ((CompletionStage) result).thenCompose(this::toResultNode); + } else if (result instanceof CompletedStage) { + return (CompletableFuture) ((CompletionStage) result).thenCompose(this::toResultNode); + } else if (unrestrictedCompletionStages && result instanceof CompletionStage) { + return ((CompletionStage) result).thenCompose(this::toResultNode); } else { return CompletedStage.of(new SingleResultNode(result, this)); } diff --git a/independent-projects/qute/core/src/main/java/io/quarkus/qute/FieldAccessor.java b/independent-projects/qute/core/src/main/java/io/quarkus/qute/FieldAccessor.java index 48fe7c4a33f9b..c8636ae393041 100644 --- a/independent-projects/qute/core/src/main/java/io/quarkus/qute/FieldAccessor.java +++ b/independent-projects/qute/core/src/main/java/io/quarkus/qute/FieldAccessor.java @@ -15,16 +15,10 @@ class FieldAccessor implements ValueAccessor, AccessorCandidate { this.field = field; } - @SuppressWarnings("unchecked") @Override public CompletionStage getValue(Object instance) { try { - Object ret = field.get(instance); - if (ret instanceof CompletionStage) { - return (CompletionStage) ret; - } else { - return CompletedStage.of(ret); - } + return CompletionStageSupport.toCompletionStage(field.get(instance)); } catch (Exception e) { throw new IllegalStateException("Reflection invocation error", e); } diff --git a/independent-projects/qute/core/src/main/java/io/quarkus/qute/GetterAccessor.java b/independent-projects/qute/core/src/main/java/io/quarkus/qute/GetterAccessor.java index 67d0b2266f4e2..7d5c5feeb8dc6 100644 --- a/independent-projects/qute/core/src/main/java/io/quarkus/qute/GetterAccessor.java +++ b/independent-projects/qute/core/src/main/java/io/quarkus/qute/GetterAccessor.java @@ -15,16 +15,10 @@ class GetterAccessor implements ValueAccessor, AccessorCandidate { this.method = method; } - @SuppressWarnings("unchecked") @Override public CompletionStage getValue(Object instance) { try { - Object ret = method.invoke(instance); - if (ret instanceof CompletionStage) { - return (CompletionStage) ret; - } else { - return CompletedStage.of(ret); - } + return CompletionStageSupport.toCompletionStage(method.invoke(instance)); } catch (Exception e) { throw new IllegalStateException("Reflection invocation error", e); } diff --git a/independent-projects/qute/core/src/main/java/io/quarkus/qute/LazyValue.java b/independent-projects/qute/core/src/main/java/io/quarkus/qute/LazyValue.java index 53f3195feedba..0efec4d8472ac 100644 --- a/independent-projects/qute/core/src/main/java/io/quarkus/qute/LazyValue.java +++ b/independent-projects/qute/core/src/main/java/io/quarkus/qute/LazyValue.java @@ -1,11 +1,15 @@ package io.quarkus.qute; +import java.util.concurrent.locks.Lock; +import java.util.concurrent.locks.ReentrantLock; import java.util.function.Supplier; -final class LazyValue { +class LazyValue { private final Supplier supplier; + private final Lock lock = new ReentrantLock(); + private transient volatile T value; public LazyValue(Supplier supplier) { @@ -17,11 +21,15 @@ public T get() { if (valueCopy != null) { return valueCopy; } - synchronized (this) { + + lock.lock(); + try { if (value == null) { value = supplier.get(); } return value; + } finally { + lock.unlock(); } } @@ -30,13 +38,13 @@ public T getIfPresent() { } public void clear() { - synchronized (this) { - value = null; - } + lock.lock(); + value = null; + lock.unlock(); } public boolean isSet() { return value != null; } -} \ No newline at end of file +} diff --git a/independent-projects/qute/core/src/main/java/io/quarkus/qute/LoopSectionHelper.java b/independent-projects/qute/core/src/main/java/io/quarkus/qute/LoopSectionHelper.java index bbc8013b76caf..d5b38ce6cc2c3 100644 --- a/independent-projects/qute/core/src/main/java/io/quarkus/qute/LoopSectionHelper.java +++ b/independent-projects/qute/core/src/main/java/io/quarkus/qute/LoopSectionHelper.java @@ -3,6 +3,8 @@ import static io.quarkus.qute.Parameter.EMPTY; import java.lang.reflect.Array; +import java.util.AbstractCollection; +import java.util.AbstractMap; import java.util.ArrayList; import java.util.Collection; import java.util.Collections; @@ -71,27 +73,31 @@ public CompletionStage resolve(SectionResolutionContext context) { } private static int extractSize(Object it) { - if (it instanceof Collection) { - return ((Collection) it).size(); - } else if (it instanceof Map) { - return ((Map) it).size(); + // Note that we intentionally use "instanceof" to test interfaces as the last resort in order to mitigate the "type pollution" + // See https://github.com/RedHatPerf/type-pollution-agent for more information + if (it instanceof AbstractCollection) { + return ((AbstractCollection) it).size(); + } else if (it instanceof AbstractMap) { + return ((AbstractMap) it).size(); } else if (it.getClass().isArray()) { return Array.getLength(it); } else if (it instanceof Integer) { return ((Integer) it); + } else if (it instanceof Collection) { + return ((Collection) it).size(); + } else if (it instanceof Map) { + return ((Map) it).size(); } return 10; } private Iterator extractIterator(Object it) { - if (it instanceof Iterable) { - return ((Iterable) it).iterator(); - } else if (it instanceof Iterator) { - return (Iterator) it; - } else if (it instanceof Map) { - return ((Map) it).entrySet().iterator(); - } else if (it instanceof Stream) { - return ((Stream) it).sequential().iterator(); + // Note that we intentionally use "instanceof" to test interfaces as the last resort in order to mitigate the "type pollution" + // See https://github.com/RedHatPerf/type-pollution-agent for more information + if (it instanceof AbstractCollection) { + return ((AbstractCollection) it).iterator(); + } else if (it instanceof AbstractMap) { + return ((AbstractMap) it).entrySet().iterator(); } else if (it instanceof Integer) { return IntStream.rangeClosed(1, (Integer) it).iterator(); } else if (it.getClass().isArray()) { @@ -102,6 +108,14 @@ private Iterator extractIterator(Object it) { elements.add(Array.get(it, i)); } return elements.iterator(); + } else if (it instanceof Iterable) { + return ((Iterable) it).iterator(); + } else if (it instanceof Iterator) { + return (Iterator) it; + } else if (it instanceof Map) { + return ((Map) it).entrySet().iterator(); + } else if (it instanceof Stream) { + return ((Stream) it).sequential().iterator(); } else { TemplateException.Builder builder; if (Results.isNotFound(it)) { diff --git a/independent-projects/qute/core/src/main/java/io/quarkus/qute/MultiResultNode.java b/independent-projects/qute/core/src/main/java/io/quarkus/qute/MultiResultNode.java index 392a423e3417c..ade187dee54dc 100644 --- a/independent-projects/qute/core/src/main/java/io/quarkus/qute/MultiResultNode.java +++ b/independent-projects/qute/core/src/main/java/io/quarkus/qute/MultiResultNode.java @@ -6,7 +6,7 @@ /** * A result node backed by an array of result nodes. */ -public final class MultiResultNode implements ResultNode { +public final class MultiResultNode extends ResultNode { private final Supplier[] results; diff --git a/independent-projects/qute/core/src/main/java/io/quarkus/qute/ResultNode.java b/independent-projects/qute/core/src/main/java/io/quarkus/qute/ResultNode.java index 6db63739a01c5..aa12fbaed1bbb 100644 --- a/independent-projects/qute/core/src/main/java/io/quarkus/qute/ResultNode.java +++ b/independent-projects/qute/core/src/main/java/io/quarkus/qute/ResultNode.java @@ -6,9 +6,9 @@ /** * Node of a result tree. */ -public interface ResultNode { +public abstract class ResultNode { - static CompletionStage NOOP = CompletedStage.of(new ResultNode() { + public static CompletionStage NOOP = CompletedStage.of(new ResultNode() { @Override public void process(Consumer resultConsumer) { } @@ -18,6 +18,6 @@ public void process(Consumer resultConsumer) { * * @param resultConsumer */ - void process(Consumer resultConsumer); + public abstract void process(Consumer resultConsumer); } diff --git a/independent-projects/qute/core/src/main/java/io/quarkus/qute/SingleResultNode.java b/independent-projects/qute/core/src/main/java/io/quarkus/qute/SingleResultNode.java index 36fe86500fb8e..bfbe85078e08f 100644 --- a/independent-projects/qute/core/src/main/java/io/quarkus/qute/SingleResultNode.java +++ b/independent-projects/qute/core/src/main/java/io/quarkus/qute/SingleResultNode.java @@ -9,7 +9,7 @@ /** * A result node backed by a single object value. */ -public final class SingleResultNode implements ResultNode { +public final class SingleResultNode extends ResultNode { private final Object value; private final ExpressionNode node; diff --git a/independent-projects/qute/core/src/main/java/io/quarkus/qute/TextNode.java b/independent-projects/qute/core/src/main/java/io/quarkus/qute/TextNode.java index 453d624263192..9bd0a4a66184b 100644 --- a/independent-projects/qute/core/src/main/java/io/quarkus/qute/TextNode.java +++ b/independent-projects/qute/core/src/main/java/io/quarkus/qute/TextNode.java @@ -6,7 +6,7 @@ /** * Static text. */ -public class TextNode implements TemplateNode, ResultNode { +public class TextNode extends ResultNode implements TemplateNode { private final CompletedStage result; private final String value;