sbglasius commented on code in PR #16048: URL: https://github.com/apache/grails-core/pull/16048#discussion_r4060335674
########## 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: You're right and my premise was wrong. I reproduced your result before replying: `security.userLookup { ... }` throws `MissingMethodException` on both Groovy 4.0.31 and 5.1.0 — plain, with a `security { }` block elsewhere in the same file, and for the nested `security { remember.me { x = 1 } }` form. Only the dotted *assignment* parses, and that path is already handled. So there is no working configuration whose settings vanish here, and the "statically indistinguishable from `items.each { }`" point stands on its own. Withdrawn — leave the 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) { + 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: Fix looks right. Worth noting for anyone reading later that the guard is `inference.value != null` rather than a Groovy truth check, so `false`, `0` and `''` defaults are still recorded — that was the trap here and it's avoided. Nulls inside a list or map value are untouched, as you say. One leftover, since the principle you state — *a null default is no default* — only got applied to the DSL-derived side. The curated overlays are passed through verbatim by `mergeNamed`, so at this PR's head 17 entries still ship an explicit `"defaultValue": null` into the generated canonical file: - `grails-data-hibernate5/dbmigration` — 4: `updateOnStartDefaultSchema`, `contexts`, `excludeObjects`, `includeObjects` - `grails-data-hibernate7/dbmigration` — the same 4 - `grails-data-mongodb/grails-plugin` — 9: `databaseName`, `username`, `password`, `options.readPreference`, `options.writeConcern`, `options.readConcern`, `options.retryWrites`, `options.retryReads`, `options.applicationName` Pre-existing content that this PR only renames, so not a blocker — but this PR is what makes those three modules publish a generated canonical file, so it seems like the moment to drop them. -- 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]
