jdaugherty commented on code in PR #16048: URL: https://github.com/apache/grails-core/pull/16048#discussion_r3674976224
########## build-logic/plugins/src/main/groovy/org/apache/grails/buildsrc/GroovyDslConfigurationMetadataParser.groovy: ########## @@ -0,0 +1,207 @@ +/* + * 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.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() { + } + + static List<Map<String, Object>> parse(Collection<File> files, Map<String, String> rootPrefixes) { + List<Map<String, Object>> properties = [] + files.findAll { File file -> file.isFile() }.sort { File file -> file.absolutePath }.each { File file -> + parseFile(file, rootPrefixes, properties) + } + properties.sort { Map<String, Object> property -> property.name as String } + } + + private static void parseFile(File file, Map<String, String> rootPrefixes, + List<Map<String, Object>> properties) { + ModuleNode module = parseSource(file) + module.statementBlock.statements.each { Statement statement -> + MethodCallExpression call = methodCall(statement) + String root = call?.methodAsString + ClosureExpression closure = call == null ? null : closureArgument(call) + String prefix = root == null ? null : rootPrefixes[root] + if (prefix != null && closure != null) { + parseStatements(closure.code, prefix, false, properties) + } + } + } + + 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) { + if (statement instanceof BlockStatement) { + statement.statements.each { Statement child -> parseStatements(child, prefix, conditional, properties) } + } else if (statement instanceof IfStatement) { + parseStatements(statement.ifBlock, prefix, true, properties) + parseStatements(statement.elseBlock, prefix, true, properties) + } else if (statement instanceof ExpressionStatement) { + Expression expression = statement.expression + if (expression instanceof BinaryExpression && expression.operation.type == Types.ASSIGN) { + addAssignment(expression, prefix, conditional, properties) + } else if (expression instanceof MethodCallExpression) { + ClosureExpression closure = closureArgument(expression) + String nestedName = expression.methodAsString + if (closure != null && nestedName != null) { + parseStatements(closure.code, "${prefix}.${nestedName}", conditional, properties) Review Comment: These sources are ConfigSlurper scripts at runtime (`SpringSecurityUtils` loads `DefaultSecurityConfig` with `new ConfigSlurper(Environment.current.name).parse(...)`), so an `environments` block is valid in them. This branch would treat it as an ordinary nested section and silently emit names like `<prefix>.environments.production.foo` instead of `<prefix>.foo`. Nothing hits this today since `DefaultSecurityConfig.groovy` uses `Environment.current` if-checks instead, but it may be worth handling an `environments` call like `IfStatement` (recurse into each environment closure with the unchanged prefix and `conditional = true`) — or failing loudly when one is encountered. ########## grails-spring-security/plugin/src/test/groovy/grails/plugin/springsecurity/ConfigurationMetadataSpec.groovy: ########## @@ -0,0 +1,62 @@ +/* + * 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 grails.plugin.springsecurity + +import groovy.json.JsonSlurper +import spock.lang.Specification + +class ConfigurationMetadataSpec extends Specification { + + void 'generates merged configuration metadata from DefaultSecurityConfig'() { + given: + Map metadata = configurationMetadata() + Map<String, Map> properties = metadata.get('properties').collectEntries { Map property -> [(property.get('name')): property] } + + expect: 'the complete curated metadata contract is retained' + metadata.get('groups').size() == 14 + properties.size() == 145 Review Comment: These exact counts mean every future addition to `DefaultSecurityConfig.groovy` or the curated overlay fails this spec with only a number mismatch. If a deliberate tripwire is the intent that's fine, but consider deriving the expectation from the packaged `META-INF/additional-spring-configuration-metadata.json` in the same JAR (assert every curated group/property name appears in the canonical file) — the spec then stays self-maintaining and a failure names the missing entry instead of reporting `146 != 145`. ########## build-logic/plugins/src/main/groovy/org/apache/grails/buildsrc/GroovyDslConfigurationMetadataParser.groovy: ########## @@ -0,0 +1,207 @@ +/* + * 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.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() { + } + + static List<Map<String, Object>> parse(Collection<File> files, Map<String, String> rootPrefixes) { + List<Map<String, Object>> properties = [] + files.findAll { File file -> file.isFile() }.sort { File file -> file.absolutePath }.each { File file -> + parseFile(file, rootPrefixes, properties) + } + properties.sort { Map<String, Object> property -> property.name as String } + } + + private static void parseFile(File file, Map<String, String> rootPrefixes, + List<Map<String, Object>> properties) { + ModuleNode module = parseSource(file) + module.statementBlock.statements.each { Statement statement -> + MethodCallExpression call = methodCall(statement) + String root = call?.methodAsString + ClosureExpression closure = call == null ? null : closureArgument(call) + String prefix = root == null ? null : rootPrefixes[root] + if (prefix != null && closure != null) { + parseStatements(closure.code, prefix, false, properties) + } + } + } + + 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) { + if (statement instanceof BlockStatement) { + statement.statements.each { Statement child -> parseStatements(child, prefix, conditional, properties) } + } else if (statement instanceof IfStatement) { + parseStatements(statement.ifBlock, prefix, true, properties) + parseStatements(statement.elseBlock, prefix, true, properties) + } else if (statement instanceof ExpressionStatement) { + Expression expression = statement.expression + if (expression instanceof BinaryExpression && expression.operation.type == Types.ASSIGN) { + addAssignment(expression, prefix, conditional, properties) + } else if (expression instanceof MethodCallExpression) { + ClosureExpression closure = closureArgument(expression) + String nestedName = expression.methodAsString + if (closure != null && nestedName != null) { + parseStatements(closure.code, "${prefix}.${nestedName}", conditional, properties) + } + } + } + } + + private static void addAssignment(BinaryExpression assignment, String prefix, boolean conditional, + List<Map<String, Object>> properties) { + List<String> segments = leftHandPath(assignment.leftExpression) + if (segments == null) { + return + } + String name = "${prefix}.${segments.join('.')}" + Inference inference = infer(assignment.rightExpression) + Map<String, Object> property = [name: name] + if (inference.type != null) { + property.type = inference.type + } + if (!conditional && inference.literal) { + property.defaultValue = inference.value + } + properties << property Review Comment: Duplicate assignments to the same property produce distinct entries here, and the DSL index in `mergeNamed` (`indexByName`) fails the task when two entries for one name aren't map-equal. Two patterns that are valid at runtime under ConfigSlurper's last-write-wins semantics trip this: ```groovy foo = 'default' if (Environment.current == Environment.TEST) { foo = 'test' // conditional entry has no defaultValue -> conflicts with the unconditional entry } ``` and if/else branches whose literals infer different types (e.g. `= null` in one branch, `= true` in the other). `DefaultSecurityConfig.groovy` avoids both today — the `password` branches assign the same types on both sides — but the next DSL source registered could fail the build on a perfectly legitimate config. Consider collapsing same-name entries before returning (union the type the way `sharedType` does, keep the unconditional `defaultValue`), or at least add a spec pinning the conflict failure so it's an explicit contract. -- 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]
