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: __dirdata gets added too often #2477

Open
markus2330 opened this Issue Mar 10, 2019 · 6 comments

Comments

Projects
None yet
2 participants
@markus2330
Copy link
Contributor

markus2330 commented Mar 10, 2019

Steps to Reproduce the Problem

sudo kdb mount `pwd`/file.json system/tests/json json
kdb set system/tests/json/some/test "some value"
kdb set system/tests/json/some/array/#0 val1
kdb set system/tests/json/some/array/#1 val2

Expected Result

Following JSON file:

{
    "some": {
        "array": [
            "val1",
            "val2"
        ],
        "test": "some value"
    }
}

Actual Result

Following JSON file:

{
    "___dirdata": "",
    "some": {
        "___dirdata": "",
        "array": [
            "___dirdata: ",
            "val1",
            "val2"
        ],
        "test": "some value"
    }
}

System Information

  • Elektra Version: master

@markus2330 markus2330 added the bug label Mar 10, 2019

@markus2330 markus2330 changed the title __dirdata gets added too often directoryvalue: __dirdata gets added too often Mar 10, 2019

@sanssecours

This comment has been minimized.

Copy link
Member

sanssecours commented Mar 10, 2019

I do not think that is a problem of the Directory Value plugin, but rather of YAJL. For example, YAML CPP handles this situation correctly:

kdb mount file.yaml system/tests/json yamlcpp
kdb set system/tests/json/some/test "some value"
kdb set system/tests/json/some/array/#0 val1
kdb set system/tests/json/some/array/#1 val2
kdb file system/tests/json | xargs cat
#> some:
#>   array:
#>     - val1
#>     - val2
#>   test: some value

. The problem is that the YJAIL adds additional keys to the key set. For example, if I remove directoryvalue from infos/needs in the YAJL plugin, then the commands

kdb mount file.json system/tests/json yajl
kdb set system/tests/json/some/test "some value"
kdb set system/tests/json/some/array/#0 val1
kdb set system/tests/json/some/array/#1 val2

produce a key set with 6 keys:

kdb ls system/tests/json
#> system/tests/json
#> system/tests/json/some
#> system/tests/json/some/array
#> system/tests/json/some/array/#0
#> system/tests/json/some/array/#1
#> system/tests/json/some/test

. The proper number would be either three or four (if the array parent key stores data or metadata, such as the name of the last element):

# The following output should include `system/tests/json/some/array`, 
# if the plugin stores data in this key.
kdb ls system/tests/json
#> system/tests/json/some/array
#> system/tests/json/some/array/#0
#> system/tests/json/some/array/#1
#> system/tests/json/some/test

. If the storage plugin adds metadata to the array key, then it should probably also check if the array parent only stores the name of the last element, when writing the data back. In this special case, it does not need to write the data stored in array/___dirdata back to the file. For example, YAML CPP does this, while YAML Smith (used by Yan LR) does not:

kdb mount file.yaml system/tests/json yanlr
kdb set system/tests/json/some/test "some value"
kdb set system/tests/json/some/array/#0 val1
kdb set system/tests/json/some/array/#1 val2
kdb file system/tests/json | xargs cat
#> some:
#>   array:
#>     -
#>       "___dirdata: "
#>     -
#>       "val1"
#>     -
#>       "val2"
#> some:
#>   test:
#>     "some value"
kdb ls system/tests/json
#> system/tests/json/some/array
#> system/tests/json/some/array/#0
#> system/tests/json/some/array/#1
#> system/tests/json/some/test

. Anyway, as you can see, Yan LR does not add additional keys. Therefore the configuration file only contains a single unnecessary ___dirdata: entry.

@markus2330 markus2330 changed the title directoryvalue: __dirdata gets added too often yajl: __dirdata gets added too often Mar 10, 2019

@markus2330

This comment has been minimized.

Copy link
Contributor Author

markus2330 commented Mar 10, 2019

I do not think that is a problem of the Directory Value plugin, but rather of YAJL.

Thank you, I changed the title.

I was thinking that simply not writing empty __dirdata would solve the issue (also for yanlr). Would this approach have side effects?

@sanssecours

This comment has been minimized.

Copy link
Member

sanssecours commented Mar 10, 2019

I was thinking that simply not writing empty __dirdata would solve the issue (also for yanlr). Would this approach have side effects?

If the Directory Value plugin does not create the entries for empty directory keys, then we are not able to distinguish between a key set that contains certain keys without a value and one that does not.

I really think the creation of additional keys is a bug in the YAJL plugin. For example, dump also does not create any additional keys:

kdb mount file.ecf system/tests/json dump
kdb set system/tests/json/some/test "some value"
kdb set system/tests/json/some/array/#0 val1
kdb set system/tests/json/some/array/#1 val2
kdb ls system/tests/json
#> system/tests/json/some/array/#0
#> system/tests/json/some/array/#1
#> system/tests/json/some/test

.

…also for yanlr…

The minor problem of the Yan LR plugin should be solved in the YAML Smith plugin in my opinion, since this plugin should know how to deal with metadata in YAML files.

@markus2330

This comment has been minimized.

Copy link
Contributor Author

markus2330 commented Mar 10, 2019

Yes, I fully agree, storage plugins should not produce any additional keys which were not set.

Can you document these guidelines for storage plugins? Then we could have a infos/status #666 for storage plugins that confirm to that semantics.

@markus2330

This comment has been minimized.

Copy link
Contributor Author

markus2330 commented Mar 10, 2019

(I think at the moment INI in its default configuration also violates against this principle)

@markus2330 markus2330 referenced this issue Mar 10, 2019

Open

add to infos/status #666

4 of 23 tasks complete
@sanssecours

This comment has been minimized.

Copy link
Member

sanssecours commented Mar 11, 2019

Can you document these guidelines for storage plugins?

I added a text about the proper behavior in pull request #2479.

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.