Handle empty input in AddStringKeyStoreCommand #39490
Conversation
This change ensures that we do not make assumptions about the length of the input that we can read from the stdin. It still consumes only one line, as the previous implementation
jkakavas
added
>bug
:Core/Infra/Settings
|
Pinging @elastic/es-core-infra |
| } | ||
| value = writer.toCharArray(); | ||
| } catch (IOException e) { | ||
| throw new UserException(ExitCodes.DATA_ERROR, e.getMessage()); |
There was a problem hiding this comment.
UserException is meant to be something clear for users which does not warrant a stack trace. Is the raw IOException message actually clear? What possible error scenarios exist?
There was a problem hiding this comment.
You're right, this was an oversight. IOException can be thrown by read() in the ByteBuffer, so I added a generic message and added the IOException as cause in the UserException
|
This is ready for another pass @rjernst , thanks |
|
Ping @rjernst |
| } | ||
| value = writer.toCharArray(); | ||
| } catch (IOException e) { | ||
| throw new UserException(ExitCodes.DATA_ERROR, "Error reading the value from stdin", e); |
There was a problem hiding this comment.
This message is still opaque to a user. Remember UserException does not print a stacktrace. In this case, I think we should just let IOException bubble up and print the full stacktrace? What benefit is there in wrapping it? If there is a subclass of io errors that the user can directly do something about, then we should distinguish that and message appropriately with actions the user can take.
There was a problem hiding this comment.
I got the idea to add the exceptions to the UserException from Jay's feedback in #38498 (comment) but I see what you're saying here. There is no subset of i/o errors that I know of where we could print something useful for the user, so I'll let the IOException be thrown
This change ensures that we do not make assumptions about the length of the input that we can read from the stdin. It still consumes only one line, as the previous implementation
This change ensures that we do not make assumptions about the length of the input that we can read from the stdin. It still consumes only one line, as the previous implementation
This change ensures that we do not make assumptions about the length of the input that we can read from the stdin. It still consumes only one line, as the previous implementation Co-authored-by: Ioannis Kakavas <[email protected]>
This change ensures that we do not make assumptions about the length
of the input that we can read from stdin and that we can handle empty
strings. The behavior remains the same with regards to the fact that we
don't accept multi-line input
Resolves: #39413