From 66391b44b6d5c9266120331ceda1c8f890bd3208 Mon Sep 17 00:00:00 2001 From: Martin Kouba Date: Tue, 7 Nov 2023 15:01:25 +0100 Subject: [PATCH] Qute: consider synthetic parameter declarations during validation - Template#getParameterDeclarations() now also returns type param declarations defined by parser hooks (i.e. type-safe templates and globals) --- .../qute/deployment/QuteProcessor.java | 186 +++++------------- .../TemplatesAnalysisBuildItem.java | 19 ++ .../qute/deployment/TemplateAnalysisTest.java | 87 ++++++++ .../io/quarkus/qute/ParameterDeclaration.java | 2 +- .../src/main/java/io/quarkus/qute/Parser.java | 18 +- .../java/io/quarkus/qute/ParserHelper.java | 3 +- .../src/main/java/io/quarkus/qute/Scope.java | 4 + .../main/java/io/quarkus/qute/Template.java | 5 +- .../java/io/quarkus/qute/TemplateNode.java | 7 + .../test/java/io/quarkus/qute/ParserTest.java | 12 ++ 10 files changed, 204 insertions(+), 139 deletions(-) create mode 100644 extensions/qute/deployment/src/test/java/io/quarkus/qute/deployment/TemplateAnalysisTest.java diff --git a/extensions/qute/deployment/src/main/java/io/quarkus/qute/deployment/QuteProcessor.java b/extensions/qute/deployment/src/main/java/io/quarkus/qute/deployment/QuteProcessor.java index d4e52487d5ecd..8497e70b4dedf 100644 --- a/extensions/qute/deployment/src/main/java/io/quarkus/qute/deployment/QuteProcessor.java +++ b/extensions/qute/deployment/src/main/java/io/quarkus/qute/deployment/QuteProcessor.java @@ -104,7 +104,6 @@ import io.quarkus.qute.ErrorCode; import io.quarkus.qute.Expression; import io.quarkus.qute.Expression.VirtualMethodPart; -import io.quarkus.qute.Expressions; import io.quarkus.qute.LoopSectionHelper; import io.quarkus.qute.NamespaceResolver; import io.quarkus.qute.ParameterDeclaration; @@ -121,7 +120,6 @@ import io.quarkus.qute.TemplateGlobal; import io.quarkus.qute.TemplateInstance; import io.quarkus.qute.TemplateLocator; -import io.quarkus.qute.TemplateNode; import io.quarkus.qute.UserTagSectionHelper; import io.quarkus.qute.ValueResolver; import io.quarkus.qute.Variant; @@ -657,49 +655,9 @@ public Optional getVariant() { } }); - // It's a file-based template - // We need to find out whether the parsed template represents a checked template - Map pathToPathWithoutSuffix = new HashMap<>(); - for (String path : filePaths.getFilePaths()) { - for (String suffix : config.suffixes) { - if (path.endsWith(suffix)) { - // Remove the suffix and add to Map - pathToPathWithoutSuffix.put(path, path.substring(0, path.length() - (suffix.length() + 1))); - break; - } - } - - // Path has already no suffix - if (!pathToPathWithoutSuffix.containsKey(path)) { - pathToPathWithoutSuffix.put(path, path); - } - } - - // Checked Template id -> method parameter declaration - Map> checkedTemplateIdToParamDecl = new HashMap<>(); - for (CheckedTemplateBuildItem checkedTemplate : checkedTemplates) { - if (checkedTemplate.isFragment()) { - continue; - } - for (Entry entry : checkedTemplate.bindings.entrySet()) { - checkedTemplateIdToParamDecl - .computeIfAbsent(checkedTemplate.templateId, s -> new HashMap<>()) - .put(entry.getKey(), new MethodParameterDeclaration(entry.getValue(), entry.getKey())); - } - } - - // Message Bundle Template id -> method parameter declaration - Map> msgBundleTemplateIdToParamDecl = new HashMap<>(); - for (MessageBundleMethodBuildItem messageBundleMethod : messageBundleMethods) { - MethodInfo method = messageBundleMethod.getMethod(); - for (ListIterator it = method.parameterTypes().listIterator(); it.hasNext();) { - Type paramType = it.next(); - String name = MessageBundleProcessor.getParameterName(method, it.previousIndex()); - msgBundleTemplateIdToParamDecl - .computeIfAbsent(messageBundleMethod.getTemplateId(), s -> new HashMap<>()) - .put(name, new MethodParameterDeclaration(getCheckedTemplateParameterTypeName(paramType), name)); - } - } + Map messageBundleMethodsMap = messageBundleMethods.stream() + .filter(MessageBundleMethodBuildItem::isValidatable) + .collect(Collectors.toMap(MessageBundleMethodBuildItem::getTemplateId, Function.identity())); builder.addParserHook(new ParserHook() { @@ -715,8 +673,17 @@ public void beforeParsing(ParserHelper parserHelper) { getCheckedTemplateParameterTypeName(global.getVariableType()).toString()); } - addMethodParamsToParserHelper(parserHelper, pathToPathWithoutSuffix.get(templateId), - checkedTemplateIdToParamDecl); + // It's a file-based template + // We need to find out whether the parsed template represents a checked template + String path = templatePathWithoutSuffix(templateId, config); + for (CheckedTemplateBuildItem checkedTemplate : checkedTemplates) { + if (checkedTemplate.templateId.equals(path)) { + for (Entry entry : checkedTemplate.bindings.entrySet()) { + parserHelper.addParameter(entry.getKey(), entry.getValue()); + } + break; + } + } if (templateId.startsWith(TemplatePathBuildItem.TAGS)) { parserHelper.addParameter(UserTagSectionHelper.Factory.ARGS, @@ -724,7 +691,16 @@ public void beforeParsing(ParserHelper parserHelper) { } } - addMethodParamsToParserHelper(parserHelper, templateId, msgBundleTemplateIdToParamDecl); + // If needed add params to message bundle templates + MessageBundleMethodBuildItem messageBundleMethod = messageBundleMethodsMap.get(templateId); + if (messageBundleMethod != null) { + MethodInfo method = messageBundleMethod.getMethod(); + for (ListIterator it = method.parameterTypes().listIterator(); it.hasNext();) { + Type paramType = it.next(); + String name = MessageBundleProcessor.getParameterName(method, it.previousIndex()); + parserHelper.addParameter(name, getCheckedTemplateParameterTypeName(paramType)); + } + } } }).build(); @@ -736,17 +712,7 @@ public void beforeParsing(ParserHelper parserHelper) { for (TemplatePathBuildItem path : templatePaths) { Template template = dummyEngine.getTemplate(path.getPath()); if (template != null) { - String templateIdWithoutSuffix = pathToPathWithoutSuffix.get(template.getId()); - - final List parameterDeclarations; - if (checkedTemplateIdToParamDecl.isEmpty()) { - parameterDeclarations = template.getParameterDeclarations(); - } else { - // Add method parameter declarations if they were not overridden in the template - parameterDeclarations = mergeParamDeclarations( - template.getParameterDeclarations(), - checkedTemplateIdToParamDecl.get(templateIdWithoutSuffix)); - } + String templateIdWithoutSuffix = templatePathWithoutSuffix(template.getId(), config); if (!checkedFragments.isEmpty()) { for (CheckedTemplateBuildItem checkedFragment : checkedFragments) { @@ -766,21 +732,15 @@ public void beforeParsing(ParserHelper parserHelper) { } analysis.add(new TemplateAnalysis(null, template.getGeneratedId(), template.getExpressions(), - parameterDeclarations, path.getPath(), template.getFragmentIds())); + template.getParameterDeclarations(), path.getPath(), template.getFragmentIds())); } } // Message bundle templates for (MessageBundleMethodBuildItem messageBundleMethod : messageBundleMethods) { Template template = dummyEngine.parse(messageBundleMethod.getTemplate(), null, messageBundleMethod.getTemplateId()); - - // Add method parameter declarations if they were not overridden in the template - List paramDeclarations = mergeParamDeclarations( - template.getParameterDeclarations(), - msgBundleTemplateIdToParamDecl.get(messageBundleMethod.getTemplateId())); - analysis.add(new TemplateAnalysis(messageBundleMethod.getTemplateId(), template.getGeneratedId(), - template.getExpressions(), paramDeclarations, + template.getExpressions(), template.getParameterDeclarations(), messageBundleMethod.getMethod().declaringClass().name() + "#" + messageBundleMethod.getMethod().name() + "()", template.getFragmentIds())); @@ -791,6 +751,17 @@ public void beforeParsing(ParserHelper parserHelper) { return new TemplatesAnalysisBuildItem(analysis); } + private String templatePathWithoutSuffix(String path, QuteConfig config) { + for (String suffix : config.suffixes) { + if (path.endsWith(suffix)) { + // Remove the suffix + path = path.substring(0, path.length() - (suffix.length() + 1)); + break; + } + } + return path; + } + @BuildStep void validateCheckedFragments(List validations, List expressionMatches, @@ -925,29 +896,6 @@ private static String getCheckedTemplateParameterParameterizedTypeName(Parameter return builder.toString(); } - private List mergeParamDeclarations(List parameterDeclarations, - Map paramNameToDeclaration) { - if (paramNameToDeclaration != null) { - Map mergeResult = new HashMap<>(paramNameToDeclaration); - for (ParameterDeclaration paramDeclaration : parameterDeclarations) { - // Template parameter declarations override method parameter declarations - mergeResult.put(paramDeclaration.getKey(), paramDeclaration); - } - return List.copyOf(mergeResult.values()); - } - return parameterDeclarations; - } - - private void addMethodParamsToParserHelper(ParserHelper parserHelper, String templateId, - Map> templateIdToParamDecl) { - var paramNameToDeclaration = templateIdToParamDecl.get(templateId); - if (paramNameToDeclaration != null) { - for (MethodParameterDeclaration parameterDeclaration : paramNameToDeclaration.values()) { - parserHelper.addParameter(parameterDeclaration.getKey(), parameterDeclaration.getParamType()); - } - } - } - @BuildStep void validateExpressions(TemplatesAnalysisBuildItem templatesAnalysis, BeanArchiveIndexBuildItem beanArchiveIndex, @@ -1481,17 +1429,24 @@ private static NamespaceResult processNamespace(Expression expression, MatchResu // data: Expression.Part firstPart = expression.getParts().get(0); String firstPartName = firstPart.getName(); - for (ParameterDeclaration paramDeclaration : templateAnalysis.parameterDeclarations) { - if (paramDeclaration.getKey().equals(firstPartName)) { - // Data Namespace expression has bounded parameter declaration - dataNamespaceTypeInfo = TypeInfos - .create(paramDeclaration.getTypeInfo(), firstPart, index, templateIdToPathFun, - expression.getOrigin()) - .asTypeInfo(); + // FIXME This is not entirely correct + // First we try to find a non-synthetic param declaration that matches the given name, + // and then we try the synthetic ones. + // However, this might result in confusing behavior when type-safe templates are used together with type-safe expressions. + // But this should not be a common use case. + ParameterDeclaration paramDeclaration = null; + for (ParameterDeclaration pd : templateAnalysis.getSortedParameterDeclarations()) { + if (pd.getKey().equals(firstPartName)) { + paramDeclaration = pd; break; } } - if (dataNamespaceTypeInfo == null) { + if (paramDeclaration != null) { + dataNamespaceTypeInfo = TypeInfos + .create(paramDeclaration.getTypeInfo(), firstPart, index, templateIdToPathFun, + expression.getOrigin()) + .asTypeInfo(); + } else { putResult(match, results, expression); ignored = true; } @@ -3538,39 +3493,4 @@ public String getName() { } - private static final class MethodParameterDeclaration implements ParameterDeclaration { - - private final String paramType; - private final String paramName; - - private MethodParameterDeclaration(String paramType, String paramName) { - this.paramType = paramType; - this.paramName = paramName; - } - - public String getParamType() { - return paramType; - } - - @Override - public String getTypeInfo() { - return Expressions.typeInfoFrom(paramType); - } - - @Override - public String getKey() { - return paramName; - } - - @Override - public Expression getDefaultValue() { - return null; - } - - @Override - public TemplateNode.Origin getOrigin() { - return null; - } - } - } diff --git a/extensions/qute/deployment/src/main/java/io/quarkus/qute/deployment/TemplatesAnalysisBuildItem.java b/extensions/qute/deployment/src/main/java/io/quarkus/qute/deployment/TemplatesAnalysisBuildItem.java index 830588305c4ec..d67efdf86ab79 100644 --- a/extensions/qute/deployment/src/main/java/io/quarkus/qute/deployment/TemplatesAnalysisBuildItem.java +++ b/extensions/qute/deployment/src/main/java/io/quarkus/qute/deployment/TemplatesAnalysisBuildItem.java @@ -1,5 +1,7 @@ package io.quarkus.qute.deployment; +import java.util.ArrayList; +import java.util.Comparator; import java.util.List; import java.util.Objects; import java.util.Set; @@ -62,6 +64,23 @@ Expression findExpression(int id) { return null; } + /** + * Non-synthetic declarations go first, then sorted by the line. + * + * @return the sorted list of parameter declarations + */ + public List getSortedParameterDeclarations() { + List ret = new ArrayList<>(parameterDeclarations); + ret.sort(new Comparator() { + @Override + public int compare(ParameterDeclaration pd1, ParameterDeclaration pd2) { + int ret = Boolean.compare(pd1.getOrigin().isSynthetic(), pd2.getOrigin().isSynthetic()); + return ret == 0 ? Integer.compare(pd1.getOrigin().getLine(), pd2.getOrigin().getLine()) : ret; + } + }); + return ret; + } + @Override public int hashCode() { final int prime = 31; diff --git a/extensions/qute/deployment/src/test/java/io/quarkus/qute/deployment/TemplateAnalysisTest.java b/extensions/qute/deployment/src/test/java/io/quarkus/qute/deployment/TemplateAnalysisTest.java new file mode 100644 index 0000000000000..143d778592b4f --- /dev/null +++ b/extensions/qute/deployment/src/test/java/io/quarkus/qute/deployment/TemplateAnalysisTest.java @@ -0,0 +1,87 @@ +package io.quarkus.qute.deployment; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.util.List; +import java.util.Optional; + +import org.junit.jupiter.api.Test; + +import io.quarkus.qute.Expression; +import io.quarkus.qute.ParameterDeclaration; +import io.quarkus.qute.TemplateNode.Origin; +import io.quarkus.qute.Variant; +import io.quarkus.qute.deployment.TemplatesAnalysisBuildItem.TemplateAnalysis; + +public class TemplateAnalysisTest { + + @Test + public void testSortedParamDeclarations() { + TemplateAnalysis analysis = new TemplateAnalysis(null, null, null, List.of(paramDeclaration("foo", -1), + paramDeclaration("bar", -1), paramDeclaration("qux", 10), paramDeclaration("baz", 1)), null, null); + List sorted = analysis.getSortedParameterDeclarations(); + assertEquals(4, sorted.size()); + assertEquals("baz", sorted.get(0).getKey()); + assertEquals("qux", sorted.get(1).getKey()); + assertTrue(sorted.get(2).getKey().equals("foo") || sorted.get(2).getKey().equals("bar")); + assertTrue(sorted.get(3).getKey().equals("foo") || sorted.get(3).getKey().equals("bar")); + } + + ParameterDeclaration paramDeclaration(String key, int line) { + return new ParameterDeclaration() { + + @Override + public String getTypeInfo() { + return null; + } + + @Override + public Origin getOrigin() { + return new Origin() { + + @Override + public Optional getVariant() { + return Optional.empty(); + } + + @Override + public String getTemplateId() { + return null; + } + + @Override + public String getTemplateGeneratedId() { + return null; + } + + @Override + public int getLineCharacterStart() { + return 0; + } + + @Override + public int getLineCharacterEnd() { + return 0; + } + + @Override + public int getLine() { + return line; + } + }; + } + + @Override + public String getKey() { + return key; + } + + @Override + public Expression getDefaultValue() { + return null; + } + }; + } + +} diff --git a/independent-projects/qute/core/src/main/java/io/quarkus/qute/ParameterDeclaration.java b/independent-projects/qute/core/src/main/java/io/quarkus/qute/ParameterDeclaration.java index 6a6da4bce112b..8373f9f3cd1d8 100644 --- a/independent-projects/qute/core/src/main/java/io/quarkus/qute/ParameterDeclaration.java +++ b/independent-projects/qute/core/src/main/java/io/quarkus/qute/ParameterDeclaration.java @@ -4,7 +4,7 @@ import io.quarkus.qute.TemplateNode.Origin; /** - * Represents a parameter declaration, i.e. {@org.acme.Foo foo}. + * Represents a type parameter declaration, i.e. {@org.acme.Foo foo}. */ public interface ParameterDeclaration { diff --git a/independent-projects/qute/core/src/main/java/io/quarkus/qute/Parser.java b/independent-projects/qute/core/src/main/java/io/quarkus/qute/Parser.java index dad999d56c211..b6eba251f7d9b 100644 --- a/independent-projects/qute/core/src/main/java/io/quarkus/qute/Parser.java +++ b/independent-projects/qute/core/src/main/java/io/quarkus/qute/Parser.java @@ -126,9 +126,19 @@ private Template currentTemplate() { Template parse() { - sectionStack.addFirst(SectionNode.builder(ROOT_HELPER_NAME, origin(0), this, this) + SectionNode.Builder rootBuilder = SectionNode.builder(ROOT_HELPER_NAME, syntheticOrigin(), this, this) .setEngine(engine) - .setHelperFactory(ROOT_SECTION_HELPER_FACTORY)); + .setHelperFactory(ROOT_SECTION_HELPER_FACTORY); + sectionStack.addFirst(rootBuilder); + + // Add synthetic nodes for param declarations added by parser hooks + Map bindings = scopeStack.peek().getBindings(); + if (bindings != null && !bindings.isEmpty()) { + for (Entry e : bindings.entrySet()) { + rootBuilder.currentBlock().addNode( + new ParameterDeclarationNode(e.getValue(), e.getKey(), null, syntheticOrigin())); + } + } long start = System.nanoTime(); Reader r = reader; @@ -1066,6 +1076,10 @@ Origin origin(int lineCharacterOffset) { return new OriginImpl(line, lineCharacter - lineCharacterOffset, lineCharacter, templateId, generatedId, variant); } + Origin syntheticOrigin() { + return new OriginImpl(-1, -1, -1, templateId, generatedId, variant); + } + private List> readLines(SectionNode rootNode) { List> lines = new ArrayList<>(); // Add the last line manually - there is no line separator to trigger flush diff --git a/independent-projects/qute/core/src/main/java/io/quarkus/qute/ParserHelper.java b/independent-projects/qute/core/src/main/java/io/quarkus/qute/ParserHelper.java index 9d25886f270b2..d44fb05de3c85 100644 --- a/independent-projects/qute/core/src/main/java/io/quarkus/qute/ParserHelper.java +++ b/independent-projects/qute/core/src/main/java/io/quarkus/qute/ParserHelper.java @@ -16,8 +16,7 @@ public interface ParserHelper { /** * Adds an implicit parameter declaration. This is an alternative approach to explicit parameter - * declarations - * used directly in the templates, e.g. {@org.acme.Foo foo}. + * declarations used directly in the templates, e.g. {@org.acme.Foo foo}. * * @param name * @param type diff --git a/independent-projects/qute/core/src/main/java/io/quarkus/qute/Scope.java b/independent-projects/qute/core/src/main/java/io/quarkus/qute/Scope.java index b60e10c5691d6..3760d45e2bac7 100644 --- a/independent-projects/qute/core/src/main/java/io/quarkus/qute/Scope.java +++ b/independent-projects/qute/core/src/main/java/io/quarkus/qute/Scope.java @@ -38,6 +38,10 @@ public String getBinding(String binding) { return parentScope != null ? parentScope.getBinding(binding) : null; } + Map getBindings() { + return bindings; + } + public String getBindingTypeOrDefault(String binding, String defaultValue) { String type = getBinding(binding); return type != null ? type : defaultValue; diff --git a/independent-projects/qute/core/src/main/java/io/quarkus/qute/Template.java b/independent-projects/qute/core/src/main/java/io/quarkus/qute/Template.java index 3f9008fa2f998..615d72b3da530 100644 --- a/independent-projects/qute/core/src/main/java/io/quarkus/qute/Template.java +++ b/independent-projects/qute/core/src/main/java/io/quarkus/qute/Template.java @@ -155,9 +155,12 @@ default String render() { Optional getVariant(); /** + * Returns all type parameter declarations of the template, including the declarations added by a + * {@link io.quarkus.qute.ParserHook}. + *

* If invoked upon a fragment instance then delegate to the defining template. * - * @return an immutable list of all parameter declarations defined in the template + * @return an immutable list of all type parameter declarations defined in the template */ List getParameterDeclarations(); diff --git a/independent-projects/qute/core/src/main/java/io/quarkus/qute/TemplateNode.java b/independent-projects/qute/core/src/main/java/io/quarkus/qute/TemplateNode.java index af05f6c74d283..23096cb61698c 100644 --- a/independent-projects/qute/core/src/main/java/io/quarkus/qute/TemplateNode.java +++ b/independent-projects/qute/core/src/main/java/io/quarkus/qute/TemplateNode.java @@ -107,6 +107,13 @@ default void appendTo(StringBuilder builder) { } } + /** + * @return {@code true} if the template node was not part of the original template, {@code false} otherwise + */ + default boolean isSynthetic() { + return getLine() == -1; + } + } } diff --git a/independent-projects/qute/core/src/test/java/io/quarkus/qute/ParserTest.java b/independent-projects/qute/core/src/test/java/io/quarkus/qute/ParserTest.java index 350b092e20513..6337cadc49d36 100644 --- a/independent-projects/qute/core/src/test/java/io/quarkus/qute/ParserTest.java +++ b/independent-projects/qute/core/src/test/java/io/quarkus/qute/ParserTest.java @@ -3,6 +3,7 @@ import static org.assertj.core.api.Assertions.assertThatExceptionOfType; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.junit.jupiter.api.Assertions.fail; @@ -165,6 +166,10 @@ public void testLines() { assertEquals(8, itemNameOrigin.getLine()); assertEquals(1, itemNameOrigin.getLineCharacterStart()); assertEquals(11, itemNameOrigin.getLineCharacterEnd()); + ParameterDeclaration pd = template.getParameterDeclarations().get(0); + assertEquals(1, pd.getOrigin().getLine()); + assertEquals("foo", pd.getKey()); + assertNull(pd.getDefaultValue()); } @Test @@ -316,9 +321,16 @@ public void testParserHook() { public void beforeParsing(ParserHelper parserHelper) { parserHelper.addContentFilter(contents -> contents.replace("bard", "bar")); parserHelper.addContentFilter(contents -> contents.replace("${", "$\\{")); + parserHelper.addParameter("foo", String.class.getName()); } }).build().parse("${foo}::{bard}"); assertEquals("${foo}::true", template.data("bar", true).render()); + List paramDeclarations = template.getParameterDeclarations(); + assertEquals(1, paramDeclarations.size()); + ParameterDeclaration pd = paramDeclarations.get(0); + assertEquals("foo", pd.getKey()); + assertEquals("|java.lang.String|", pd.getTypeInfo()); + assertTrue(pd.getOrigin().isSynthetic()); } @Test