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

Yamlcpp treats integers as booleans #2833

Open
Piankero opened this issue Jul 27, 2019 · 4 comments

Comments

@Piankero
Copy link
Contributor

commented Jul 27, 2019

Steps to Reproduce the Problem

kdb set user/yaml/bug1 false
#> Create a new key user/yaml/bug1 with string "false"

kdb set user/yaml/bug2 0 
#> Create a new key user/yaml/bug2 with string "0"

kdb export user/yaml yamlcpp
#> bug1: false
#> bug2: false

Expected Result

kdb export user/yaml yamlcpp
#> bug1: false
#> bug2: 0

Actual Result

0 will be treated as false

System Information

  • Elektra Version: master

Further Log Files and Output

Since I needed it fast I fixed this specific usecase myself in my branch. Test fixes or other implications I did not investigate but this is a good first point for further coding.

@markus2330

This comment has been minimized.

Copy link
Contributor

commented Jul 27, 2019

Thank you for reporting this problem!

@markus2330

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2019

Is this problem specific to Yamlcpp? Iirc other yaml plugins had similar code for boolean values.

(@sanssecours please do not redo the benchmarks because of such fixes.)

@sanssecours

This comment has been minimized.

Copy link
Member

commented Aug 1, 2019

Is this problem specific to Yamlcpp?

The other YAML plugins will show the same behavior.

The problem here is that we use YAML booleans (usually true and false) to save Elektra’s boolean values (0 or 1). That is also why the plugins that do not use types, such as dump and quickdump, work correctly.

I think it would be a good idea to add the metakey type with the value boolean in the get direction and remove it in the set direction. This way the plugin that writes back a value knows the correct type. I already did that for YAML CPP in my local copy of the repository.

However, this situation is still problematic, since we do not have a (non deprecated) plugin that converts boolean values to a canonical version of true or false. Currently the set plugin has to interpret the value from the user (e.g. true, TRUE, ON, off, …). In theory the type plugin could help in this situation, but it always converts back to the user value, after it successfully checked a type. This works, if we do not use types in a storage format. A set plugin that uses types on the other hand would still like to know, if a boolean value represents true or false, so it can act accordingly.

One option to solve this whole dilemma would be to always use string values, when writing back to YAML. However, I clearly prefer something like this:

key: true

, over this:

key: !elektra/meta
     - "true"
     - type: boolean

.

@markus2330

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2019

That is also why the plugins that do not use types, such as dump and quickdump, work correctly.

Afaik yajl also works (it has not fully changed to use our new type system, though).

I think it would be a good idea to add the metakey type with the value boolean

Yes, without type you are lost, you cannot know what "0" means.

in the get direction and remove it in the set direction.

You do not need to "remove" it. When it is a boolean, the value "0" is rendered to true and not to "0". So the information is persisted.

In theory the type plugin could help in this situation, but it always converts back to the user value, after it successfully checked a type.

The type plugin is unfortunately quite difficult to use (and buggy?), it should normalize to 0/1 from a known list of true/false values. Maybe you can achieve the desired behavior by configuring the plugin. We can then change the defaults so that it will work correctly with correctly implemented storage plugins (I hope you will also update your tutorial for the types).

At the moment, your plugin should at least work with:

kdb set user/tests/b 0
cat `kdb file /tests`
#> {
#>    "b": "0"
#> }
kdb setmeta user/tests/b type boolean 
cat `kdb file /tests`
#> {
#>    "b": false
#> }

. As in this situation no normalization is needed.

One option to solve this whole dilemma would be to always use string values, when writing back to YAML. However, I clearly prefer something like this:

I also clearly prefer the short version!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.