Github user tillrohrmann commented on a diff in the pull request: https://github.com/apache/flink/pull/4921#discussion_r148398599 --- Diff: flink-java/src/main/java/org/apache/flink/api/java/utils/RequiredParameters.java --- @@ -83,82 +82,100 @@ public void add(Option option) throws RequiredParametersException { * <p>If any check fails, a RequiredParametersException is thrown * * @param parameterTool - parameters supplied by the user. + * @return New ParameterTool instance with default values set * @throws RequiredParametersException if any of the specified checks fail */ - public void applyTo(ParameterTool parameterTool) throws RequiredParametersException { + public ParameterTool applyTo(ParameterTool parameterTool) throws RequiredParametersException { List<String> missingArguments = new LinkedList<>(); + + ParameterTool resultParameterTool = parameterTool; for (Option o : data.values()) { - if (parameterTool.data.containsKey(o.getName())) { - if (Objects.equals(parameterTool.data.get(o.getName()), ParameterTool.NO_VALUE_KEY)) { + if (resultParameterTool.has(o.getName())) { + if (Objects.equals(resultParameterTool.get(o.getName()), ParameterTool.NO_VALUE_KEY)) { // the parameter has been passed, but no value, check if there is a default value - checkAndApplyDefaultValue(o, parameterTool.data); + resultParameterTool = checkAndApplyDefaultValue(o, resultParameterTool); } else { // a value has been passed in the parameterTool, now check if it adheres to all constraints - checkAmbiguousValues(o, parameterTool.data); - checkIsCastableToDefinedType(o, parameterTool.data); - checkChoices(o, parameterTool.data); + checkAmbiguousValues(o, resultParameterTool); + checkIsCastableToDefinedType(o, resultParameterTool); + checkChoices(o, resultParameterTool); } } else { // check if there is a default name or a value passed for a possibly defined alternative name. - if (hasNoDefaultValueAndNoValuePassedOnAlternativeName(o, parameterTool.data)) { + resultParameterTool = synchronizeAlternativeName(o, resultParameterTool); + + if (!resultParameterTool.has(o.getName()) || Objects.equals(resultParameterTool.get(o.getName()), ParameterTool.NO_VALUE_KEY)) { missingArguments.add(o.getName()); } } } if (!missingArguments.isEmpty()) { throw new RequiredParametersException(this.missingArgumentsText(missingArguments), missingArguments); } + + return resultParameterTool; } // check if the given parameter has a default value and add it to the passed map if that is the case // else throw an exception - private void checkAndApplyDefaultValue(Option o, Map<String, String> data) throws RequiredParametersException { - if (hasNoDefaultValueAndNoValuePassedOnAlternativeName(o, data)) { + private ParameterTool checkAndApplyDefaultValue(Option o, ParameterTool parameterTool) throws RequiredParametersException { + final ParameterTool resultParameterTool = synchronizeAlternativeName(o, parameterTool); + + if (!resultParameterTool.has(o.getName()) || Objects.equals(resultParameterTool.get(o.getName()), ParameterTool.NO_VALUE_KEY)) { throw new RequiredParametersException("No default value for undefined parameter " + o.getName()); + } else { + return resultParameterTool; } } // check if the value in the given map which corresponds to the name of the given option // is castable to the type of the option (if any is defined) - private void checkIsCastableToDefinedType(Option o, Map<String, String> data) throws RequiredParametersException { - if (o.hasType() && !o.isCastableToDefinedType(data.get(o.getName()))) { + private void checkIsCastableToDefinedType(Option o, ParameterTool parameterTool) throws RequiredParametersException { + if (o.hasType() && !o.isCastableToDefinedType(parameterTool.get(o.getName()))) { throw new RequiredParametersException("Value for parameter " + o.getName() + " cannot be cast to type " + o.getType()); } } // check if the value in the given map which corresponds to the name of the given option // adheres to the list of given choices for the param in the options (if any are defined) - private void checkChoices(Option o, Map<String, String> data) throws RequiredParametersException { - if (o.getChoices().size() > 0 && !o.getChoices().contains(data.get(o.getName()))) { - throw new RequiredParametersException("Value " + data.get(o.getName()) + + private void checkChoices(Option o, ParameterTool parameterTool) throws RequiredParametersException { + if (o.getChoices().size() > 0 && !o.getChoices().contains(parameterTool.get(o.getName()))) { + throw new RequiredParametersException("Value " + parameterTool.get(o.getName()) + " is not in the list of valid choices for key " + o.getName()); } } // move value passed on alternative name to standard name or apply default value if any defined - // else return true to indicate parameter is 'really' missing - private boolean hasNoDefaultValueAndNoValuePassedOnAlternativeName(Option o, Map<String, String> data) - throws RequiredParametersException { - if (o.hasAlt() && data.containsKey(o.getAlt())) { - data.put(o.getName(), data.get(o.getAlt())); + // if any change was applied, then this method returns a new ParameterTool instance with these + // changes. If not, then the passed ParameterTool instance will be returned. + private ParameterTool synchronizeAlternativeName(Option o, ParameterTool parameterTool) { + // TODO: Throw this all away!!! + if (o.hasAlt() && parameterTool.has(o.getAlt())) { + HashMap<String, String> newData = new HashMap<>(parameterTool.toMap()); + newData.put(o.getName(), parameterTool.get(o.getAlt())); + + return ParameterTool.fromMap(newData); } else { if (o.hasDefaultValue()) { - data.put(o.getName(), o.getDefaultValue()); + HashMap<String, String> newData = new HashMap<>(parameterTool.toMap()); --- End diff -- Yes we could. But I didn't want to touch more than necessary. I think that this actually dead code and should be removed.
---