Copilot commented on code in PR #15397:
URL: https://github.com/apache/grails-core/pull/15397#discussion_r2816941209
##########
grails-console/src/main/groovy/grails/ui/command/GrailsApplicationContextCommandRunner.groovy:
##########
@@ -85,6 +97,32 @@ class GrailsApplicationContextCommandRunner extends
DevelopmentGrailsApplication
return null
}
+ /**
+ * Filters out command-specific options (arguments starting with '--')
from the
+ * args array before they are passed to Spring Boot's {@code
SpringApplication.run()}.
+ *
+ * <p>Spring Boot interprets {@code --key=value} arguments as property
overrides via
+ * {@code CommandLinePropertySource}. When Grails command options like
+ * {@code --dataSource=analytics} are passed through, Spring Boot sets
+ * {@code dataSource=analytics} as a top-level property, corrupting GORM's
datasource
+ * configuration which expects {@code dataSource} to be a Map containing
url, username, etc.</p>
+ *
+ * <p>Command options are still available to the Grails command via
+ * {@code CommandLineParser.parse(args)} which receives the unfiltered
args.</p>
+ *
+ * @param args the full argument array including command options
+ * @return a filtered array with command options removed, safe for Spring
Boot
+ */
+ static String[] filterCommandOptions(String[] args) {
+ List<String> filtered = new ArrayList<>()
+ for (String arg : args) {
+ if (!arg.startsWith('--')) {
+ filtered.add(arg)
+ }
+ }
+ filtered.toArray(new String[0])
+ }
Review Comment:
The filter doesn't handle the edge case of arguments that are exactly "--"
(with no key or value). While rare, if such an argument exists, it would be
filtered out. Consider whether this is the intended behavior, as "--" is
commonly used as a delimiter in command-line parsing to separate options from
positional arguments. If this is intentional, it should be documented.
##########
grails-console/src/main/groovy/grails/ui/command/GrailsApplicationContextCommandRunner.groovy:
##########
@@ -85,6 +97,32 @@ class GrailsApplicationContextCommandRunner extends
DevelopmentGrailsApplication
return null
}
+ /**
+ * Filters out command-specific options (arguments starting with '--')
from the
+ * args array before they are passed to Spring Boot's {@code
SpringApplication.run()}.
+ *
+ * <p>Spring Boot interprets {@code --key=value} arguments as property
overrides via
+ * {@code CommandLinePropertySource}. When Grails command options like
+ * {@code --dataSource=analytics} are passed through, Spring Boot sets
+ * {@code dataSource=analytics} as a top-level property, corrupting GORM's
datasource
+ * configuration which expects {@code dataSource} to be a Map containing
url, username, etc.</p>
+ *
+ * <p>Command options are still available to the Grails command via
+ * {@code CommandLineParser.parse(args)} which receives the unfiltered
args.</p>
+ *
+ * @param args the full argument array including command options
+ * @return a filtered array with command options removed, safe for Spring
Boot
+ */
+ static String[] filterCommandOptions(String[] args) {
+ List<String> filtered = new ArrayList<>()
+ for (String arg : args) {
+ if (!arg.startsWith('--')) {
+ filtered.add(arg)
+ }
+ }
+ filtered.toArray(new String[0])
+ }
Review Comment:
The filterCommandOptions method doesn't handle null elements within the args
array. If any element in the array is null, calling startsWith('--') will throw
a NullPointerException. Consider adding a null check before accessing the
argument.
##########
grails-console/src/test/groovy/grails/ui/command/GrailsApplicationContextCommandRunnerSpec.groovy:
##########
@@ -0,0 +1,124 @@
+/*
+ * 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.ui.command
+
+import spock.lang.Specification
+import spock.lang.Unroll
+
+/**
+ * Tests for {@link
GrailsApplicationContextCommandRunner#filterCommandOptions(String[])}.
+ *
+ * Verifies that command-specific options (--key=value, --flag) are filtered
out
+ * before being passed to Spring Boot's SpringApplication.run(), while
preserving
+ * non-option arguments like the command name.
+ */
+class GrailsApplicationContextCommandRunnerSpec extends Specification {
+
+ @Unroll
+ def "filterCommandOptions filters '#description'"() {
+ expect:
+ GrailsApplicationContextCommandRunner.filterCommandOptions(input as
String[]) == expected as String[]
+
+ where:
+ description | input
| expected
+ 'single --dataSource option' | ['dbm-status',
'--dataSource=analytics'] | ['dbm-status']
+ 'multiple command options' | ['dbm-status',
'--dataSource=analytics', '--contexts=dev'] | ['dbm-status']
+ 'no options (passthrough)' | ['dbm-status']
| ['dbm-status']
+ 'empty args' | []
| []
+ 'only options (no command)' | ['--dataSource=analytics']
| []
+ 'mixed options and positional args' | ['dbm-gorm-diff',
'--dataSource=analytics', 'output.groovy'] | ['dbm-gorm-diff', 'output.groovy']
+ 'flag without value' | ['dbm-update', '--verbose']
| ['dbm-update']
+ 'single dash args preserved' | ['dbm-status', '-v',
'--dataSource=analytics'] | ['dbm-status', '-v']
+ // Database Migration Plugin: --contexts and --defaultSchema
+ 'database migration --contexts option' | ['dbm-update',
'--contexts=production'] | ['dbm-update']
+ 'database migration --defaultSchema' | ['dbm-update',
'--defaultSchema=myschema'] | ['dbm-update']
+ 'all three dbm options together' | ['dbm-gorm-diff',
'--dataSource=analytics', '--contexts=dev', '--defaultSchema=public'] |
['dbm-gorm-diff']
+ 'dbm command with positional + options' | ['dbm-gorm-diff',
'output.groovy', '--dataSource=analytics', '--add'] | ['dbm-gorm-diff',
'output.groovy']
+ // Spring Security Plugin: --groupClassName
+ 'spring security --groupClassName' | ['s2-quickstart',
'com.example', 'User', 'Role', '--groupClassName=RoleGroup'] |
['s2-quickstart', 'com.example', 'User', 'Role']
+ // Script runner: --option in script args
+ 'script runner with --option' | ['myscript.groovy',
'--someOption=value'] | ['myscript.groovy']
+ }
+
+ def "filterCommandOptions preserves argument order"() {
+ given:
+ String[] input = ['first', 'second', '--removed', 'third'] as String[]
+
+ when:
+ String[] result =
GrailsApplicationContextCommandRunner.filterCommandOptions(input)
+
+ then:
+ result == ['first', 'second', 'third'] as String[]
+ }
+
+ def "filterCommandOptions returns new array instance"() {
+ given:
+ String[] input = ['dbm-status'] as String[]
+
+ when:
+ String[] result =
GrailsApplicationContextCommandRunner.filterCommandOptions(input)
+
+ then:
+ !result.is(input)
+ result == input
+ }
+
+ def "filterCommandOptions handles typical Gradle dbm task args"() {
+ given: 'args as they arrive from ApplicationContextCommandTask via
GrailsGradlePlugin'
+ // configureApplicationCommands() passes: [commandName, ...userArgs,
applicationClassName]
+ // main() extracts: commandName = args[0], appClass = args.last()
+ // run() receives: args.init() = [commandName, ...userArgs]
+ String[] runArgs = ['dbm-status', '--dataSource=analytics'] as String[]
+
+ when:
+ String[] springBootArgs =
GrailsApplicationContextCommandRunner.filterCommandOptions(runArgs)
+
+ then: 'only the command name reaches Spring Boot'
+ springBootArgs == ['dbm-status'] as String[]
+ springBootArgs.length == 1
+ }
+
+ def "filterCommandOptions prevents dataSource property corruption"() {
+ given: 'the problematic args that caused the original bug'
+ String[] args = ['dbm-gorm-diff', '--dataSource=analytics',
'--contexts=production'] as String[]
+
+ when:
+ String[] filtered =
GrailsApplicationContextCommandRunner.filterCommandOptions(args)
+
+ then: 'no --key=value args remain to corrupt Spring Boot properties'
+ filtered.every { !it.startsWith('--') }
+ filtered == ['dbm-gorm-diff'] as String[]
+ }
+
+ def "filterCommandOptions protects all known Grails ecosystem command
options"() {
+ expect: 'every known --option from Grails plugins is filtered'
+ GrailsApplicationContextCommandRunner.filterCommandOptions(input as
String[]) == expected as String[]
+
+ where:
+ input |
expected
+ // Database Migration Plugin (30+ commands accept these)
+ ['dbm-status', '--dataSource=analytics'] |
['dbm-status']
+ ['dbm-update', '--contexts=production,staging'] |
['dbm-update']
+ ['dbm-gorm-diff', '--defaultSchema=public'] |
['dbm-gorm-diff']
+ ['dbm-status', '--verbose'] |
['dbm-status']
+ ['dbm-diff', '--add'] |
['dbm-diff']
+ // Spring Security Plugin (s2-quickstart)
+ ['s2-quickstart', 'com.example', 'User', 'Role',
'--groupClassName=RoleGroup'] | ['s2-quickstart', 'com.example', 'User', 'Role']
+ // Custom ApplicationCommand with arbitrary --options
+ ['my-command', '--customFlag', '--anotherOption=value'] |
['my-command']
+ }
Review Comment:
Test coverage is missing for the edge case where args array contains null
elements. This should be tested to ensure the implementation handles nulls
gracefully (either by skipping them or throwing an appropriate error).
```suggestion
}
def "filterCommandOptions handles null elements in args"() {
given:
String[] input = ['dbm-status', null, '--dataSource=analytics'] as
String[]
when:
GrailsApplicationContextCommandRunner.filterCommandOptions(input)
then:
noExceptionThrown()
}
```
--
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]
