[new-backend] spec hook

[atmaxinger]

This is still a work in progress:

• remove meta data
• settings for enabling/disabling spec hook - will be discussed in Enable/disable hooks. #4499
• clarify wether copy should behave differently between get and set - will be discussed in [new-backend] spec: reimplement to completely comply with hooks #4500
• clarify handling of default:/ keys with regards to backend - see also [new-backend] spec hook - default:/ backend #4493

Basics

• Short descriptions of your changes are in the release notes
  (added as entry in doc/news/_preparation_next_release.md which
  contains _(my name)_)
  Please always add something to the release notes.
• Details of what you changed are in commit messages
  (first line should have module: short statement syntax)
• References to issues, e.g. close #X, are in the commit messages.
• The buildservers are happy. If not, fix in this order:
  • add a line in doc/news/_preparation_next_release.md
  • reformat the code with scripts/dev/reformat-all
  • make all unit tests pass
  • fix all memleaks
• The PR is rebased with current master.

Checklist

• I added unit tests for my code
• I fully described what my PR does in the documentation
  (not in the PR description)
• I fixed all affected documentation
• I added code comments, logging, and assertions as appropriate (see Coding Guidelines)
• I updated all meta data (e.g. README.md of plugins and METADATA.ini)
• I mentioned every code not directly written by me in reuse syntax

Review

• Documentation is introductory, concise, good to read and describes everything what the PR does
• Examples are well chosen and understandable
• Code is conforming to our Coding Guidelines
• APIs are conforming to our Design Guidelines
• Code is consistent to our Design Decisions

Labels

• Add the "work in progress" label if you do not want the PR to be reviewed yet.
• Add the "ready to merge" label if the basics are fulfilled and no further pushes are planned by you.

[atmaxinger]

Should we test all of them for both the get and set operations? Should there be differences between the operations?
For now, e.g. the set operation does not create default:/ keys.

[kodebach]

The copy should behave the same in kdbGet and kdbSet that's why I used the same name.

That said, the current set from spec is a bit broken.

To make validation for newly added keys during kdbSet work properly, we need to copy meta:/ keys from spec:/ to other namespaces at the start of kdbSet (before prestorage). This should behave the same as in kdbGet and the new meta:/ keys should be visible outside kdbSet I think (see #4484 (comment)).

However, we don't want to store the copied meta:/ keys in the other namespace, so we need to remove them before the storage phase. To do this we should simply check for every metakey of every key in a namespace other than spec:/ whether it (the metakey) is pointer-equal to the corresponding metakey on the equivalent spec:/ key. So something like:

KeySet * specMeta = keyMeta (specKey);
KeySet * meta = keyMeta (key);

for (elektraCursor it = 0; i < ksGetSize (specMeta); i++)
{
    Key * m = ksAtCursor (specMeta, it);
    if (ksLookup (meta, m, 0) == m)
    {
        keySetMeta (key, keyName (m), NULL);
    }
}

[atmaxinger]

The differences between a set and get operation for the spec plugin were copied from the existing implementation:

• There can be different settings for the conflict handling
  • This can probably be removed
• neither assign/condition nor default are processed in set
  • We can probably also execute this on set
• set operation sets internal/spec/remove on array
  • remove or also do it in get?
• set removes internal/spec/array/validated
  • remove or also do it in get?
• set does this thing here, I'm not sure what it is
  • ??? If its not an array and it should not be removed it appends the specKey to the returned KeySet ???

if (!isKdbGet)
{
	keySetMeta (specKey, "internal/spec/array/validated", NULL);

	if (keyGetMeta (specKey, "internal/spec/array") == NULL && keyGetMeta (specKey, "internal/spec/remove") == NULL)
	{
		ksAppendKey (returned, specKey);
	}
}

[kodebach]

There can be different settings for the conflict handling

• This can probably be removed

Yes, all the conflict handling settings should be removed entirely. During get all conflicts should be warnings during set they should be errors (unless @markus2330 thinks some conflicts should never be errors). That part could be handled from the libelektra-kdb side with the "error blocking trick", so the spec would be the same.

neither assign/condition nor default are processed in set

Not sure about assign/condition and whether we actually want to keep that (@markus2330 ?), but default should definitely be processed in set. See also #4484 (comment)

• set operation sets internal/spec/remove on array
• set removes internal/spec/array/validated
• set does this thing here, I'm not sure what it is

All of the stuff related to array handling is very complicated and weird. Part of the weirdness comes from the fact that on master the spec plugin is only called once during kdbSet and somehow there should still be validation during kdbSet, but also spec should cleanup the copied data (obviously not possible).

See also #2700 and #4061

---

However, please don't waste time on this. Like I said already: Just try to imitate what spec does on master. The existing tests obviously also expect this behaviour. Once that is done and merged, you can create another PR with the actual updates for spec (probably after new-backend is merged in master).

[kodebach]

Especially, the conflict handling should remain the same as on master for now. Changing this will probably break some tests and take more time than might expect.

[atmaxinger]

However, please don't waste time on this. [...] Once that is done and merged, you can create another PR with the actual updates for spec.

I agree, this is probably the correct method, so I can continue with the other hooks. Created issue #4500 to track this.

Please take a look at my comment below with the test cases. I think those still need to be resolved before we can merge it.

[markus2330]

It looks like we got someone who will work on the spec plugin! 🚀

So we should soon have a coordination meeting about spec-related topics.

[markus2330]

Not sure about assign/condition and whether we actually want to keep that (@markus2330 ?)

No, we can remove it. Defaults are a superior way. It should be possible via namespace spec to give default higher priority than others, giving something like an assignment via defaults.

[kodebach]

Haven't had time to look at this properly yet. But there are something that need to be changed about the spec plugin (see also #4495 (comment)). If you don't have the time right now, or don't want to do it right now. Just try to emulate what happens on master and leave some TODOs to point people in the direction of what still needs to be change in kdb.c.

---

The things that need to be changed in spec are:

• The copy & remove stuff for kdbSet described in [new-backend] spec hook #4495 (comment)
• spec should always report errors/warnings for conflicts instead of the useless it does now.
• Instead of lots of ksDup/ksCut we should use ksFindHierarchy where possible.

[markus2330]

Great work. Elegant tests. I did not check functionality, though.

[atmaxinger]

New failing tests in this PR:

testshell_markdown_kdb_get:

libelektra/doc/help/kdb-get.md

Lines 80 to 89 in fac8496

	# To explain why a specific key was used (for cascading keys):
	kdb get -v /tests/get/examples/kdb-get/key
	#> got 2 keys
	#> searching spec:/tests/get/examples/kdb-get/key, found: <nothing>, options: KDB_O_CALLBACK
	#> searching proc:/tests/get/examples/kdb-get/key, found: <nothing>
	#> searching dir:/tests/get/examples/kdb-get/key, found: <nothing>
	#> searching user:/tests/get/examples/kdb-get/key, found: user:/tests/get/examples/kdb-get/key
	#> The resulting keyname is user:/tests/get/examples/kdb-get/key
	#> The resulting value size is 6
	#> myKey

kdb get -v /tests/get/examples/kdb-get/key should get 2 keys, but gets 3.

The keys are:

• spec:/tests/get/examples/kdb-get/anotherKey
• user:/tests/get/examples/kdb-get/key
• default:/tests/get/examples/kdb-get/anotherKey

@kodebach The only difference to previously is that the default:/ key is part of the resulting KeySet. Should I modify the test case so it expects 3 keys, or is it wrong that the default key is returned? After all, it is not the default key for the searched key (../key) but for another key (../anotherKey). But on the other hand, why would we include the spec:/ key but not the default:/ key ...

---

testshell_markdown_kdb_global_umount: Expects spec to be in the list of global plugins, which it isn't anymore. Easy fix, just remove it from the expected values. But as the global mount/global unmount stuff should actually go away with hooks, maybe delete those tests entirely?

---

testscr_check_kdb_internal_check: fails with the error message error: check of plugin spec with args '' failed. This seems to somehow test all plugins via execution of kdb plugin-check $PLUGIN_NAME. This fails for the spec plugin with the following message:

There are 1 warnings for this plugin
For high quality plugins there should be no warning
Warning #0: no placements information found

This seems about right, as spec does not have any placements anymore. Should we just ignore the spec plugin for this test?

[atmaxinger]

I don't think we need to solve this before merging this PR. Can be added later at any time.

[kodebach]

Agreed.

[kodebach]

testshell_markdown_kdb_get

If the issue is just the #> got 2 keys line then change it to whatever gets printed now. AFAIK that line simply prints the size of the keyset returned by kdbGet. That's not really something we make any guarantees about, so we can just change the test whenever the size of the keyset changes.

testshell_markdown_kdb_global_umount

Just remove the test.

testscr_check_kdb_internal_check

For now I think excluding spec and probably gopts from the test is the best solution. For the future we might want to introduce a infos/placements = hook to indicate the plugin is used for a kdb hook. But adding the probably is a good chunk of work, so just exclude the known hook plugins from these checks.

[kodebach]

Great work, LGTM. If you take care of the 3 tests, we can merge this IMO.

[atmaxinger]

@markus2330 the tests are fixed now, you can merge this PR.

[markus2330]

@atmaxinger please don't self-merge. Probably nobody looked at the test fixes now. In this case (for merges to new-backend) it is not a big deal, however, as it gets reviewed again.