From 8af3ff5a0225187a45d05812781453405801341e Mon Sep 17 00:00:00 2001 From: Arturo Bernal Date: Mon, 24 Feb 2025 19:30:37 +0100 Subject: [PATCH] Fix #575: Scope IParametersValidator to its object's parameters Changed the storage and application of IParametersValidator instances from a global Set to a Map keyed by object, ensuring validators only see their own object's parameters. This resolves the bug where validators on a @ParametersDelegate saw top-level parameters (e.g., --argtop), making mutex group implementation more intuitive. Updated addDescription and validateOptions accordingly. --- .../java/com/beust/jcommander/JCommander.java | 43 +++++++---- .../Issue575ValidatorScopeTest.java | 77 +++++++++++++++++++ 2 files changed, 104 insertions(+), 16 deletions(-) create mode 100644 src/test/java/com/beust/jcommander/Issue575ValidatorScopeTest.java diff --git a/src/main/java/com/beust/jcommander/JCommander.java b/src/main/java/com/beust/jcommander/JCommander.java index 827a3d8e..be45eac0 100644 --- a/src/main/java/com/beust/jcommander/JCommander.java +++ b/src/main/java/com/beust/jcommander/JCommander.java @@ -129,9 +129,12 @@ public void addValue(Object convertedValue) { private Map requiredFields = Maps.newHashMap(); /** - * A set of all the parameters validators to be applied. + * A map of objects to their associated parameter validators. Each key is an object containing + * fields annotated with {@link Parameter}, and the value is a set of {@link IParametersValidator} + * instances that validate the parameters specific to that object. This ensures validators + * are scoped to their respective object's parameters rather than all parameters globally. */ - private Set parametersValidators = Sets.newHashSet(); + private final Map> objectValidators = Maps.newHashMap(); /** * A map of all the parameterized fields/methods. @@ -425,14 +428,19 @@ private void validateOptions() { } } - Map nameValuePairs = Maps.newHashMap(); - fields.values().forEach(pd -> - nameValuePairs.put(pd.getLongestName(), pd.getValue()) - ); + // Group parameters by their owning object + final Map> objectParameters = Maps.newHashMap(); + fields.values().forEach(pd -> { + Object owner = pd.getObject(); + Map params = objectParameters.computeIfAbsent(owner, k -> Maps.newHashMap()); + params.put(pd.getLongestName(), pd.getValue()); + }); - parametersValidators.forEach(parametersValidator -> - parametersValidator.validate(nameValuePairs) - ); + // Apply validators for each object + objectValidators.forEach((object, validators) -> { + Map params = objectParameters.getOrDefault(object, Maps.newHashMap()); + validators.forEach(validator -> validator.validate(params)); + }); } private static String pluralize(int quantity, String singular, String plural) { @@ -619,13 +627,16 @@ private void addDescription(Object object) { Parameters parameters = cls.getAnnotation(Parameters.class); if (parameters != null) { - Class[] parametersValidatorClasses = parameters.parametersValidators(); - for (Class parametersValidatorClass : parametersValidatorClasses) { - try { - IParametersValidator parametersValidator = parametersValidatorClass.getDeclaredConstructor().newInstance(); - parametersValidators.add(parametersValidator); - } catch (ReflectiveOperationException e) { - throw new ParameterException("Cannot instantiate rule: " + parametersValidatorClass, e); + final Class[] parametersValidatorClasses = parameters.parametersValidators(); + if (parametersValidatorClasses.length > 0) { + Set validators = objectValidators.computeIfAbsent(object, k -> Sets.newHashSet()); + for (final Class parametersValidatorClass : parametersValidatorClasses) { + try { + final IParametersValidator parametersValidator = parametersValidatorClass.getDeclaredConstructor().newInstance(); + validators.add(parametersValidator); + } catch (ReflectiveOperationException e) { + throw new ParameterException("Cannot instantiate rule: " + parametersValidatorClass, e); + } } } } diff --git a/src/test/java/com/beust/jcommander/Issue575ValidatorScopeTest.java b/src/test/java/com/beust/jcommander/Issue575ValidatorScopeTest.java new file mode 100644 index 00000000..44b4cc06 --- /dev/null +++ b/src/test/java/com/beust/jcommander/Issue575ValidatorScopeTest.java @@ -0,0 +1,77 @@ +package com.beust.jcommander; + +import org.testng.Assert; +import org.testng.annotations.Test; + +import java.util.Map; + +public class Issue575ValidatorScopeTest { + + @Parameters(parametersValidators = MyValidator.class) + static class SubGroup { + @Parameter(names = "--arg1") + private boolean arg1; + + @Parameter(names = "--arg2") + private String arg2; + + public boolean isArg1() { + return arg1; + } + + public String getArg2() { + return arg2; + } + } + + static class ArgsTop { + @Parameter(names = "--argtop") + private boolean argtop; + + @ParametersDelegate + private SubGroup subGroup = new SubGroup(); + + public boolean isArgtop() { + return argtop; + } + } + + static class MyValidator implements IParametersValidator { + @Override + public void validate(Map params) throws ParameterException { + if (params.containsKey("--argtop")) { + throw new ParameterException("Validator should not see --argtop, but found it in: " + params.keySet()); + } + if (!params.containsKey("--arg1") || !params.containsKey("--arg2")) { + throw new ParameterException("Validator should see --arg1 and --arg2, but found: " + params.keySet()); + } + } + } + + // Pre-fix: Expect exception due to bug (passes with original JCommander) + @Test(expectedExceptions = ParameterException.class, expectedExceptionsMessageRegExp = ".*Validator should not see --argtop.*") + public void testValidatorScopeBug() { + ArgsTop args = new ArgsTop(); + JCommander jCommander = JCommander.newBuilder() + .addObject(args) + .build(); + jCommander.parse("--argtop", "--arg1", "--arg2", "value"); + // These won’t run pre-fix due to exception + Assert.assertTrue(args.isArgtop(), "--argtop should be set"); + Assert.assertTrue(args.subGroup.isArg1(), "--arg1 should be set"); + Assert.assertEquals(args.subGroup.getArg2(), "value", "--arg2 should be set"); + } + + // Post-fix: Expect no exception, verify parameters (passes with fixed JCommander) + @Test + public void testValidatorScopeFixed() { + ArgsTop args = new ArgsTop(); + JCommander jCommander = JCommander.newBuilder() + .addObject(args) + .build(); + jCommander.parse("--argtop", "--arg1", "--arg2", "value"); + Assert.assertTrue(args.isArgtop(), "--argtop should be set"); + Assert.assertTrue(args.subGroup.isArg1(), "--arg1 should be set"); + Assert.assertEquals(args.subGroup.getArg2(), "value", "--arg2 should be set"); + } +} \ No newline at end of file