validate_not_empty broken with 1.2.12

[vdesabou]

Bashly Version

Latest Docker Image

Description

After upgrading to latest docker image 1.2.12, my commands using https://github.com/DannyBen/bashly/blob/444eea2e14e71eb8efa00c5b1f665f8c8dba5637/lib/bashly/libraries/validations/validate_not_empty.sh is no longer working, I think due to #629

I have a command with

    - name: exec
      group: Container
      help: |-
        🪄  Execute command in a container

      flags:
      - *container

      - long: --command
        required: true
        validate: not_empty
        arg: command
        help: |-
          📲 Command to execute

playground container exec --command "yum -y install libaio"

fails with error 1

with set -x I see:

+ declare -g version=1.0.0
+ set -e
+ run container exec --command 'yum -y install libaio'
+ declare -g long_usage=
+ args=()
+ declare -g -A args
+ other_args=()
+ declare -g -a other_args
+ deps=()
+ declare -g -A deps
+ env_var_names=()
+ declare -g -a env_var_names
+ input=()
+ declare -g -a input
+ unique_lookup=()
+ declare -g -A unique_lookup
+ normalize_input container exec --command 'yum -y install libaio'
+ local arg passthru flags
+ passthru=false
+ [[ 4 -gt 0 ]]
+ arg=container
+ [[ false == true ]]
+ [[ container =~ ^(--[a-zA-Z0-9_-]+)=(.+)$ ]]
+ [[ container =~ ^(-[a-zA-Z0-9])=(.+)$ ]]
+ [[ container =~ ^-([a-zA-Z0-9][a-zA-Z0-9]+)$ ]]
+ [[ container == \-\- ]]
+ input+=("$arg")
+ shift
+ [[ 3 -gt 0 ]]
+ arg=exec
+ [[ false == true ]]
+ [[ exec =~ ^(--[a-zA-Z0-9_-]+)=(.+)$ ]]
+ [[ exec =~ ^(-[a-zA-Z0-9])=(.+)$ ]]
+ [[ exec =~ ^-([a-zA-Z0-9][a-zA-Z0-9]+)$ ]]
+ [[ exec == \-\- ]]
+ input+=("$arg")
+ shift
+ [[ 2 -gt 0 ]]
+ arg=--command
+ [[ false == true ]]
+ [[ --command =~ ^(--[a-zA-Z0-9_-]+)=(.+)$ ]]
+ [[ --command =~ ^(-[a-zA-Z0-9])=(.+)$ ]]
+ [[ --command =~ ^-([a-zA-Z0-9][a-zA-Z0-9]+)$ ]]
+ [[ --command == \-\- ]]
+ input+=("$arg")
+ shift
+ [[ 1 -gt 0 ]]
+ arg='yum -y install libaio'
+ [[ false == true ]]
+ [[ yum -y install libaio =~ ^(--[a-zA-Z0-9_-]+)=(.+)$ ]]
+ [[ yum -y install libaio =~ ^(-[a-zA-Z0-9])=(.+)$ ]]
+ [[ yum -y install libaio =~ ^-([a-zA-Z0-9][a-zA-Z0-9]+)$ ]]
+ [[ yum -y install libaio == \-\- ]]
+ input+=("$arg")
+ shift
+ [[ 0 -gt 0 ]]
+ parse_requirements container exec --command 'yum -y install libaio'
+ [[ 4 -gt 0 ]]
+ key=container
+ case "$key" in
+ break
+ missing_deps=
+ command -v docker
++ command -v docker
++ head -n1
+ deps['docker']=/usr/local/bin/docker
+ [[ -n '' ]]
+ action=container
+ case $action in
+ action=container
+ shift
+ playground_container_parse_requirements exec --command 'yum -y install libaio'
+ [[ 3 -gt 0 ]]
+ key=exec
+ case "$key" in
+ break
+ action=exec
+ case $action in
+ action=exec
+ shift
+ playground_container_exec_parse_requirements --command 'yum -y install libaio'
+ [[ 2 -gt 0 ]]
+ key=--command
+ case "$key" in
+ break
+ action='container exec'
+ [[ 2 -gt 0 ]]
+ key=--command
+ case "$key" in
+ [[ -n x ]]
+ args['--command']='yum -y install libaio'
+ shift
+ shift
+ [[ 0 -gt 0 ]]
+ [[ -z x ]]
+ [[ -n '' ]]
+ args['--container']=connect
+ [[ -n '' ]]
+ args['--shell']=bash
+ [[ -v args[--command] ]]
++ validate_not_empty 'yum -y install libaio'
++ [[ -z yum -y install libaio ]]
+ validation_output=

Contents of bashly.yml

- name: exec
      group: Container
      help: |-
        🪄  Execute command in a container

      flags:
      - *container

      - long: --command
        required: true
        validate: not_empty
        arg: command
        help: |-
          📲 Command to execute

Reproduction Steps

Use a flag with validate: not_empty

Actual Behavior

command exits with 1

Expected Behavior

command is executed, as before 1.2.12

[DannyBen]

Checking.

[DannyBen]

I found the problem. The not empty validation function is the only validation function that uses &&, and therefore when there is a failure, it returns non zero, which aborts the script in strict mode.

The change is going to be in the built in validation function - so you can update it on your side for now, and everything should work again:

# src/lib/validate_not_empty.sh
validate_not_empty() {
  if [[ -z "$1" ]]; then
    echo "must not be empty"
  fi
}

Please confirm this fixes your issue, since I could not use your reproduction case verbatim.

I will also ensure that the validation specs (as tested in example/validation) cover all cases.

[vdesabou]

Yes it fixes my issue.

Thanks for the quick response (as usual !)

[DannyBen]

Thanks for the report. Keep the issue open, I will close it upon release.

[meleu]

Correct me if I'm wrong:

In #629 we unintentionally made it mandatory that when in strict mode, the custom validations MUST ALWAYS return success.

If that's the case, I think we should find an alternative to remove this cognitive burden from the developer's mind (and one more caveat to add to the documentation).

Forcing a function to return success when it wants to say that a validation failed really annoys me! 😖

I understand that validations rely on output generation, but forcing success to make my program work is very unintuitive.

What do you think about using something like this in the lib/bashly/views/flag/validations.gtx?:

-validation_output="$(validate_{{ validate }} "$value")"
+validation_output="$(validate_{{ validate }} "$value" || true)"

[DannyBen]

You are correct. The || true solution was the first one that popped to mind, but I dislike it more, for several reasons:

1. It may hide unintentional errors.
2. Every function in a strict-mode bash script must always return success, unless it is intentionally designed to error.
3. If we allow these functions to return any code by updating the caller - why not do this to other functions?
4. I do not feel like there is cognitive strain in the validation functions - users can simply follow the pattern of the functions we provide of "if then echo".

All that said, I am not totally opposed to the || true addition, but I am currently leaning towards not applying it.

[meleu]

Your first reason is the one that convinced me:

1. It may hide unintentional errors.

Indeed... If the validate_* function fails for whatever reason with no output the || true would make it continue with no validation.

Anyways, I'm gonna share my random thoughts here with no intention to push the || true solution...

---

I've been reflecting about this and trying to find why I have such a biased opinion. And I think I've found it...

I'm averse to the idea of having a function that claims to validate something returning success when the validation fails.

I think the bias is a resistance to see the validate_* functions as a "bashly thing" and a desire to make them usable in ("copy'n'pastable" to) a non-bashly development environment. I'm realizing that I should give up on this idea.

---

Trying to explain my feelings of when I was new to bashly and learning about the validate: thing...

I was learning mostly by looking at the awesome examples we have in the documentation.

I saw the validations example. Became excited and assumed I understood how it works. Tried it and failed.

Read the documentation more and learned that my validate_* functions MUST output something. I was slightly bothered. With a feeling that I'm being forced to write my code in a "bashly way". But, man!, it was a clever solution to the "which message to show to user when validation fails" problem. This and all the awesome things bashly offers made me tolerate it.

Now, the "MUST return success when in strict mode" is another "thing to tolerate". That's what motivated me to being vocal here...

But, as I mentioned, I'm starting to accept the fact that a validate_* function IS a bashly thing. Also, nothing stops me from implementing my own validation solution and still use bashly (it even provides a before hook to make it easier).

And I'm grateful that @DannyBen thoughtfully explain his design decisions. :)

[DannyBen]

Hey @meleu, and thanks for engaging. I love these discussions, and all the "decisions" we make here are open to changes if new information becomes available, or convincing arguments are made.

About "echo string == error"

As you realized (and despite being slightly annoyed by it), the "echo something == error" approach was a way for the function to provide an error message.

I do not feel like this is purely a "bashly thing", here is why:

1. In other programming languages, you would either return a string, or "raise" an error with a message.
2. Bash does not support neither of these.
3. Returning a string from a bash function is commonly emulated by echoing it, and calling it with $(...).
4. So - I see it as a "bash thing" that was fully, and unapologetically adopted by bashly.

About "must return a success code"

This is true for ANY function you write in strict mode (or at least when set -e is on). Any non zero return code, anywhere - be it a bashly function, your own function, or an external command - will halt the bash script immediately. So here, I do not feel like this decision adds a cognitive strain - if anything, it nudges you to a good practice direction, and reminds you that non-zero return codes mean error.

I feel that this is a good bash programming practice. This is why the set -e is the default mode of generated scripts, but recognize that some people may prefer something else, and in this case they can change it.

In other words - if you don't want to worry about function return codes, you need to change the strict setting, instead of arbitrarily hiding return code from some functions (validations in this case).

In other-other words - this approach makes it so that the problem is solved in its source rather than its symptoms.

Am I convincing someone other than myself? :)

[meleu]

Disclaimer: You already convinced me to not use || true in the validation caller. So, we're here just for the, let's say, "philosophical" conversation. 🤓

---

Turns out that those 2 points you mentioned highlight where my bias comes from! :)

This is the way I was used to manage validations (before incorporating bashly):

# Dead simple validation function!
# Most likely a single command.
# It's JUST the validation. Nothing more.
# Send text to stdout/stderr would be a plus, but not required.
#
# What's really important here is:
# exit status 0 means "something" is valid, otherwise it's invalid.
validate_something() {
  [[ -z "${SOME_VAR}" ]]
}

my_func() {
  # The caller is responsible for giving a more meaningful
  # error message to the end user.
  if ! validate_something; then
    echo "failed to validate something" >&2
    return 1
  fi

  echo "doing something with valid data"
}

I recognize that I mistakenly assumed this 👆 approach as some sort of a "pure bash way". But maybe this is just a "meleu way". 😅

Managing Complexity

I think it's all about managing complexity. Let's go fancy and mention the Law of conservation of complexity:

Every application has an inherent amount of complexity that cannot be removed or hidden. Instead, it must be dealt with, either in product development or in user interaction.

In our case: "either in validation function or in its caller".

Our complex things

In our situation here, these are the "complex things":

1. find a programatic way to say input is valid
2. show an error message to the user when input is not valid
3. validation should work even when in strict mode

meleu way

In (pre-bashly) "meleu way", these points are solved by:

1. a validation function and its exit/return status
2. the caller checks function return and make up a meaningful message to the user
3. nothing to solve here, strict mode is not a concern

• 👍 good
  • validation function is must be extremely simple
  • no need to even think about strict mode
• 👎 bad
  • puts a burden in the caller, as the responsible to make up a meaningful error message
  • because of this 👆 burden, it's not simple to use such approach in bashly (as it aims to abstract the validation caller from the developer)

Summing up: it favors validation function simplicity and makes the caller handle the complexity.

bashly way

In the "bashly way", they are solved like this:

1. a validation function and its output
2. the validation function must make up a meaningful message to the user
3. the validation function must finish with success in strict mode

• 👍 good
  • makes it simple to abstract validation call
  • smartly handles the way to create error messages to the user
• 👎 bad
  • the programmer MUST be aware that
    • validation must output something to mean failure
    • validation must return success when in strict mode

Summing up: it favors caller simplicity (so it can be easily integrated in bashly) and makes the validation function creator handle the complexity.

---

That being said, I reaffirm that I also don't want the || true solution. As that would put another cognitive burden in the validation function programmer: keep in mind that if your validation function is buggy, the strict mode will not protect you.

[DannyBen]

Your bias is completely understood, and the way you did things was also good in my opinion.

However, consider this:

With the approach you demonstrated, if the validation needs to be used in multiple places, you have to rewrite the error message. And then, you may realize that you want to have a "wrapper" function that uses the simple boolean validation function, and displays a consistent error message, then exits if the validation fails.

This would bring you closer to what bashly is doing - with one difference: In your code, you decide where to exit, whereas in bashly, the validation is handled by the framework, and it decides when to exit. This is the only difference as I see it.

If the thought of reusing the code in either a bashly or non bashly script is important, I would do something like this:

# reusable
is_docker_installed() {
  # return 0 if docker is available, 1 otherwise
}

# bashly specific validation function
validate_docker_installed() {
  is_docker_installed || echo "docker is not installed"
}

# "generic bash" validation function
validate_docker_installed() {
  is_docker_installed || (echo "docker is not installed" && return 1)
}

I feel this demonstrates clearly that the only difference, is who is responsible for exiting.

Anyways - I think are both in agreement, and the different points of view are just a matter where we come from.

[meleu]

I liked the conversation we had here. And I feel like this issue can be closed as "solved".

[DannyBen]

Sure, me too. I am usually keeping issues open until there is a release that fixes them, so its easy for people to distinguish between solved and solved-but-not-released.