Skip to content
This repository has been archived by the owner on Oct 15, 2024. It is now read-only.

YAJL: Invalid JSON Sequence #1862

Closed
beku opened this issue Mar 26, 2018 · 16 comments
Closed

YAJL: Invalid JSON Sequence #1862

beku opened this issue Mar 26, 2018 · 16 comments

Comments

@beku
Copy link
Member

beku commented Mar 26, 2018

A key creation sequence involving a object followed by a array creates invalid json.

Steps to Reproduce the Problem

kdb set /org/freedesktop/openicc/one/key val
#> Using name user/org/freedesktop/openicc/one/key
#> Create a new key user/org/freedesktop/openicc/one/key with string "val"

kdb set /org/freedesktop/openicc/#1/key other
#> Using name user/org/freedesktop/openicc/#1/key
#> Create a new key user/org/freedesktop/openicc/#1/key with string "other"

kdb ls /org/freedesktop/openicc
#> The command kdb ls failed while accessing the key database with the info:
#> Sorry, the error (#77) occurred ;(
#> Description: Yajl parser error
#> Reason: parse error: after array element, I expect ',' or ']'
#>                          }             }         }     } } 
#>                     (right here) ------^
#>
#> Ingroup: plugin
#> Module: yajl
#> At: /tmp/libelektra/src/plugins/yajl/yajl_parse.c:381
#> Mountpoint: user/org/freedesktop/openicc
#> Configfile: ~/.config/color/settings/openicc.json

Expected Result

Return just an error for the array index on a object path.
Or use the #1 index to access a existing object, but do not create a array if a a object is already in place.

Actual Result

The below JSON contains a opening array '[' without closing ']'.

{
    "org": {
        "freedesktop": {
            "openicc": [
                {
                    "key": "other",
                    "key": "val"
                }
            }
        }
    }
}

System Information

  • Elektra Version: master
  • yajl-2.1
@beku beku added the bug label Mar 26, 2018
markus2330 pushed a commit that referenced this issue Mar 26, 2018
@markus2330
Copy link
Contributor

Thank you for reporting!

With proper validation of arrays, such situations should be gracefully rejected. I updated the docu and #1137 should fully resolve the problem for you.

@markus2330 markus2330 mentioned this issue Mar 26, 2018
10 tasks
@markus2330
Copy link
Contributor

markus2330 commented Mar 27, 2018

@beku Btw. did an end user run into this problem or did this occur during testing?

I fully agree that the error should be also caught without a spec. This array/object mix is also a problem for YAML. @sanssecours can you add an "array" plugin which fails on such a array/object mix?

As not-so-urgent features the plugin could also normalize arrays:

@beku
Copy link
Member Author

beku commented Mar 27, 2018

@markus2330 not an end user. However I find it useful to obtain the count at a certain level. It tells for instance how many devices are configured. kdb ls ... can not do that. Accessing objects over index is not different that with arrays. Objects can be seen as named array items. The oyjl tool works like this.

@markus2330
Copy link
Contributor

However I find it useful to obtain the count at a certain level. It tells for instance how many devices are configured. kdb ls ... can not do that.

Doesn't kdb ls --max-depth=1 /org/freedesktop/openicc/ do what you want? (Count array or map members?)

Accessing objects over index is not different that with arrays. Objects can be seen as named array items. The oyjl tool works like this.

What exactly does the oyjl tool do?

In Elektra we want exactly one unique key for every configuration setting. We could add syntactic sugar on top to refer to the "first" map entry with #0. But JSON does not define any order (only Elektra does), and the order is subject to be changed if an "object"/map gets additional members. So it seems to be quite dangerous to index by number if only a string is unambiguous?

@beku
Copy link
Member Author

beku commented Mar 27, 2018

Doesn't kdb ls --max-depth=1 /org/freedesktop/openicc/ do what you want? (Count array or map members?)

Indeed it works:

kdb ls --max-depth=1 /org/freedesktop/openicc/display/display_white_point_XYZ/
#> user/org/freedesktop/openicc/display/display_white_point_XYZ/#0
#> user/org/freedesktop/openicc/display/display_white_point_XYZ/#1
#> user/org/freedesktop/openicc/display/display_white_point_XYZ/#2

What exactly does the oyjl tool do?

It is only a low level JSON editor for scripting, API design and testing.

In Elektra we want exactly one unique key for every configuration setting. We could add syntactic sugar on top to refer to the "first" map entry with #0. But JSON does not define any order (only Elektra does), and the order is subject to be changed if an "object"/map gets additional members. So it seems to be quite dangerous to index by number if only a string is unambiguous?

Correct. However arrays are quite practical and can be used reasonably. JSON is a serialisation syntax. So I am still optimistic and guess most users will not shuffle keys or arrays.

@sanssecours
Copy link
Member

This array/object mix is also a problem for YAML.

It is not really a problem for the YAML CPP plugin, which just uses a map (a.k.a. object) if one of the keys does not fit the array “specification”, as I interpreted it. As I write this down I also realize that my interpretation of an array – a sequence of values – and the one Elektra seems to use – an array with possibly empty slots – differ 😢.

kdb mount default.yaml / yamlcpp
kdb set /org/freedesktop/openicc/one/key val
kdb file /org/freedesktop/openicc/one/key | xargs cat
#> org:
#>   freedesktop:
#>     openicc:
#>       one:
#>         key: val
kdb set /org/freedesktop/openicc/#1/key other
kdb file /org/freedesktop/openicc/one/key | xargs cat
#> org:
#>   freedesktop:
#>     openicc:
#>       1:
#>         key: other
#>       one:
#>         key: val
kdb rm -r /org

Below is another example of the current behavior of YAML CPP

kdb set user/array/#0 first
kdb file user | xargs cat
#> array:
#>   - first
kdb set user/array/#1 second
kdb file user | xargs cat
#> array:
#>   - first
#>   - second
kdb set user/array/#1337 hello
kdb file user | xargs cat
#> array:
#>   0: first
#>   1: second
#>   1337: hello

.

@sanssecours can you add an "array" plugin which fails on such a array/object mix?

I would assume so. However, if your question is actually about me wanting to write such a plugin my current answer is “no”.

Objects can be seen as named array items. The oyjl tool works like this.

Another tool that behaves similar would be the programming language Lua, where arrays are just tables (a.k.a. maps) with integer indizes.

@markus2330
Copy link
Contributor

Rejection of mixtures and normalization of array indexing seems to be the best way to get a unified behavior between plugins. Then the differences, we currently have between the plugins, cannot be triggered.

@markus2330
Copy link
Contributor

@sanssecours Is this problem now fixed because of the directoryvalue plugin?

@sanssecours sanssecours removed their assignment Oct 27, 2018
@sanssecours
Copy link
Member

Nope. I think this problem should be fixed inside YAJL and not in the Directory Value plugin.

@markus2330
Copy link
Contributor

I think several plugins have the problem that they do not support mixed arrays/objects. So a separate implementation (in a new plugin) would make sense.

How do you currently handle this problem in your YAML plugins?

@sanssecours
Copy link
Member

How do you currently handle this problem in your YAML plugins?

None of the YAML plugins handle this situation (in the set direction) properly. In my opinion the best solution to solve this problem is to just use a map containing the keys #1 and one.

"#1":
  key: other
one:
  key: val

. In the get direction this already works in YAML CPP, Yan LR, YAMBi, YAwn and YAy PEG:

kdb mount config.yaml user/tests/yaml yamlcpp
printf '"#1":\n'        >  `kdb file user/tests/yaml`
printf '  key: other\n' >> `kdb file user/tests/yaml`
printf 'one:\n'         >> `kdb file user/tests/yaml`
printf '  key: val\n'   >> `kdb file user/tests/yaml`
kdb ls user/tests/yaml
#> user/tests/yaml/#1/key
#> user/tests/yaml/one/key

. The same strategy should also work for YAJL. The JSON version of the YAML file above looks like this:

{
  "#1": {
    "key": "other"
  },
  "one": {
    "key": "other"
  }
}

. Unfortunately YAJL does not handle this situation in the get direction properly:

kdb mount config.json user/tests/yajl yajl
printf '{'                  2> /dev/null >  `kdb file user/tests/yajl` 
printf '  "#1": {'          2> /dev/null >> `kdb file user/tests/yajl` 
printf '    "key": "other"' 2> /dev/null >> `kdb file user/tests/yajl` 
printf '  },'               2> /dev/null >> `kdb file user/tests/yajl` 
printf '  "one": {'         2> /dev/null >> `kdb file user/tests/yajl` 
printf '    "key": "other"' 2> /dev/null >> `kdb file user/tests/yajl` 
printf '  }'                2> /dev/null >> `kdb file user/tests/yajl` 
printf '}'                  2> /dev/null >> `kdb file user/tests/yajl` 
kdb ls user/tests/yajl
#> user/tests/yajl
#> user/tests/yajl/#1
#> user/tests/yajl/#2
#> user/tests/yajl/#2/key
#> user/tests/yajl/#3
#> user/tests/yajl/one
#> user/tests/yajl/one/key

.

@markus2330
Copy link
Contributor

Thank you for the detailed answer! Why do you add "key"? Isn't this enough:

{
  "#1": "other", 
  "one": "val"
}

@sanssecours
Copy link
Member

Why do you add "key"?

I added key, because it was part of the initial issue description.

Isn't this enough: …

Yep, that simpler JSON object also shows how to handle the issue.

@sanssecours sanssecours changed the title invalid json sequence YAJL: Invalid JSON Sequence Mar 22, 2019
sanssecours added a commit to sanssecours/elektra that referenced this issue Mar 22, 2019
sanssecours added a commit to sanssecours/elektra that referenced this issue Mar 22, 2019
sanssecours added a commit to sanssecours/elektra that referenced this issue Mar 23, 2019
sanssecours added a commit to sanssecours/elektra that referenced this issue Mar 25, 2019
sanssecours added a commit to sanssecours/elektra that referenced this issue Mar 26, 2019
sanssecours added a commit to sanssecours/elektra that referenced this issue Mar 26, 2019
sanssecours added a commit to sanssecours/elektra that referenced this issue Mar 26, 2019
sanssecours added a commit to sanssecours/elektra that referenced this issue Mar 28, 2019
@beku
Copy link
Member Author

beku commented Apr 30, 2019

Why do you add "key"?

I added key, because it was part of the initial issue description.

Oh, the sequence was only a example. No need to name a key "key" from my side

{
  "KEY_abc": "VALUE_def",
  "my_entry": "a_value"
}

should be enough :-D

@stale
Copy link

stale bot commented May 5, 2020

I mark this issue stale as it did not have any activity for one year. I'll close it in two weeks if no further activity occurs. If you want it to be alive again, ping the issue by writing a message here or create a new issue with the remainder of this issue.
Thank you for your contributions 💖

@stale stale bot added the stale label May 5, 2020
@stale
Copy link

stale bot commented May 19, 2020

I closed this issue now because it has been inactive for more than one year. If I closed it by mistake, please do not hesitate to reopen it or create a new issue with the remainder of this issue.
Thank you for your contributions 💖

@stale stale bot closed this as completed May 19, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants