matrei commented on code in PR #16048: URL: https://github.com/apache/grails-core/pull/16048#discussion_r4059952461
########## build-logic/plugins/src/main/groovy/org/apache/grails/buildsrc/GroovyDslConfigurationMetadataParser.groovy: ########## @@ -0,0 +1,339 @@ +/* + * 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 + * + * https://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.grails.buildsrc + +import org.codehaus.groovy.ast.ModuleNode +import org.codehaus.groovy.ast.expr.ArgumentListExpression +import org.codehaus.groovy.ast.expr.BinaryExpression +import org.codehaus.groovy.ast.expr.ClassExpression +import org.codehaus.groovy.ast.expr.ClosureExpression +import org.codehaus.groovy.ast.expr.ConstantExpression +import org.codehaus.groovy.ast.expr.DeclarationExpression +import org.codehaus.groovy.ast.expr.Expression +import org.codehaus.groovy.ast.expr.ListExpression +import org.codehaus.groovy.ast.expr.MapExpression +import org.codehaus.groovy.ast.expr.MethodCallExpression +import org.codehaus.groovy.ast.expr.PropertyExpression +import org.codehaus.groovy.ast.expr.TernaryExpression +import org.codehaus.groovy.ast.expr.VariableExpression +import org.codehaus.groovy.ast.stmt.BlockStatement +import org.codehaus.groovy.ast.stmt.ExpressionStatement +import org.codehaus.groovy.ast.stmt.IfStatement +import org.codehaus.groovy.ast.stmt.Statement +import org.codehaus.groovy.control.CompilationFailedException +import org.codehaus.groovy.control.SourceUnit +import org.codehaus.groovy.syntax.Types + +import java.nio.charset.StandardCharsets + +/** Extracts configuration metadata from Groovy DSL source without evaluating source code. */ +final class GroovyDslConfigurationMetadataParser { + + private GroovyDslConfigurationMetadataParser() { + } + + /** + * DSL sources are registered explicitly, so a source that is missing, or a root name that no source + * uses, means the registration went stale and would otherwise silently drop the metadata it stood for. + */ + static List<Map<String, Object>> parse(Collection<File> files, Map<String, String> rootPrefixes) { + List<Map<String, Object>> properties = [] + Set<String> matchedRoots = [] + files.sort { File file -> file.absolutePath }.each { File file -> + if (!file.isFile()) { + throw new IllegalArgumentException("Groovy DSL configuration source '${file.absolutePath}' does not exist") + } + parseTopLevel(parseSource(file).statementBlock, rootPrefixes, false, properties, matchedRoots) + } + List<String> unmatchedRoots = (rootPrefixes.keySet() - matchedRoots).sort() + if (unmatchedRoots) { + throw new IllegalArgumentException( + "No Groovy DSL configuration source declares the registered root(s) ${unmatchedRoots}") + } + mergeBranches(properties) + } + + /** + * Reconciles multiple entries for the same property name before they reach the stricter + * cross-source conflict check. Mutually exclusive branches (if/else) commonly assign a + * property differently per branch, e.g. a different literal type or null in one branch, or + * override an unconditional default only under some environments; neither is an authoring + * conflict. Two *unconditional* assignments to the same name disagreeing is still a real + * conflict, regardless of how many conditional entries for that name sit between them. + */ + private static List<Map<String, Object>> mergeBranches(List<Map<String, Object>> properties) { + properties.groupBy { Map<String, Object> property -> property.name as String } + .collect { String name, List<Map<String, Object>> entries -> mergeGroup(name, entries) } + .sort { Map<String, Object> property -> property.name as String } + } + + private static Map<String, Object> mergeGroup(String name, List<Map<String, Object>> entries) { + List<Map<String, Object>> unconditional = (entries.findAll { Map<String, Object> entry -> + !(entry.conditional as boolean) + }.collect { Map<String, Object> entry -> stripConditional(entry) } as Set).toList() + if (unconditional.size() > 1) { + throw new IllegalArgumentException("Conflicting DSL properties metadata for '${name}'") + } + Map<String, Object> merged = [name: name] + Set<String> types = (entries*.type.findAll { String type -> type != null } as Set) + // a null literal says nothing about the type, but a value that cannot be inferred may be of any type + if (types.size() == 1 && !entries.any { Map<String, Object> entry -> entry.dynamic as boolean }) { + merged.type = types.first() + } + if (unconditional && unconditional[0].containsKey('defaultValue')) { + merged.defaultValue = unconditional[0].defaultValue + } + merged + } + + private static Map<String, Object> stripConditional(Map<String, Object> entry) { + Map<String, Object> stripped = new LinkedHashMap<>(entry) + stripped.remove('conditional') + stripped.remove('dynamic') + stripped + } + + /** + * The top level of a ConfigSlurper script may open a root section, assign a dotted path that starts + * with a root, or wrap either of those in an if statement or an environments block. + */ + private static void parseTopLevel(Statement statement, Map<String, String> rootPrefixes, boolean conditional, + List<Map<String, Object>> properties, Set<String> matchedRoots) { + if (statement instanceof BlockStatement) { + statement.statements.each { Statement child -> + parseTopLevel(child, rootPrefixes, conditional, properties, matchedRoots) + } + } else if (statement instanceof IfStatement) { + parseTopLevel(statement.ifBlock, rootPrefixes, true, properties, matchedRoots) + parseTopLevel(statement.elseBlock, rootPrefixes, true, properties, matchedRoots) + } else if (statement instanceof ExpressionStatement) { + Expression expression = statement.expression + if (isAssignment(expression)) { + List<String> segments = leftHandPath((expression as BinaryExpression).leftExpression) + String prefix = segments == null || segments.size() < 2 ? null : rootPrefixes[segments[0]] + if (prefix != null) { + matchedRoots << segments[0] + addProperty("${prefix}.${segments.tail().join('.')}", + (expression as BinaryExpression).rightExpression, conditional, properties) + } + } else if (isSectionCall(expression)) { + MethodCallExpression call = expression as MethodCallExpression + ClosureExpression closure = closureArgument(call) + String root = call.methodAsString + if (root == 'environments') { + environmentClosures(closure.code).each { ClosureExpression environment -> + parseTopLevel(environment.code, rootPrefixes, true, properties, matchedRoots) + } + } else if (rootPrefixes[root] != null) { + matchedRoots << root + parseStatements(closure.code, rootPrefixes[root], conditional, properties, new LinkedHashSet<String>()) + } + } + } + } + + private static ModuleNode parseSource(File file) { + try { + SourceUnit source = SourceUnit.create(file.absolutePath, file.getText(StandardCharsets.UTF_8.name())) + source.parse() + source.completePhase() + source.nextPhase() + source.convert() + source.errorCollector.failIfErrors() + source.AST + } catch (CompilationFailedException exception) { + throw new IllegalArgumentException("Failed to parse Groovy DSL source '${file.absolutePath}'", exception) + } + } + + private static void parseStatements(Statement statement, String prefix, boolean conditional, + List<Map<String, Object>> properties, Set<String> locals) { + if (statement instanceof BlockStatement) { + Set<String> scopedLocals = new LinkedHashSet<>(locals) + statement.statements.each { Statement child -> + parseStatements(child, prefix, conditional, properties, scopedLocals) + } + } else if (statement instanceof IfStatement) { + parseStatements(statement.ifBlock, prefix, true, properties, locals) + parseStatements(statement.elseBlock, prefix, true, properties, locals) + } else if (statement instanceof ExpressionStatement) { + Expression expression = statement.expression + if (expression instanceof DeclarationExpression) { + locals.addAll(declaredNames(expression)) + } else if (isAssignment(expression)) { + List<String> segments = leftHandPath((expression as BinaryExpression).leftExpression) + if (segments != null && !(segments[0] in locals)) { + addProperty("${prefix}.${segments.join('.')}", + (expression as BinaryExpression).rightExpression, conditional, properties) + } + } else if (isSectionCall(expression)) { + MethodCallExpression call = expression as MethodCallExpression + ClosureExpression closure = closureArgument(call) + if (call.methodAsString == 'environments') { + environmentClosures(closure.code).each { ClosureExpression environment -> + parseStatements(environment.code, prefix, true, properties, locals) + } + } else { + parseStatements(closure.code, "${prefix}.${call.methodAsString}", conditional, properties, locals) + } + } + } + } + + /** A local variable declaration is also a binary assignment expression, but never a setting. */ + private static boolean isAssignment(Expression expression) { + expression instanceof BinaryExpression && !(expression instanceof DeclarationExpression) && + expression.operation.type == Types.ASSIGN + } + + /** + * Only a call on the script itself opens a configuration section: {@code items.each { }} or + * {@code value.with { }} take a closure as well, but their receiver is ordinary code. + */ + private static boolean isSectionCall(Expression expression) { Review Comment: I checked this one against ConfigSlurper before changing anything, and `security.userLookup { ... }` is not a section idiom — it does not parse at runtime. ConfigSlurper only intercepts closure calls on the script itself; `security` resolves to a `ConfigObject`, and the call on it fails: ``` new ConfigSlurper('production').parse("security.userLookup {\n userDomainClassName = 'User'\n}") => MissingMethodException: No signature of method: userLookup for class: groovy.util.ConfigObject ``` Same result with a `security { }` block elsewhere in the file, and for the nested form (`security { remember.me { x = 1 } }`), on both Groovy 4.0.31 and 5.1.2. Only dotted *assignments* (`security.userLookup.userDomainClassName = 'User'`) are valid, and those are handled. So there is no working configuration whose settings vanish here: a source written this way throws the first time the plugin loads it. The fail-loud checks in `parse()` are for the case the runtime cannot catch — a build registration drifting away from a source that still works. Inside a section the shape is also statically indistinguishable from `items.each { }`, which the parser has to keep ignoring. I left this path as it is. ########## build-logic/plugins/src/main/groovy/org/apache/grails/buildsrc/GroovyDslConfigurationMetadataParser.groovy: ########## @@ -0,0 +1,339 @@ +/* + * 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 + * + * https://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.grails.buildsrc + +import org.codehaus.groovy.ast.ModuleNode +import org.codehaus.groovy.ast.expr.ArgumentListExpression +import org.codehaus.groovy.ast.expr.BinaryExpression +import org.codehaus.groovy.ast.expr.ClassExpression +import org.codehaus.groovy.ast.expr.ClosureExpression +import org.codehaus.groovy.ast.expr.ConstantExpression +import org.codehaus.groovy.ast.expr.DeclarationExpression +import org.codehaus.groovy.ast.expr.Expression +import org.codehaus.groovy.ast.expr.ListExpression +import org.codehaus.groovy.ast.expr.MapExpression +import org.codehaus.groovy.ast.expr.MethodCallExpression +import org.codehaus.groovy.ast.expr.PropertyExpression +import org.codehaus.groovy.ast.expr.TernaryExpression +import org.codehaus.groovy.ast.expr.VariableExpression +import org.codehaus.groovy.ast.stmt.BlockStatement +import org.codehaus.groovy.ast.stmt.ExpressionStatement +import org.codehaus.groovy.ast.stmt.IfStatement +import org.codehaus.groovy.ast.stmt.Statement +import org.codehaus.groovy.control.CompilationFailedException +import org.codehaus.groovy.control.SourceUnit +import org.codehaus.groovy.syntax.Types + +import java.nio.charset.StandardCharsets + +/** Extracts configuration metadata from Groovy DSL source without evaluating source code. */ +final class GroovyDslConfigurationMetadataParser { + + private GroovyDslConfigurationMetadataParser() { + } + + /** + * DSL sources are registered explicitly, so a source that is missing, or a root name that no source + * uses, means the registration went stale and would otherwise silently drop the metadata it stood for. + */ + static List<Map<String, Object>> parse(Collection<File> files, Map<String, String> rootPrefixes) { + List<Map<String, Object>> properties = [] + Set<String> matchedRoots = [] + files.sort { File file -> file.absolutePath }.each { File file -> + if (!file.isFile()) { + throw new IllegalArgumentException("Groovy DSL configuration source '${file.absolutePath}' does not exist") + } + parseTopLevel(parseSource(file).statementBlock, rootPrefixes, false, properties, matchedRoots) + } + List<String> unmatchedRoots = (rootPrefixes.keySet() - matchedRoots).sort() + if (unmatchedRoots) { + throw new IllegalArgumentException( + "No Groovy DSL configuration source declares the registered root(s) ${unmatchedRoots}") + } + mergeBranches(properties) + } + + /** + * Reconciles multiple entries for the same property name before they reach the stricter + * cross-source conflict check. Mutually exclusive branches (if/else) commonly assign a + * property differently per branch, e.g. a different literal type or null in one branch, or + * override an unconditional default only under some environments; neither is an authoring + * conflict. Two *unconditional* assignments to the same name disagreeing is still a real + * conflict, regardless of how many conditional entries for that name sit between them. + */ + private static List<Map<String, Object>> mergeBranches(List<Map<String, Object>> properties) { + properties.groupBy { Map<String, Object> property -> property.name as String } + .collect { String name, List<Map<String, Object>> entries -> mergeGroup(name, entries) } + .sort { Map<String, Object> property -> property.name as String } + } + + private static Map<String, Object> mergeGroup(String name, List<Map<String, Object>> entries) { + List<Map<String, Object>> unconditional = (entries.findAll { Map<String, Object> entry -> + !(entry.conditional as boolean) + }.collect { Map<String, Object> entry -> stripConditional(entry) } as Set).toList() + if (unconditional.size() > 1) { + throw new IllegalArgumentException("Conflicting DSL properties metadata for '${name}'") + } + Map<String, Object> merged = [name: name] + Set<String> types = (entries*.type.findAll { String type -> type != null } as Set) + // a null literal says nothing about the type, but a value that cannot be inferred may be of any type + if (types.size() == 1 && !entries.any { Map<String, Object> entry -> entry.dynamic as boolean }) { + merged.type = types.first() + } + if (unconditional && unconditional[0].containsKey('defaultValue')) { + merged.defaultValue = unconditional[0].defaultValue + } + merged + } + + private static Map<String, Object> stripConditional(Map<String, Object> entry) { + Map<String, Object> stripped = new LinkedHashMap<>(entry) + stripped.remove('conditional') + stripped.remove('dynamic') + stripped + } + + /** + * The top level of a ConfigSlurper script may open a root section, assign a dotted path that starts + * with a root, or wrap either of those in an if statement or an environments block. + */ + private static void parseTopLevel(Statement statement, Map<String, String> rootPrefixes, boolean conditional, + List<Map<String, Object>> properties, Set<String> matchedRoots) { + if (statement instanceof BlockStatement) { + statement.statements.each { Statement child -> + parseTopLevel(child, rootPrefixes, conditional, properties, matchedRoots) + } + } else if (statement instanceof IfStatement) { + parseTopLevel(statement.ifBlock, rootPrefixes, true, properties, matchedRoots) + parseTopLevel(statement.elseBlock, rootPrefixes, true, properties, matchedRoots) + } else if (statement instanceof ExpressionStatement) { + Expression expression = statement.expression + if (isAssignment(expression)) { + List<String> segments = leftHandPath((expression as BinaryExpression).leftExpression) + String prefix = segments == null || segments.size() < 2 ? null : rootPrefixes[segments[0]] + if (prefix != null) { + matchedRoots << segments[0] + addProperty("${prefix}.${segments.tail().join('.')}", + (expression as BinaryExpression).rightExpression, conditional, properties) + } + } else if (isSectionCall(expression)) { + MethodCallExpression call = expression as MethodCallExpression + ClosureExpression closure = closureArgument(call) + String root = call.methodAsString + if (root == 'environments') { + environmentClosures(closure.code).each { ClosureExpression environment -> + parseTopLevel(environment.code, rootPrefixes, true, properties, matchedRoots) + } + } else if (rootPrefixes[root] != null) { + matchedRoots << root + parseStatements(closure.code, rootPrefixes[root], conditional, properties, new LinkedHashSet<String>()) + } + } + } + } + + private static ModuleNode parseSource(File file) { + try { + SourceUnit source = SourceUnit.create(file.absolutePath, file.getText(StandardCharsets.UTF_8.name())) + source.parse() + source.completePhase() + source.nextPhase() + source.convert() + source.errorCollector.failIfErrors() + source.AST + } catch (CompilationFailedException exception) { + throw new IllegalArgumentException("Failed to parse Groovy DSL source '${file.absolutePath}'", exception) + } + } + + private static void parseStatements(Statement statement, String prefix, boolean conditional, + List<Map<String, Object>> properties, Set<String> locals) { + if (statement instanceof BlockStatement) { + Set<String> scopedLocals = new LinkedHashSet<>(locals) + statement.statements.each { Statement child -> + parseStatements(child, prefix, conditional, properties, scopedLocals) + } + } else if (statement instanceof IfStatement) { + parseStatements(statement.ifBlock, prefix, true, properties, locals) + parseStatements(statement.elseBlock, prefix, true, properties, locals) + } else if (statement instanceof ExpressionStatement) { + Expression expression = statement.expression + if (expression instanceof DeclarationExpression) { + locals.addAll(declaredNames(expression)) + } else if (isAssignment(expression)) { + List<String> segments = leftHandPath((expression as BinaryExpression).leftExpression) + if (segments != null && !(segments[0] in locals)) { + addProperty("${prefix}.${segments.join('.')}", + (expression as BinaryExpression).rightExpression, conditional, properties) + } + } else if (isSectionCall(expression)) { + MethodCallExpression call = expression as MethodCallExpression + ClosureExpression closure = closureArgument(call) + if (call.methodAsString == 'environments') { + environmentClosures(closure.code).each { ClosureExpression environment -> + parseStatements(environment.code, prefix, true, properties, locals) + } + } else { + parseStatements(closure.code, "${prefix}.${call.methodAsString}", conditional, properties, locals) + } + } + } + } + + /** A local variable declaration is also a binary assignment expression, but never a setting. */ + private static boolean isAssignment(Expression expression) { + expression instanceof BinaryExpression && !(expression instanceof DeclarationExpression) && + expression.operation.type == Types.ASSIGN + } + + /** + * Only a call on the script itself opens a configuration section: {@code items.each { }} or + * {@code value.with { }} take a closure as well, but their receiver is ordinary code. + */ + private static boolean isSectionCall(Expression expression) { + expression instanceof MethodCallExpression && expression.implicitThis && + expression.methodAsString != null && closureArgument(expression) != null + } + + private static List<String> declaredNames(DeclarationExpression declaration) { + declaration.multipleAssignmentDeclaration ? + declaration.tupleExpression.expressions.findAll { Expression variable -> + variable instanceof VariableExpression + }.collect { Expression variable -> (variable as VariableExpression).name } : + [declaration.variableExpression.name] + } + + /** + * A ConfigSlurper {@code environments { production { ... } } } block picks exactly one named + * environment closure at runtime; unlike an ordinary nested section, its name is not part of + * the property path. Each environment closure is parsed under the unchanged prefix, marked + * conditional the same way an if/else branch is. + */ + private static List<ClosureExpression> environmentClosures(Statement statement) { Review Comment: Both confirmed, both fixed in 4d49894f05. Too narrow: ConfigSlurper does accept an `if` inside `environments` (your example yields `[security:[foo:1]]` under `production`), so `environmentClosures` now recurses into both branches of an `IfStatement`. Too wide: it now uses the same `isSectionCall` predicate as the other two paths, so `candidates.each { ... }` is no longer taken for an environment. That made the private `methodCall` helper unused, so it is gone. Covered by `"parses an environment declared inside an if statement and ignores ordinary calls in an environments block"` (an environment in each branch of an if/else, plus the `candidates.each` case). ########## build-logic/plugins/src/main/groovy/org/apache/grails/buildsrc/GroovyDslConfigurationMetadataParser.groovy: ########## @@ -0,0 +1,339 @@ +/* + * 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 + * + * https://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.grails.buildsrc + +import org.codehaus.groovy.ast.ModuleNode +import org.codehaus.groovy.ast.expr.ArgumentListExpression +import org.codehaus.groovy.ast.expr.BinaryExpression +import org.codehaus.groovy.ast.expr.ClassExpression +import org.codehaus.groovy.ast.expr.ClosureExpression +import org.codehaus.groovy.ast.expr.ConstantExpression +import org.codehaus.groovy.ast.expr.DeclarationExpression +import org.codehaus.groovy.ast.expr.Expression +import org.codehaus.groovy.ast.expr.ListExpression +import org.codehaus.groovy.ast.expr.MapExpression +import org.codehaus.groovy.ast.expr.MethodCallExpression +import org.codehaus.groovy.ast.expr.PropertyExpression +import org.codehaus.groovy.ast.expr.TernaryExpression +import org.codehaus.groovy.ast.expr.VariableExpression +import org.codehaus.groovy.ast.stmt.BlockStatement +import org.codehaus.groovy.ast.stmt.ExpressionStatement +import org.codehaus.groovy.ast.stmt.IfStatement +import org.codehaus.groovy.ast.stmt.Statement +import org.codehaus.groovy.control.CompilationFailedException +import org.codehaus.groovy.control.SourceUnit +import org.codehaus.groovy.syntax.Types + +import java.nio.charset.StandardCharsets + +/** Extracts configuration metadata from Groovy DSL source without evaluating source code. */ +final class GroovyDslConfigurationMetadataParser { + + private GroovyDslConfigurationMetadataParser() { + } + + /** + * DSL sources are registered explicitly, so a source that is missing, or a root name that no source + * uses, means the registration went stale and would otherwise silently drop the metadata it stood for. + */ + static List<Map<String, Object>> parse(Collection<File> files, Map<String, String> rootPrefixes) { + List<Map<String, Object>> properties = [] + Set<String> matchedRoots = [] + files.sort { File file -> file.absolutePath }.each { File file -> + if (!file.isFile()) { + throw new IllegalArgumentException("Groovy DSL configuration source '${file.absolutePath}' does not exist") + } + parseTopLevel(parseSource(file).statementBlock, rootPrefixes, false, properties, matchedRoots) + } + List<String> unmatchedRoots = (rootPrefixes.keySet() - matchedRoots).sort() + if (unmatchedRoots) { + throw new IllegalArgumentException( + "No Groovy DSL configuration source declares the registered root(s) ${unmatchedRoots}") + } + mergeBranches(properties) + } + + /** + * Reconciles multiple entries for the same property name before they reach the stricter + * cross-source conflict check. Mutually exclusive branches (if/else) commonly assign a + * property differently per branch, e.g. a different literal type or null in one branch, or + * override an unconditional default only under some environments; neither is an authoring + * conflict. Two *unconditional* assignments to the same name disagreeing is still a real + * conflict, regardless of how many conditional entries for that name sit between them. + */ + private static List<Map<String, Object>> mergeBranches(List<Map<String, Object>> properties) { + properties.groupBy { Map<String, Object> property -> property.name as String } + .collect { String name, List<Map<String, Object>> entries -> mergeGroup(name, entries) } + .sort { Map<String, Object> property -> property.name as String } + } + + private static Map<String, Object> mergeGroup(String name, List<Map<String, Object>> entries) { + List<Map<String, Object>> unconditional = (entries.findAll { Map<String, Object> entry -> + !(entry.conditional as boolean) + }.collect { Map<String, Object> entry -> stripConditional(entry) } as Set).toList() + if (unconditional.size() > 1) { + throw new IllegalArgumentException("Conflicting DSL properties metadata for '${name}'") + } + Map<String, Object> merged = [name: name] + Set<String> types = (entries*.type.findAll { String type -> type != null } as Set) + // a null literal says nothing about the type, but a value that cannot be inferred may be of any type + if (types.size() == 1 && !entries.any { Map<String, Object> entry -> entry.dynamic as boolean }) { + merged.type = types.first() + } + if (unconditional && unconditional[0].containsKey('defaultValue')) { + merged.defaultValue = unconditional[0].defaultValue + } + merged + } + + private static Map<String, Object> stripConditional(Map<String, Object> entry) { + Map<String, Object> stripped = new LinkedHashMap<>(entry) + stripped.remove('conditional') + stripped.remove('dynamic') + stripped + } + + /** + * The top level of a ConfigSlurper script may open a root section, assign a dotted path that starts + * with a root, or wrap either of those in an if statement or an environments block. + */ + private static void parseTopLevel(Statement statement, Map<String, String> rootPrefixes, boolean conditional, + List<Map<String, Object>> properties, Set<String> matchedRoots) { + if (statement instanceof BlockStatement) { + statement.statements.each { Statement child -> + parseTopLevel(child, rootPrefixes, conditional, properties, matchedRoots) + } + } else if (statement instanceof IfStatement) { + parseTopLevel(statement.ifBlock, rootPrefixes, true, properties, matchedRoots) + parseTopLevel(statement.elseBlock, rootPrefixes, true, properties, matchedRoots) + } else if (statement instanceof ExpressionStatement) { + Expression expression = statement.expression + if (isAssignment(expression)) { + List<String> segments = leftHandPath((expression as BinaryExpression).leftExpression) + String prefix = segments == null || segments.size() < 2 ? null : rootPrefixes[segments[0]] + if (prefix != null) { + matchedRoots << segments[0] + addProperty("${prefix}.${segments.tail().join('.')}", + (expression as BinaryExpression).rightExpression, conditional, properties) + } + } else if (isSectionCall(expression)) { + MethodCallExpression call = expression as MethodCallExpression + ClosureExpression closure = closureArgument(call) + String root = call.methodAsString + if (root == 'environments') { + environmentClosures(closure.code).each { ClosureExpression environment -> + parseTopLevel(environment.code, rootPrefixes, true, properties, matchedRoots) + } + } else if (rootPrefixes[root] != null) { + matchedRoots << root + parseStatements(closure.code, rootPrefixes[root], conditional, properties, new LinkedHashSet<String>()) + } + } + } + } + + private static ModuleNode parseSource(File file) { + try { + SourceUnit source = SourceUnit.create(file.absolutePath, file.getText(StandardCharsets.UTF_8.name())) + source.parse() + source.completePhase() + source.nextPhase() + source.convert() + source.errorCollector.failIfErrors() + source.AST + } catch (CompilationFailedException exception) { + throw new IllegalArgumentException("Failed to parse Groovy DSL source '${file.absolutePath}'", exception) + } + } + + private static void parseStatements(Statement statement, String prefix, boolean conditional, + List<Map<String, Object>> properties, Set<String> locals) { + if (statement instanceof BlockStatement) { + Set<String> scopedLocals = new LinkedHashSet<>(locals) + statement.statements.each { Statement child -> + parseStatements(child, prefix, conditional, properties, scopedLocals) + } + } else if (statement instanceof IfStatement) { + parseStatements(statement.ifBlock, prefix, true, properties, locals) + parseStatements(statement.elseBlock, prefix, true, properties, locals) + } else if (statement instanceof ExpressionStatement) { + Expression expression = statement.expression + if (expression instanceof DeclarationExpression) { + locals.addAll(declaredNames(expression)) + } else if (isAssignment(expression)) { + List<String> segments = leftHandPath((expression as BinaryExpression).leftExpression) + if (segments != null && !(segments[0] in locals)) { + addProperty("${prefix}.${segments.join('.')}", + (expression as BinaryExpression).rightExpression, conditional, properties) + } + } else if (isSectionCall(expression)) { + MethodCallExpression call = expression as MethodCallExpression + ClosureExpression closure = closureArgument(call) + if (call.methodAsString == 'environments') { + environmentClosures(closure.code).each { ClosureExpression environment -> + parseStatements(environment.code, prefix, true, properties, locals) + } + } else { + parseStatements(closure.code, "${prefix}.${call.methodAsString}", conditional, properties, locals) + } + } + } + } + + /** A local variable declaration is also a binary assignment expression, but never a setting. */ + private static boolean isAssignment(Expression expression) { + expression instanceof BinaryExpression && !(expression instanceof DeclarationExpression) && + expression.operation.type == Types.ASSIGN + } + + /** + * Only a call on the script itself opens a configuration section: {@code items.each { }} or + * {@code value.with { }} take a closure as well, but their receiver is ordinary code. + */ + private static boolean isSectionCall(Expression expression) { + expression instanceof MethodCallExpression && expression.implicitThis && + expression.methodAsString != null && closureArgument(expression) != null + } + + private static List<String> declaredNames(DeclarationExpression declaration) { + declaration.multipleAssignmentDeclaration ? + declaration.tupleExpression.expressions.findAll { Expression variable -> + variable instanceof VariableExpression + }.collect { Expression variable -> (variable as VariableExpression).name } : + [declaration.variableExpression.name] + } + + /** + * A ConfigSlurper {@code environments { production { ... } } } block picks exactly one named + * environment closure at runtime; unlike an ordinary nested section, its name is not part of + * the property path. Each environment closure is parsed under the unchanged prefix, marked + * conditional the same way an if/else branch is. + */ + private static List<ClosureExpression> environmentClosures(Statement statement) { + if (statement instanceof BlockStatement) { + return statement.statements.collectMany { Statement child -> environmentClosures(child) } + } + MethodCallExpression call = methodCall(statement) + ClosureExpression closure = call == null ? null : closureArgument(call) + closure == null ? [] : [closure] + } + + private static void addProperty(String name, Expression value, boolean conditional, + List<Map<String, Object>> properties) { + Inference inference = infer(value) + Map<String, Object> property = [name: name, conditional: conditional] + if (inference.type != null) { + property.type = inference.type + } else if (!inference.literal) { + property.dynamic = true + } + if (!conditional && inference.literal) { + property.defaultValue = inference.value Review Comment: Confirmed, and it was wider than six: `DefaultSecurityConfig.groovy` has 14 unconditional null assignments, and all 14 shipped an explicit `"defaultValue": null` (the six you listed plus `userLookup.userDomainClassName`, `userLookup.authorityJoinClassName`, `authority.className`, `authority.groupAuthorityNameField`, `requestMap.className`, `dao.reflectionSaltSourceProperty`, `switchUser.targetUrl` and `switchUser.switchFailureUrl`). Fixed in 4d49894f05: `addProperty` now records a default only for a non-null literal. A semantic diff of the regenerated Spring Security metadata shows exactly those 14 keys removed and nothing else changed (145 properties, 14 groups before and after); `"defaultValue": null` no longer occurs in the file. Nulls *inside* a list or map default (`[null, 'present']`) are still preserved, since there they are part of the value. `"preserves null literals and omits map defaults with non-string keys"` now also asserts that `nothing = null` produces an entry with only a `name`. ########## build-logic/plugins/src/main/groovy/org/apache/grails/buildsrc/GroovyDslConfigurationMetadataParser.groovy: ########## @@ -0,0 +1,339 @@ +/* + * 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 + * + * https://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.grails.buildsrc + +import org.codehaus.groovy.ast.ModuleNode +import org.codehaus.groovy.ast.expr.ArgumentListExpression +import org.codehaus.groovy.ast.expr.BinaryExpression +import org.codehaus.groovy.ast.expr.ClassExpression +import org.codehaus.groovy.ast.expr.ClosureExpression +import org.codehaus.groovy.ast.expr.ConstantExpression +import org.codehaus.groovy.ast.expr.DeclarationExpression +import org.codehaus.groovy.ast.expr.Expression +import org.codehaus.groovy.ast.expr.ListExpression +import org.codehaus.groovy.ast.expr.MapExpression +import org.codehaus.groovy.ast.expr.MethodCallExpression +import org.codehaus.groovy.ast.expr.PropertyExpression +import org.codehaus.groovy.ast.expr.TernaryExpression +import org.codehaus.groovy.ast.expr.VariableExpression +import org.codehaus.groovy.ast.stmt.BlockStatement +import org.codehaus.groovy.ast.stmt.ExpressionStatement +import org.codehaus.groovy.ast.stmt.IfStatement +import org.codehaus.groovy.ast.stmt.Statement +import org.codehaus.groovy.control.CompilationFailedException +import org.codehaus.groovy.control.SourceUnit +import org.codehaus.groovy.syntax.Types + +import java.nio.charset.StandardCharsets + +/** Extracts configuration metadata from Groovy DSL source without evaluating source code. */ +final class GroovyDslConfigurationMetadataParser { + + private GroovyDslConfigurationMetadataParser() { + } + + /** + * DSL sources are registered explicitly, so a source that is missing, or a root name that no source + * uses, means the registration went stale and would otherwise silently drop the metadata it stood for. + */ + static List<Map<String, Object>> parse(Collection<File> files, Map<String, String> rootPrefixes) { + List<Map<String, Object>> properties = [] + Set<String> matchedRoots = [] + files.sort { File file -> file.absolutePath }.each { File file -> + if (!file.isFile()) { + throw new IllegalArgumentException("Groovy DSL configuration source '${file.absolutePath}' does not exist") + } + parseTopLevel(parseSource(file).statementBlock, rootPrefixes, false, properties, matchedRoots) + } + List<String> unmatchedRoots = (rootPrefixes.keySet() - matchedRoots).sort() + if (unmatchedRoots) { + throw new IllegalArgumentException( + "No Groovy DSL configuration source declares the registered root(s) ${unmatchedRoots}") + } + mergeBranches(properties) + } + + /** + * Reconciles multiple entries for the same property name before they reach the stricter + * cross-source conflict check. Mutually exclusive branches (if/else) commonly assign a + * property differently per branch, e.g. a different literal type or null in one branch, or + * override an unconditional default only under some environments; neither is an authoring + * conflict. Two *unconditional* assignments to the same name disagreeing is still a real + * conflict, regardless of how many conditional entries for that name sit between them. + */ + private static List<Map<String, Object>> mergeBranches(List<Map<String, Object>> properties) { + properties.groupBy { Map<String, Object> property -> property.name as String } + .collect { String name, List<Map<String, Object>> entries -> mergeGroup(name, entries) } + .sort { Map<String, Object> property -> property.name as String } + } + + private static Map<String, Object> mergeGroup(String name, List<Map<String, Object>> entries) { + List<Map<String, Object>> unconditional = (entries.findAll { Map<String, Object> entry -> + !(entry.conditional as boolean) + }.collect { Map<String, Object> entry -> stripConditional(entry) } as Set).toList() + if (unconditional.size() > 1) { + throw new IllegalArgumentException("Conflicting DSL properties metadata for '${name}'") + } + Map<String, Object> merged = [name: name] + Set<String> types = (entries*.type.findAll { String type -> type != null } as Set) + // a null literal says nothing about the type, but a value that cannot be inferred may be of any type + if (types.size() == 1 && !entries.any { Map<String, Object> entry -> entry.dynamic as boolean }) { + merged.type = types.first() + } + if (unconditional && unconditional[0].containsKey('defaultValue')) { + merged.defaultValue = unconditional[0].defaultValue + } + merged + } + + private static Map<String, Object> stripConditional(Map<String, Object> entry) { + Map<String, Object> stripped = new LinkedHashMap<>(entry) + stripped.remove('conditional') + stripped.remove('dynamic') + stripped + } + + /** + * The top level of a ConfigSlurper script may open a root section, assign a dotted path that starts + * with a root, or wrap either of those in an if statement or an environments block. + */ + private static void parseTopLevel(Statement statement, Map<String, String> rootPrefixes, boolean conditional, Review Comment: Confirmed. ConfigSlurper yields an empty config for `def security = [:]; security.enabled = true`, while the parser published `grails.plugin.springsecurity.enabled` — and, worse, counted it as a declaration of the root, so it also defeated the stale-registration check. Fixed in 4d49894f05: `parseTopLevel` now threads a `locals` set the same way `parseStatements` does (block-scoped, fed by declarations), skips dotted assignments and section calls whose first segment is a local, and hands the script-level locals on to the sections it opens, since those are visible inside the closures. For symmetry, `parseStatements` also no longer treats a call on a local (`helper { ... }`) as a nested section. New coverage: `"ignores a top-level local variable that shares its name with a registered root"`, `"does not take a top-level local variable for the declaration of a registered root"` (fails with the unmatched-root message), and a `helper { viaLocal = true }` line in the existing local-variable spec. All five new/extended features fail against the previous parser and pass now. Ran fresh with `--no-build-cache`: `build-logic` `check` (11 specs, `ConfigurationMetadataPluginSpec` 34/34) and `:grails-spring-security:test --tests '*ConfigurationMetadataSpec'`. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
