Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce a failing test for char[] type conversion #648

Merged
merged 1 commit into from
Oct 16, 2022

Conversation

triceo
Copy link
Contributor

@triceo triceo commented Mar 23, 2019

When I have a char[]-typed @Option in my command-line object, I get an error when trying to set it from the command line:

Invalid value for option '-charArray' (<charArrayField>): 'abcd' is not a single character

It appears that Picocli treats char[] as char. The expected behavior instead would be that the input String is converted to char[] using its toCharArray() method.

Couple more notes:

  • Using char[] as the data type is a pretty common thing when the parameter is a password. The array can be manually zeroed when the password is no longer needed, thus completely removing it from memory while its parent object still lives.
  • Using String for the same purpose is sub-optimal, as JVM does all kinds of magic with Strings and the password could, for example, be interned - irrevocably burning it into the JVM memory for the entire runtime of the application.
  • The original code is lifted directly from a JCommander-based implementation, which handled this situation properly.
  • I attempted to fix the problem, but the type conversion code seems very convoluted. Without gaining a deep understanding of all the code branches, I risk causing more problems than I solve.

@remkop
Copy link
Owner

remkop commented Mar 23, 2019

Hi,

What you're saying makes total sense. It has been on the todo list to improve this but other things took priority.

As you pointed out, the type conversion logic in picocli is not easy, because it needs to support adding values to multi-value types like Map, Collection and arrays, as well as simply replacing values of single-value fields. However, I have some idea for adding support for password char[] values without changing the type conversion logic too much.

I am considering combining a fix for this with #536 to improve password handling overall and cutting a separate 3.9.x maintenance release for this.

How urgent is this for you? I am currently working on #358, which is quite involved and will probably take some more time. I'd like to finish that and do a picocli-4.0-alpha-1 release before context-switching to the topic of passwords.

@remkop remkop added this to the 3.9.6 milestone Mar 23, 2019
@triceo
Copy link
Contributor Author

triceo commented Mar 24, 2019

@remkop I can wait. Combining this with #536 makes a lot of sense.

@remkop
Copy link
Owner

remkop commented Mar 31, 2019

@triceo Would you be okay with getting this in 4.0.0-alpha-2, or do you need a backport to the 3.9.x branch? I would prefer to spend as much time as possible on 4.0...

@triceo
Copy link
Contributor Author

triceo commented Mar 31, 2019

@remkop That depends on how many incompatible changes there are in 4.0, and when that is coming out. Right now, I don't know anything about the release.

@remkop
Copy link
Owner

remkop commented Mar 31, 2019

So far it’s all backwards compatible.

The final GA release is a few months away, but the alpha releases are high quality and pass all 1500+ unit tests.

I’m releasing this functionality as alpha releases so that I have the option to change the API if peer review turns up any issues.

@triceo
Copy link
Contributor Author

triceo commented Mar 31, 2019

@remkop Unfortunately, the release rules won't let me depend on a non-final artifact. That said, I understand your motivation for not putting this into a 3.9 - I'll wait for a final 4.0 then.

@remkop
Copy link
Owner

remkop commented Mar 31, 2019

the release rules won't let me depend on a non-final artifact.

Understood. Thanks for the clarification. I’ll try to do a separate 3.9.x release for these then.

@remkop
Copy link
Owner

remkop commented Apr 6, 2019

@triceo I’ve pushed some commits to the 3.X branch to improve support for interactive (password) options. I’ll create a 3.9.6 release for this soon, maybe later today.

This includes the char[] type support you requested, but I’ve limited my changes so far to only impact interactive options.

So, for “normal” (non-interactive) options, using a field of type char[] still works similar to using a field of type int[]: picocli expects each option parameter to be a single value that can be converted to a char or int and added to the array. Your PR adds a test for “normal” (non-interactive) options, and this test will still fail, so I cannot merge this PR yet.

It does make sense to go one step further and treat char[] more like Strings than like int[] also for “normal” (non-interactive) options. (I’ll probably hold off on this until 4.0.) Making this change would potentially break existing applications so we need to provide a setting that allows users to switch back to the old behavior. Any ideas for a good name for such a configuration?

@triceo
Copy link
Contributor Author

triceo commented Apr 6, 2019

Hmm... picocli.disableCharArrayToStringConversion?
A bit long, but descriptive.

@remkop remkop modified the milestones: 3.9.6, 4.0-alpha-2 Apr 6, 2019
@remkop
Copy link
Owner

remkop commented Apr 13, 2019

Adding some ideas:

  • charArraysCanCaptureStrings (default true going forward)
  • charArraysCaptureSingleCharacters (Default false)

@remkop remkop modified the milestones: 4.0-alpha-2, 4.0-alpha-3 Apr 18, 2019
remkop added a commit that referenced this pull request May 3, 2019
…port capturing multi-char values in char[] types for non-interactive options
@remkop
Copy link
Owner

remkop commented May 3, 2019

I just took another look at this, but I'm not sure that I want to go ahead with this:

The main goal (support for char[] types for interactive password options) has been achieved in 3.9.6.

This ticket is still open because we are considering to go one step further and also allow normal (non-interactive) options to capture multi-char (String) values in a char[] type. On the face of it this would make things more consistent, but I hesitate to go ahead:

  • I'm not sure if there is any user demand for this (nobody asked for this)
  • it would introduce more special logic for char[] in a few places, like CommandLine$Interpreter.getTypeConverter and CommandLine$Interpreter.applyValuesToArrayField (this is a minor issue, just mentioning it)
  • I am unsure how to deal with multiple values: append to the array, or overwrite the previous value? For other array types, the expected behaviour is to append, while for single value types the expected behaviour is to overwrite. Would we need to introduce another parser switch for this? Seems overkill since nobody asked for this...

So I will probably postpone this remaining work until someone asks for this feature.

Thoughts?

@triceo
Copy link
Contributor Author

triceo commented May 3, 2019

@remkop It is true that, since 3.9.6, my use case is solved and I don't need anything more. If doing this feature in its entirety is too much for the existing system, feel free to postpone/ignore it. :-)

@remkop remkop merged commit a0b6d67 into remkop:main Oct 16, 2022
@remkop
Copy link
Owner

remkop commented Oct 16, 2022

Finally merged! 😅
Thank you for the contribution!

remkop added a commit that referenced this pull request Oct 16, 2022
remkop added a commit that referenced this pull request Oct 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants