kdb set with validation

[markus2330]

kdb set: always do cascading set (for validation)

Current behavior

kdb set user:/test value

does not validate.

Expected behavior

kdb set user:/test value should validate (except if -f is used)

[kodebach]

Other problems with kdb set are:

• internal/** metadata is not removed
• origvalue metadata is not removed (breaks all future kdb get, until it is removed)

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

[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]

@atmaxinger can you check if this is still open?

[atmaxinger]

Okay so I tested this issue against the current master

The setup:

kdb mount `pwd`/i1164/spec.ni spec:/i1164 ni
kdb meta-set spec:/i1164/port type unsigned_short
kdb meta-set spec:/i1164/port check/type unsigned_short
kdb set user:/i1164/port 123

✔ kdb meta-get user:/i1164/port check/type correctly returns unsigned_short ✔ kdb meta-get /i1164/port check/type correctly returns unsigned_short

❌ kdb set user:/i1164/port abc --> does not validate, sets the value as string ❌ kdb set -N user /i1164/port abc --> invalid option -- 'N'

[markus2330]

Thank you for reproducing! That -N doesn't work anymore is fine (it was redundant).

But kdb set user:/i1164/port abc should do validation, this is very problematic, as this is the main way how values should be changed.

I updated the top-post.

[kodebach]

The setup: [...]

If that's the full setup, it cannot work. Neither the type plugin nor the any plugin for meta:/port are mounted. You either need an extra spec-mount or a more direct mount command with extra plugins.

EDIT: But looking at the code in src/tools/kdb/set.cpp, there is no attempt to assure a cascading parent key AFAICT, so adding the plugins probably won't change anything.

[atmaxinger]

If that's the full setup, it cannot work. Neither the type plugin nor the any plugin for meta:/port are mounted. You either need an extra spec-mount or a more direct mount command with extra plugins.

Yes that's the full setup. TBH I thougt it would need some plugins, but I followed this guide and there was no mentioning of extra mounting the namespace with the plugins. If this is the case, the documentation should be revised to include this information.

[kodebach]

Stopped before or you skipped the essential spec-mount step. This infers the required plugins from the meta:/ keys and creates appropriate mountpoints.

[atmaxinger]

Yes I skipped it, as the description

This specification mount makes sure that the paths where the concrete configuration should be (app.ni) are ready to fulfill our specification (spec.ni).

sounded like it was only necessary when mounting the configuration from a file. As Elektra is also usable without mounting specific files (i.e. elektrified applications), I didn't think this was necessary.

[kodebach]

It's not about whether or not you are using a file. It's about the existence of a mountpoint. If you don't call kdb mount (indirectly also happens via kdb spec-mount), your config will be saved in the default mountpoints (assuming no other more specific mountpoint exists from something else). But plugins only work, if they are explicitly added to a mountpoint. The default moutpoints only contain the bare minimum (storage and resolver plugins).

[atmaxinger]

Yes I understand that now, but we should really specify this clearly in the guide.

[markus2330]

@atmaxinger Can you please clarify what exactly should be specified in which guide?

[atmaxinger]

@markus2330 In the How to Write a Specification in Elektra that it is mandatory to call kdb spec-mount for the check plugins to work. The way this is worded now makes it seem that you only need to do it if the configuration itself is mounted.

[kodebach]

I agree, the sentence

This specification mount makes sure that the paths where the concrete configuration should be (app.ni) are ready to fulfill our specification (spec.ni).

could be worded better. It is very much clear what "ready to fulfill a specification" means.

Maybe there should just be a quick overview of the required steps at the top. That could also be helpful for people who already read the tutorial once, but need a refresher.

[markus2330]

@atmaxinger I don't find the word mandatory on that page? Can you please create a separate issue (or PR) describing more precisely how to improve the documentation? It is very valuable input but we need to fix it precisely, otherwise we cause more damage than what it helps. (It is never wrong to do a spec-mount, this is why the tutorial is insisting on that spec-mount is called. To explain in which cases it is needed would be too much for such a tutorial.)

Maybe there should just be a quick overview of the required steps at the top.

I think that in most of our problems in the docu additional overviews/links/text won't help (except for, e.g., beginner devs, there we probably really missing something like that). We need to fix vague/misleading sentences.

[atmaxinger]

I don't find the word mandatory on that page

Yes, and that's the problem. @kodebach pointed out that kdb spec-mount must be called for it to work. However, I interpreted the current wording in the guide that you only need to call it if the configuration is mounted as an external file.

It is never wrong to do a spec-mount

Then this should be stated explicitly.

[markus2330]

As said, please in a separate issue. Deeply hidden in unrelated discussions it does not help.

[kodebach]

I think that in most of our problems in the docu additional overviews/links/text won't help [...] We need to fix vague/misleading sentences.

Just a quick comment on that. I think it's also a structural problem. See #4436 (caution lots of text)

[flo91]

Aside from the discussion about the tutorial, is the main issue still relevant? @markus2330 Maybe a good issue for the FLOSS course (triage).

[markus2330]

The question is if we should fix this in old code to be replaced by @hannes99 C implementation later?

[hannes99]

hey @kodebach, a quick question regarding this.

EDIT: But looking at the code in src/tools/kdb/set.cpp, there is no attempt to assure a cascading parent key AFAICT, so adding the plugins probably won't change anything.

I'm not sure what you mean by

attempt to assure a cascading parent key

What would be needed so the plugin(if present) is used for validation? Is a ksLookup and keySetString enough? If not, what else is necessary to make this work?

[kodebach]

You just need to make sure to only use a cascading keys as the parent key that is given to kdbGet and kdbSet.

I assume, you simply use the key given to kdb set in the CLI as the parent key. In that case, you can do something like:

// Note: very rough sketch
Key * keyFromCli;

Key * parentKey = keyDup (keyFromCli);
keySetNamespace (keyFromCli, KEY_NS_CASCADING);

kdbGet (kdb, ks, parentKey);

keySetString (ksLookup (ks, keyFromCli, 0), valueFromCli);

kdbSet (kdb, ks, parentKey);

[kodebach]

To explain this a little bit, if the parent key is not cascading, then only mountpoints with the same namespace will be considered, since other mountpoints can never contain data below the parent key. However, that means in most cases spec:/ keys will not be loaded. Therefore, the spec plugin will not copy metadata, which in turn means that most validation plugins won't do anything, since normally they rely on metadata copied from spec:.

[hannes99]

ohh I see, thanks for the explanation!

[github-actions[bot]]

I mark this 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 by writing a message here or create a new issue with the remainder of this issue. Thank you for your contributions :sparkling_heart:

[github-actions[bot]]

I closed this 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 :sparkling_heart: