search

ManageIQ / manageiq-automation_engine

Automation engine for ManageIQ
Apache License 2.0
11 stars 74 forks source link

Bug with handling of Arrays in Automate #480 #484

Closed akhilkr128 closed 2 years ago

akhilkr128 commented 2 years ago

This PR aims to fix the issue - https://github.com/ManageIQ/manageiq-automation_engine/issues/480

backport: also include https://github.com/ManageIQ/manageiq/pull/21520

After the change- Screenshot 2021-10-13 at 6 42 34 PM

After spending lots of hours on regex to match the scenarios of wrapping special characters around the option values, I came to sense. I realized that array elements could be stored and retrieved with this small change without worrying about the special characters.

Fryguy commented 2 years ago

LOL that's clever :) It could still technically be a problem, but it's less likely to be a problem than a straight "," or ", ", so I'm fine with merging as is, which should at least unstick most callers.

akhilkr128 commented 2 years ago

It could still technically be a problem

Yes, if a user puts the option values like "apple, Orange" instead of "apple, Orange" it will create a problem. I have changed the separator value from " ," to ",,". I think it will be a rare case to give an option's value with two adjacent commas.

coveralls commented 2 years ago

Pull Request Test Coverage Report for Build 5981

Totals Coverage Status
Change from base Build 5886: 0.0%
Covered Lines: 5055
Relevant Lines: 5895
💛 - Coveralls
akhilkr128 commented 2 years ago

It turns out that some spec changes are needed here. :-)

Fryguy commented 2 years ago

,, seems ok, but it's a problem if someone has an empty entry in an array. I'm wondering if it's better to use a non-printable character, specifically 0x1F, which is defined as the "Unit Separator" character (like a "column separator" or "field separator" character) and is made for this purpose (you could also use 0x1E which is the "Record Separator" character, but I usually think of that more like a "row separator")

Fryguy commented 2 years ago

Can you please also add a spec demonstrating that this fixes the problem from the issue? (i.e. a string with embedded commas)?

akhilkr128 commented 2 years ago

Can you please also add a spec demonstrating that this fixes the problem from the issue? (i.e. a string with embedded commas)?

Sure

akhilkr128 commented 2 years ago

Changes done.

Fryguy commented 2 years ago

Found one minor thing, but overall LGTM.

miq-bot commented 2 years ago

Checked commit https://github.com/akhilkr128/manageiq-automation_engine/commit/2ab618686897e9f0ba963f17dd0e72cdac66add6 with ruby 2.6.3, rubocop 1.13.0, haml-lint 0.35.0, and yamllint 4 files checked, 0 offenses detected Everything looks fine. :cake:

Fryguy commented 2 years ago

Backported to morphy in commit 3bdae46d7efd464a22409df617e969fb160d5b58.

commit 3bdae46d7efd464a22409df617e969fb160d5b58
Author: Jason Frey <fryguy9@gmail.com>
Date:   Mon Oct 25 09:50:50 2021 -0400

    Merge pull request #484 from akhilkr128/multi_select_array_bug

    Bug with handling of Arrays in Automate #480

    (cherry picked from commit c75b6f543cb973772f5097ac12235c81fd4d7fdb)