Skip to content

Commit

Permalink
Merge pull request quarkusio#36921 from mkouba/qute-param-declaration…
Browse files Browse the repository at this point in the history
…s-fix

Qute: consider synthetic parameter declarations during validation
  • Loading branch information
gastaldi authored Nov 8, 2023
2 parents 80fbefb + 66391b4 commit e863473
Show file tree
Hide file tree
Showing 10 changed files with 204 additions and 139 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -657,49 +655,9 @@ public Optional<Variant> getVariant() {
}
});

// It's a file-based template
// We need to find out whether the parsed template represents a checked template
Map<String, String> 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<String, Map<String, MethodParameterDeclaration>> checkedTemplateIdToParamDecl = new HashMap<>();
for (CheckedTemplateBuildItem checkedTemplate : checkedTemplates) {
if (checkedTemplate.isFragment()) {
continue;
}
for (Entry<String, String> 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<String, Map<String, MethodParameterDeclaration>> msgBundleTemplateIdToParamDecl = new HashMap<>();
for (MessageBundleMethodBuildItem messageBundleMethod : messageBundleMethods) {
MethodInfo method = messageBundleMethod.getMethod();
for (ListIterator<Type> 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<String, MessageBundleMethodBuildItem> messageBundleMethodsMap = messageBundleMethods.stream()
.filter(MessageBundleMethodBuildItem::isValidatable)
.collect(Collectors.toMap(MessageBundleMethodBuildItem::getTemplateId, Function.identity()));

builder.addParserHook(new ParserHook() {

Expand All @@ -715,16 +673,34 @@ 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<String, String> entry : checkedTemplate.bindings.entrySet()) {
parserHelper.addParameter(entry.getKey(), entry.getValue());
}
break;
}
}

if (templateId.startsWith(TemplatePathBuildItem.TAGS)) {
parserHelper.addParameter(UserTagSectionHelper.Factory.ARGS,
UserTagSectionHelper.Arguments.class.getName());
}
}

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<Type> it = method.parameterTypes().listIterator(); it.hasNext();) {
Type paramType = it.next();
String name = MessageBundleProcessor.getParameterName(method, it.previousIndex());
parserHelper.addParameter(name, getCheckedTemplateParameterTypeName(paramType));
}
}
}

}).build();
Expand All @@ -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<ParameterDeclaration> 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) {
Expand All @@ -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<ParameterDeclaration> 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()));
Expand All @@ -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<CheckedFragmentValidationBuildItem> validations,
List<TemplateExpressionMatchesBuildItem> expressionMatches,
Expand Down Expand Up @@ -925,29 +896,6 @@ private static String getCheckedTemplateParameterParameterizedTypeName(Parameter
return builder.toString();
}

private List<ParameterDeclaration> mergeParamDeclarations(List<ParameterDeclaration> parameterDeclarations,
Map<String, MethodParameterDeclaration> paramNameToDeclaration) {
if (paramNameToDeclaration != null) {
Map<String, ParameterDeclaration> 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<String, Map<String, MethodParameterDeclaration>> 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,
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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;
}
}

}
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -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<ParameterDeclaration> getSortedParameterDeclarations() {
List<ParameterDeclaration> ret = new ArrayList<>(parameterDeclarations);
ret.sort(new Comparator<ParameterDeclaration>() {
@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;
Expand Down
Original file line number Diff line number Diff line change
@@ -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<ParameterDeclaration> 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<Variant> 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;
}
};
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
import io.quarkus.qute.TemplateNode.Origin;

/**
* Represents a parameter declaration, i.e. <code>{@org.acme.Foo foo}</code>.
* Represents a type parameter declaration, i.e. <code>{@org.acme.Foo foo}</code>.
*/
public interface ParameterDeclaration {

Expand Down
Loading

0 comments on commit e863473

Please sign in to comment.