Libelektra: spec: does not remove copied metadata before writing config

[markus2330]

I think this is a show-stopper for using in LCDproc or machinekit (who want INI as first step for smooth migration)

Steps to Reproduce the Problem

• mount a specification
• mount an INI file
• do a kdb set

kdb mount wrong-copy.ini /tests ini
kdb setmeta /tests/some-key some-meta some-value
kdb set /tests/some-key x

Expected Result

To have a file

some-key = x

Actual Result

#@META some-meta = some-value
some-key = x

System Information

• Elektra Version: master

Implementation Hints

The spec plugin does not have the positioning to remove its meta-data before writing to the storage. See discussion below: the KeySet passed back from kdbSet must not be altered. Only the meta-data within split is allowed to be removed.

So the easiest way is to call the spec plugin for every part in the split plugin separately. (As foreach plugin.)

[kodebach]

This issue is not as easy as it seems at first. We could of course just store what metadata was added (e.g. in a internal/spec/added/# meta-array), but there is a problem.

What if we added e.g. type = "string" in elektraSpecGet and then someone calls keySetMeta(k, "type", "string"). If we now delete the metakey in elektraSpecSet, the user will wonder why their keySetMeta call didn't work. (Note: I explicitly used "string" both times. If the value was the different, we could detected the change that way)

IMO the best solution is to introduce a new bitfield (of type keyswitch_t) in the Key struct. This bitfield would store in which ways the key has been changed (name, value, metadata, etc). The flags would be reset at the very end of kdbGet and recorded until the beginning of kdbSet. At that point the flags are locked, so that only changes from the user application, not from plugins are recorded.

This bitfield may also be useful for diffs and merging, as well as some storage plugins, which are able to change only part of their backing storage (e.g. databases).

[markus2330]

What if we added e.g. type = "string" in elektraSpecGet and then someone calls keySetMeta(k, "type", "string"). If we now delete the metakey in elektraSpecSet, the user will wonder why their keySetMeta call didn't work. (Note: I explicitly used "string" both times. If the value was the different, we could detected the change that way)

I think this could also be wanted (to not intermix specifications from different sources: if some meta data is in spec, the same meta data should not be in any other namespaces).

But even if we want this mixing (we would need to define the behavior for this, though) you can implement it with the features we already have:

1. spec copies the metadata (with keyCopyMeta)
2. before removing, spec checks if it is the metadata that it copied (by checking if the pointer of the metadata (using keyGetMeta) is the same as the one from spec)

[kodebach]

can implement it with the features we already have:

~I did implement the procedure you suggested in #2736. However, it doesn't work 100%. If the spec key was modified (more specifically its metadata) the check will fail to detect which metadata has to be removed. I noted this flaw in the new README.~

EDIT: I just noticed with the current setup we cannot remove the metadata. spec is a global plugin and therefore called before other presetstorage plugins. This means other presetstorage plugins won't run their validation, since we removed the metadata.

[markus2330]

Iirc the idea from @tom-wa was to add the metadata in the beginning of the set path, and remove it shortly before the end.

[kodebach]

to add the metadata in the beginning of the set path, and remove it shortly before the end.

Of course that is the only real solution. But that would require mounting the spec plugin twice on the set side. While that is possible, it is very awkward to do right now. At least we should provide a way for plugins to find out in which position they are currently being called first.

[kodebach]

spec is a global plugin and therefore called before other presetstorage plugins

The corresponding problem in kdbGet (i.e. spec is called AFTER other postgetstorage plugins) is even worse. spec MUST NOT remove copied metadata, otherwise the following kdbGet will not work as expected.

An example:

sudo kdb mount demo.ini user/tests/demo/abc ni type
sudo kdb setmeta spec/tests/demo/abc type boolean

# the following works as expected
kdb set -N user /tests/demo/abc true
kdb get user/tests/demo/abc
#> 1

# reset
kdb rm user/tests/demo/abc

# the spec plugin only runs with cascading parent keys
# so this demonstrate what would happen, if spec removed the metadata
kdb set user/tests/demo/abc true
kdb get user/tests/demo/abc
#> true

kdb lsmeta user/tests/demo/abc
#>

Right now the best (maybe only) way to correctly set a key is this sequence:

kdb set -N user /my/key value
kdb rmmeta user/my/key origvalue
kdb lsmeta "user/my/key" | grep '^internal/' | xargs -L1 kdb rmmeta "user/my/key"

[markus2330]

See implementation hint above, here it is only about removing meta-data before writing to the storage (as very last step).

As kdbSet does a deep duplication for writing, this will not effect the KeySet a tool or application uses.

[kodebach]

See implementation hint above, here it is only about removing meta-data before writing to the storage (as very last step).

As kdbSet does a deep duplication for writing, this will not effect the KeySet a tool or application uses.

I have no idea what you want to tell me here....

I explained, why removing metadata before calling the setstorage plugin DOES NOT WORK with the way kdbGet works right now. This is very relevant to this issue, since a "fix" for this issue (i.e. if in kdbSet spec actually removed the metadata it copied during kdbGet) would break every application using the high-level API or otherwise relying on the type plugin for normalising key values.

[markus2330]

From the example above:

so this demonstrate what would happen, if spec removed the metadata

I do not think this demonstrates anything as the workflow of multiple kdbGet and kdbSet within applications cannot be demonstrated with the kdb command. spec obviously will only remove keys from non-spec keys. So after doing kdb get again, the meta-data will be again copied from spec.

But I think I understand what you mean.

I explained, why removing metadata before calling the setstorage plugin

The correct implementation will only remove metadata copied from spec to other keys within the split KeySet, not the KeySet which will be passed to the user. Furthermore, it will not remove the meta-data from the spec keys. So kdb get or the KeySet of the application (or high-level API) will not be effected by the removal: it will always get fresh meta-keys from spec.

The (hopefully) only difference will be that the meta-data does not end up in config files outside of spec.

using the high-level API or otherwise relying on the type plugin for normalising key values.

The type checker already checks (and normalizes) before writing out, so it still has the metadata. The high-level API and the application's KeySet also has the meta-data as we will only remove the meta-data from a duplicated KeySet which is never passed to the high-level API.

See source code: src/libs/elektra/split.c line 818. The correct implementation would only remove the meta-data from split->keysets[i], not from ks.

So as said above in implementation hint: The spec plugin does not have the positioning to remove its meta-data before writing to the storage.. Once we have the correct hook (with the right KeySet that is not passed back), the implementation should be trivial.

[kodebach]

I do not think this demonstrates anything as the workflow of multiple kdbGet and kdbSet within applications cannot be demonstrated with the kdb command.

Oh I see now... You misunderstood my example. Only the kdb get calls would be replaced with an application. The kdb set calls before that are setting the configuration for this application.

I didn't even consider calling kdbSet and kdbGet in the same application. In this case there are of course in memory solutions that circumvent the problem.

spec obviously will only remove keys from non-spec keys. So after doing kdb get again, the meta-data will be again copied from spec.

That was my original point. It should be like that, but removing metadata in kdbSet will not achieve this.

spec is a global postgetstorage plugin and as such is called after non-global postgetstorage plugins (like type). Therefore: yes, spec will copy the metadata on kdbGet calls (if the parent key is cascading), but it will only do so after the validation/normalisation plugins (that need the metadata) are called.

---

So as said above in implementation hint: The spec plugin does not have the positioning to remove its meta-data before writing to the storage.

That is true. It is also true, that no correct position exists right now and that creating a correct position will be very involved.

Simply calling global postgetstorage plugins before non-global ones would work, as long as only spec is involved (other plugins might have other requirements). However, it would also break the symmetry between kdbGet and kdbSet, which says that the presetstorage plugins should be called in the reverse order of postgetstorage. By that rule global presetstorage plugins should then be called after non-global ones. That would break the kdbSet path, since then spec would be called after the various validation plugins.

I don't have a good solution for this problem. All I can say is that the current plugin position system might not be good enough. A more granular (and much more complex) system for ordering plugins by their tasks (e.g. normalisation before validation, creating metadata before using metadata, etc) might be required.

[markus2330]

That is true. It is also true, that no correct position exists right now and that creating a correct position will be very involved.

The split keysets need to be merged and split again or even better: we call the cleanup plugin on every split separately (what should be no problem for the cleanup. I updated the implementation hint above).

A more granular (and much more complex) system for ordering plugins by their tasks (e.g. normalisation before validation, creating metadata before using metadata, etc) might be required.

For 1.0 we will continue to merge the plugins. With less plugins there are less opportunities of unwanted feature interactions like #404. Please report all concrete feature interaction problems you know (with the concrete plugins).

e.g. normalisation before validation

Yes, this is unfortunately a completely open problem @Piankero did not work on. Does type, rgbcolor and unit now play together? Please report bugs if (and how) it does not.

With merging all type and normalization features to "type" it should be much better now. Now we "only" need to fix the interaction of "type" and "spec". Concrete reproducible bugs would help here, too.

Now with lcdproc is the first time that we can test with a real-world specification. So obviously we will find new problems.

creating metadata before using metadata

This should be enforced by the spec plugin anyway. Other plugins should (for now) not create metadata that is for others. For now we need to simplify it to be able to release 1.0.

Most likely we will need to remove further plugins that do not play well with the current architecture.

[markus2330]

@mpranj can you keep this in mind when you look at the positioning? afaik @tom-wa said it is "only" about executing spec one more time.

[kodebach]

spec also needs to be updated so that it actually removes the metadata on the second elektraSpecSet run (which must be immediately before the setstorage plugin runs). Also, I'll point out again that we should introduce some standard way for plugins to find out in which position they are begin called (without the need for each plugin to internally count positions).

---

Sidenote: I think certain metakeys (like type or description) should never be removed (even if they are unchanged to the spec), since they may be used by the storage plugin.

[mpranj]

@kodebach thanks for the info. The design doc afair states that the position should be given to the global plugin via parentKey, so the position should be easy to determine. I'll ping you if I get stuck with spec as I don't know anything about it.

[stale[bot]]

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 :sparkling_heart:

[markus2330]

If we really need this is dependent on what we want to do with spec (see #3549)