Fixes #20348 - Inconsistently escaped quotes for Host Parameter

[apuntamb]

No description provided.

[mbacovsky]

@apuntamb thank you for the patch!
There are some tests failing and it is related to the patch. Could you please take a look?
You can run the tests locally by rake test in the repo directory. It runs in less then a second but if you can run only the relevant tests you can run rake test TEST=test/unit/options/normalizers_test.rb.

I'd also recommend to start with adding new test covering the syntax you want to add and develop towards the green tests. let me know if you need any help.

[mbacovsky]

Please see the comment above

[apuntamb]

@mbacovsky thanks for the review. So, currently, with this change, tests are failing only for array parsing. Remaining tests are green. So, how should I go about it ?

[apuntamb]

@mbacovsky any suggestions on my above comment?

[apuntamb]

Apologies for the delay.
@mbacovsky @tstrachota I have a few questions on the existing scenarios :

• Is this scan method intended for array parsing only ? If yes, then do we need to consider "diskinfo2=[{\"group\": \"apache\" separately or differently for scenario where it's a {} which carries the key:value ?
• Can't we do this by using in-built JSON library ? Something like value = JSON.parse(value) if value.start_with?('[').
• And also, in normalizers_test.rb , we have formatter.format("a=1,b=[1,2,3],c=3").must_equal({'a' => '1', 'b' => ['1', '2', '3'], 'c' => '3'}). I can see all kinds of inputs being converted to string, so is this expected that even if there are integers in array, it should return us array of strings ?

Thank you.

[mbacovsky]

@apuntamb, np, take your time!
Re 1/ Yes, so far we considered only array of simple values. In this case it is basically a list of values containing , which breaks the parsing by regexp. That should be fixed
Re 2/ I don't think we should mix CSV with JSON. But thanks for this idea. It led me to noticing, that the format used in the issue is actually wrong. I'll update the issue, please check in there. Parsing the list will be difficult with regexps only and we fought that before in a list normalizer. We may consider here the solution from the list normalizer too (HammerCLI::CSVParser).
Re 3/ I'm not sure what was the reason behind that. I don't think it is necessary to solve the types as part of this PR. Could you file new issue if it to track it?

[apuntamb]

@mbacovsky thanks for the detailed answers!

To add to those, in case of comma separated list,
hammer -u username host update --name testserver.example.com --parameters diskinfo3=['{"group": "apache", "pciid": 160, "fstype": "ext4", "mntopts": "", "owner": "root", "path": "/mount/point/1", "fmtopts": "", "scsiid": 1, "size": 75}', '{"group": "apache", "pciid": 160, "fstype": "xfs", "mntopts": "", "owner": "apache", "path": "/mount/point/2", "fmtopts": "", "scsiid": 2, "size": 200}']
it throws argument error "Value must be defined as a comma-separated list of key=value or valid JSON" from #format method. This means, it is unable to parse the JSON input.

On debugging, the value of val it gives "diskinfo3=[{\"group\": \"apache\", \"pciid\": 160, \"fstype\": \"ext4\", \"mntopts\": \"\", \"owner\": \"root\", \"path\": \"/mount/point/1\", \"fmtopts\": \"\", \"scsiid\": 1, \"size\": 75}," instead of the full value. So as you mentioned in the PR about the format being wrong, I guess we need to make changes in the format method before it checks for valid_key_value? , so that it gives us the entire value of diskinfo3 param. Is that right, am I missing something else ?

[apuntamb]

@tstrachota @mbacovsky any updates ?

[bkearney]

@mbacovsky ping on this.

[mbacovsky]

@apuntamb sorry for losing this from my radar, @bkearney thanks for the poke.
I checked the input and what gets in the normalizer and it seems it is not properly quoted in shell. What we get in val is raw input from Clamp parser an it seems the parameter is pre-processed by shell.
After some trial and error I found this input giving expected results

hammer host update --id 1 --parameters diskinfo3=[\'{\"group\":\"apache\"}\',\'{\"group\":\"postgresql\"}\'],paramx=y

gives

[2] pry(#<HammerCLI::Options::Normalizers::KeyValueList>)> val
=> "diskinfo3=['{\"group\":\"apache\"}','{\"group\":\"postgresql\"}'],paramx=y"
[3] pry(#<HammerCLI::Options::Normalizers::KeyValueList>)> parse_key_value(val)
=> {"diskinfo3"=>["{\"group\":\"apache\"}", "{\"group\":\"postgresql\"}"], "paramx"=>"y"}

and sends to API as:

{
    "host" => {
                      "puppetclass_ids" => [],
                   "compute_attributes" => {},
             "content_facet_attributes" => {},
        "subscription_facet_attributes" => {},
           "host_parameters_attributes" => [
            [0] {
                 "name" => "diskinfo3",
                "value" => "[\"{\\\"group\\\":\\\"apache\\\"}\", \"{\\\"group\\\":\\\"postgresql\\\"}\"]"
            },
            [1] {
                 "name" => "paramx",
                "value" => "y"
            }
        ]
    }
}

Here it seems it works properly and does not need fixing. Just the quoting is tricky. Maybe doc update would help. What do you think?

Another case would be to pass The whole list as a string e.g.:

hammer host update --id 1 --parameters diskinfo3=$'\'[{\"group\":\"apache\"}]\'',paramx=y

but I didn't found a way to do that. I guess we should define first what input and output we want in this case and then fix the parser. We may consider replacing the regexp parser with something similar to HammerCLI::CSVParser Any idea?

[shiramax]

@mbacovsky @apuntamb
I saw this PR : theforeman/foreman#5241 which adds all types to host parameters, So maybe we can parse each value by its type.
that means we need to add a new option --type for each parameter.

[kgaikwad]

@shiramax, @mbacovsky,
I have also created hammer side PR-409 which will add separate field i.e parameter_type for global-parameter set and set-parameter commands. But I think it is not covering an above scenario for command i.e hammer host update --id 1 --parameters.

To add type for each param inside this command hammer host update --id 1 --parameters we will need to define fix input format to pass - param, value and its type.

[apuntamb]

@shiramax and @kgaikwad thank you for your inputs.
@mbacovsky Np. Thanks for debugging in depth.
I agree with the input that you've passed diskinfo3=[\'{\"group\":\"apache\"}\',\'{\"group\":\"postgresql\"}\'],paramx=y it works fine. But, I assume, with the input that we considered originally had issues with parsing because of the regex parser. I had earlier tried with the JSON library and used JSON.parse for the input but, it wasn't sufficient. I like the idea of HammerCLI::CSVParser to be extended for this.
I also think we can decide on the input that we should support for --parameters and update the document accordingly. Because as @kgaikwad mentioned, #409 does not support our scenario and hence, we need either a new definition of inputs or a new parsing mechanism.

[mbacovsky]

@apuntamb, thank you for your effort on this. I found out the original issue was fixed with 494ae8e3db821 that was focusing on list values which had similar problems. As part of the work there the usage of lists and key/values was made consistent. Therefore I'm going close the pr in favor of the mentioned patch and update the related bugs accordingly. If you have any concerns about the solution please let me know.