Libelektra: kdb import broken in spec namespace

[kodebach]

Steps to Reproduce the Problem

echo -e "[test]\n default = value" | kdb import spec/tests/import ni

echo -e "[test]\n default = value" | kdb import spec/tests/import ni

Expected Result

The second line doesn't change anything about the KDB (except file timestamp, etc.)

Actual Result

Something inside of kdb import (most likely the spec plugin, as this only occurs with the spec namespace) butchers the key name and the following error is shown:

The command bin/kdb import terminated unsuccessfully with the info:
the supplied key spec/tests/importt is not below the old parent spec/tests/import
Please report the issue at https://issues.libelektra.org/

Note: spec/tests/import/test somehow became spec/tests/importt.

Further Information

• kdb import -v doesn't provide any additional information.
• The parent key or added keys don't matter as long as the spec namespace is used.
• It is always the / and the first 3 characters after the parent key name that go missing.

[markus2330]

Thank you for reporting this problem!

[e01306821]

I'd like to take this issue

[markus2330]

Thank you for taking this issue!

[e01306821]

It seems that at the second call, "doGlobbing" inside "elektraSpecGet" in "src/plugins/spec/spec.c" returns just the key /tests/import/tests (without "spec" at the beginning), which causes the method "elektraSpecGet" to return this key without the spec-prefix.
Because of this, at merging the results in "src/tools/kdb/import.cpp", the calculation of newPath inside "src/libs/tools/src/helper/keyhelper.cpp" is wrong, the missing spec-prefix is causing the 4 missing characters of the key.

Now I'm not sure where the error is. One solution might be to just add "spec" at the beginning of the "originalKeys" (import.cpp) if kdb import wants to import to spec. Or the behavior of "elektraSpecGet" is not correct.

[markus2330]

@e01306821 very good debugging. The code you had to look into is not the easiest to understand.

elektraSpecGet is allowed to add a key without namespace (it represents the default value) but discarding the spec key does not sound right.

@kodebach did you also test with other namespaces? Is this problem specific to the spec namespace?

[kodebach]

As far as I can tell, this only happens with the spec namespace, which is why I suspected the spec plugin. But if the spec plugin behaves correctly, it is probably a merge issue and might go away with the new merging implementation.

[markus2330]

It seems like the spec plugin does not behave correctly (as described in the debugging session above). It seems to remove the spec key.

[e01306821]

Is it possible that the problem lies in how ksAppend works?
After doGlobbing two KeySets get appended to the "returned"-KeySet in the following order:
specKS: spec/tests/import/test
ks: /tests/import/test

ksAppend (returned, specKS);
ksAppend (returned, ks);

According to the documentation, ksAppend deletes a Key in its first-argument-KeySet if it is also contained in the second-argument-KeySet. It seems that our wanted key "spec/tests/import/test" gets deleted because it is considered the same as "/tests/import/test".
Is this an expected behavior?

[kodebach]

Hm, in that case it comes done to: Should a cascading key (starting with /) be allowed to be added to a KeySet at all?

Could you please try, whether reversing the order of the ksAppend calls solves the problem? If it does, I think we can accept that for a solution (at least temporarily).

[e01306821]

Thanks for your reply!
Unfortunately if executed the other way around it does not change the returned-KeySet at all.
After first append: returned = /tests/import/test
After second append: returned = /tests/import/test

My current solution now is to filter ks for keys starting with "/" and only add those keys to "returned" which do not start with this character. I'll open a pull request, please add comments if this solution is not acceptable.

[markus2330]

Hm, in that case it comes done to: Should a cascading key (starting with /) be allowed to be added to a KeySet at all?

Yes, these keys are used as default keys. (So that ksLookup does not need to append something to the KeySet, see also #972)

I'll open a pull request

Thank you!

[markus2330]

Could you please try, whether reversing the order of the ksAppend calls solves the problem?

From a quick code review ksAppendKey seems to handle cascading keys correctly (it uses ksSearchInternal which does not give cascading keys any semantic). But it would not harm to add a test case:

    key1 = keyNew ("user/key", KEY_END);
    key2 = keyNew ("/key", KEY_END);
    ksAppendKey (ks1, key1);
    ksAppendKey (ks1, key2);
    // check here if ks1 contains all keys

The tests could be added in tests/abi/testabi_key.c

[markus2330]

For me it works now flawlessly. The problem does not occur anymore.

Unfortunately, we did not add a test against regressions.