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

yajl: Elektra's boolean not supported #2571

Open
markus2330 opened this Issue Apr 2, 2019 · 15 comments

Comments

Projects
None yet
2 participants
@markus2330
Copy link
Contributor

markus2330 commented Apr 2, 2019

Steps to Reproduce the Problem

kdb mount config.json user/tests/yajl yajl type
kdb set user/tests/yajl true
kdb setmeta user/tests/yajl type boolean
kdb set user/tests/yajl 1

kdb rm user/tests/yajl
kdb umount user/tests/yajl

Expected Result

That still true is in the config file as true and 1 should be the same.

cat `kdb file user/tests/yajl`
#> true

Actual Result

 Sorry, 1 warning was issued ;(
 Warning (#78):
        Description: Unknown or unsupported type found during streaming, assume key as string, type lost
        Ingroup: plugin
        Module: yajl
        At: /home/jenkins/workspace/libelektra_master-Q2SIBK3KE2NBEMJ4WVGJXAXCSCB77DUBUULVLZDKHQEV3WNDXBMA/libelektra/src/plugins/yajl/yajl_gen.c:166
        Reason: got boolean which is neither true nor false
        Mountpoint: user/tests/yajl
        Configfile: /home/markus/.config/config.json.26097:1554202289.309349.tmp
Set string to "1"
cat `kdb file user/tests/yajl`
#> "1"

System Information

  • Elektra Version: master

Implementation Hint

The yajl plugin needs to:

  • render Elektra's "0" and "1" values to JSON's false and true.
  • fail with type errors if non-supported types are found

@markus2330 markus2330 added bug usability lang/c and removed usability labels Apr 2, 2019

@markus2330

This comment has been minimized.

Copy link
Contributor Author

markus2330 commented Apr 2, 2019

@kodebach is this still possible that the type plugin can be reconfigured to normalize to "true" instead of "1".

@kodebach

This comment has been minimized.

Copy link
Contributor

kodebach commented Apr 2, 2019

No this is not supported by the type plugin, I didn't know there was a use case for this.

IMO it also doesn't make sense. The Elektra-way of representing a boolean is 0 and 1. If a storage format supports types, the conversion from the Elektra-representation to the representation of the storage format, should be done by the storage plugin. In the end the storage plugin for format X should be the bridge between Elektra and format X.

@kodebach

This comment has been minimized.

Copy link
Contributor

kodebach commented Apr 2, 2019

Also isn't the bigger problem the restoring of values? Restoring is done in presetstorage, and you explicitly requested, that values are always restored to the representation chosen by the user, when setting the value. This means, if you used kdb set user/tests/yajl on in your example, the yajl plugin will receive the value on.

What we actually need here, is a way to set the representation to which values are restored, regardless of how the user set the value. This could be added very easily. However I am not sure there is currently a way to specify the config for a plugin, from another plugin. Meaning the user would still have to configure type correctly when using yajl.

@markus2330

This comment has been minimized.

Copy link
Contributor Author

markus2330 commented Apr 2, 2019

that values are always restored to the representation chosen by the user

This would not be a problem as in JSON the only available presentation is true/false, so the user cannot choose.

Meaning the user would still have to configure type correctly when using yajl.

This would also no be no problem because yajl can add a config/needs to reconfigure the type plugin.

The Elektra-way of representing a boolean is 0 and 1. If a storage format supports types, the conversion from the Elektra-representation to the representation of the storage format, should be done by the storage plugin.

Yes, I fully agree. The advantage of your approach is that it will also allow intermediate plugins (between type and storage) to see the correct representation of booleans.

I updated the "Implementation Hint" above to reflect this.

@kodebach another question: JSON only supports double/boolean/string. Is it possible to say to the type plugin that only these 3 types are allowed?

This would also fix the JSON types mentioned in #1092.

@markus2330 markus2330 referenced this issue Apr 2, 2019

Open

Unify type system #1092

4 of 8 tasks complete
@kodebach

This comment has been minimized.

Copy link
Contributor

kodebach commented Apr 2, 2019

This would not be a problem as in JSON the only available presentation is true/false, so the user cannot choose.

They cannot choose in JSON, but they can choose when using kdb set

between type and storage

The ordering should be like this: getstorage, type, [other], type, setstorage, Meaning there shouldn't ever be any plugins between type and storage.

I updated the "Implementation Hint" above to reflect this.

There is still the problem that e.g. kdb set user/tests/yajl on will not work, without changes to type, because in this case type will pass the value on to the setstorage plugin.

JSON only supports double/boolean/string. Is it possible to say to the type plugin that only these 3 types are allowed?

It shouldn't be hard to restrict the allowed types via the config.

@markus2330

This comment has been minimized.

Copy link
Contributor Author

markus2330 commented Apr 2, 2019

They cannot choose in JSON, but they can choose when using kdb set

What is chosen in kdb set cannot be remembered anyway if the file format does not support it.

There is still the problem that e.g. kdb set user/tests/yajl on will not work, without changes to type, because in this case type will pass the value on to the setstorage plugin.

Why does it not transform to "1" in this case?

It shouldn't be hard to restrict the allowed types via the config.

Maybe it does not even matter. Does it make a difference if the storage plugin or the type plugin says that a type is not allowed? It would be good if the storage tutorial would also say something about types (@sanssecours ?)

@kodebach

This comment has been minimized.

Copy link
Contributor

kodebach commented Apr 2, 2019

Does it make a difference if the storage plugin or the type plugin says that a type is not allowed?

Not that I know of. It would probably be even better to raise the error in the storage plugin, because then the user sees that the problem is the use of yajl not the use of type.

Why does it not transform to "1" in this case?

The normalization/restore procedure can be described by the following cases:

  • Case 1: The Key existed in kdbGet and is unchanged between kdbGet and kdbSet
    This is the easiest and most obvious case. The value is normalized in kdbGet and the original value is restored in kdbSet, so that the underlying storage file remains unchanged (wrt the key in question)
  • Case 2: The Key didn't exist in kdbGet
    Here we normalize the value to verify the type and then restore it immediately.
  • Case 3: The Key existed in kdbGet, but its value was changed between kdbGet and kdbSet
    This is essential the same as Case 2. keySetString removes the origvalue metadata, so for the type plugin this kind of key didn't exist in kdbGet.

There is one special case. I already added functionality that might be used here (I forgot to add it to infos/metadata):

The accepted values can also be overridden on a per-key-basis. Simply add the metakey `check/boolean/true` and `check/boolean/false` to set
the accepted true and false values. Only a single true/false value can be chosen. This is intended for use cases, where normally you prefer
to use only e.g. `1`, `0` and `true`, `false`, but what to override that for a key where e.g. `enabled` and `disabled` make more sense
contextually (e.g. for something like `/log/debug`). Because of this intention restoring also works differently, when `check/boolean/true`
and `check/boolean/false` are used. In this case we will always restore to the chosen override values.

If yajl injects the metadata check/boolean/true = true and check/boolean/false = false for all boolean keys, all the normalization and restoring should work as intended. The type plugin will then accept the values true, 1, false and 0 for boolean keys (in get and set), but it will always pass true or false to the setstorage plugin. The yajl plugin should still return 0/1 in get and it should accept true/false as well as 0/1 in set, so that it works with or without the type plugin.

If we choose to go down this way however, we have to document it very well, because mounting the type plugin will no longer allow to use different values for boolean keys in kdb set, because yajl secretly overrides any configuration given by the user.

@markus2330

This comment has been minimized.

Copy link
Contributor Author

markus2330 commented Apr 3, 2019

Not that I know of. It would probably be even better to raise the error in the storage plugin, because then the user sees that the problem is the use of yajl not the use of type.

Exactly.

The normalization/restore procedure can be described by the following cases
[...] (I forgot to add it to infos/metadata)

Thank you for the detailed explanation. Can you add this to our documentation please?

will no longer allow to use different values for boolean keys in kdb set

With "config/needs" yajl can ensure that type is mounted using "check/boolean/true = true and check/boolean/false = false".

So should I change the implementation hint?

@kodebach

This comment has been minimized.

Copy link
Contributor

kodebach commented Apr 3, 2019

With "config/needs" yajl can ensure that type is mounted using "check/boolean/true = true and check/boolean/false = false".

You misunderstood, right now check/boolean/true and check/boolean/false has to be set as metadata on an individual key, not in the config of type. General support within the config would have to be added (easy enough).

@markus2330

This comment has been minimized.

Copy link
Contributor Author

markus2330 commented Apr 3, 2019

Ok. No, then leave it as is. It makes more sense to implement everything in the yajl plugin.
I added as implementation hint:

The yajl plugin needs to:

  • render Elektra's "0" and "1" values to JSON's false and true.
  • fail with type errors if non-supported types are found

@sanssecours can you add this info to the storage plugin tutorial?

@kodebach

This comment has been minimized.

Copy link
Contributor

kodebach commented Apr 3, 2019

yajl also has to add the metadata check/boolean/true = true and check/boolean/false = false to each key with type = boolean. Otherwise the problem for kdb set /some/key on mentioned above will occur.

@markus2330

This comment has been minimized.

Copy link
Contributor Author

markus2330 commented Apr 3, 2019

yajl also has to add the metadata

Is this also needed if yajl would always give you only "0" and "1" for booleans?

@kodebach

This comment has been minimized.

Copy link
Contributor

kodebach commented Apr 3, 2019

Yes, because the problem occurs, when the user changes the key value after kdbGet. yajl has no influence on that.

But nevermind that, we need changes to the type plugin anyway, because if the user adds a new key, the metadata will not be present and the solution won't work.

@markus2330

This comment has been minimized.

Copy link
Contributor Author

markus2330 commented Apr 3, 2019

So we need the type plugin to be available twice in the kdbSet path so that the normalization will work properly? Can you create an issue?

@kodebach kodebach referenced this issue Apr 3, 2019

Merged

type: more boolean config #2582

9 of 9 tasks complete
@kodebach

This comment has been minimized.

Copy link
Contributor

kodebach commented Apr 3, 2019

No. With #2582 yajl (or the user) just needs to make sure that the config for type either contains

  • no booleans array and boolean/restore = #1
    or
  • has a booleans array which contains "true" and "false" at position #X and boolean/restore = #X
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.