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

type: make default booleans work #2723

Closed
markus2330 opened this issue May 26, 2019 · 14 comments
Closed

type: make default booleans work #2723

markus2330 opened this issue May 26, 2019 · 14 comments
Assignees
Milestone

Comments

@markus2330
Copy link
Contributor

As written in ElektraInitiative/lcdproc#7

Also I noticed that having boolean keys with default value true doesn't work. I don't know whether that is something that can be fixed though, because I think we would need to transform the default metakey in the type for that.

@kodebach
Copy link
Member

Just to clarify the issue, it is neither specific to the boolean values, nor to the value true.

AFAIK default values are only resolved via elektraLookupBySpecDefault, after kdbGet is done. So any plugin that transforms key values is affected.

I see 3 solutions here:

  • Update all affected plugins, so that they also transform and restore default metakeys. This has some performance impact and also complicates the transformation plugins significantly.
  • Update all affected plugins, so that they call ksLookup on every key they want to transform. This might have other unintended side effects, and definitely is a huge performance impact.
  • Change the whole plugin framework to provide a mechanism tailored for transformation plugins. This mechanism would then take care of all the busy work (restoring values, also transforming defaults, etc.) that currently these plugins need to do on their own.

IMO the first to are really more of a workaround than a solution (also a lot of work) and the third option is clearly out of scope. So I think the best way forward for now, is to just set the default metakeys to normalized values (e.g. 0 or 1 for boolean, only decimal values for integer types, etc). The specification isn't as nice, but I don't think it is a huge problem.

@markus2330
Copy link
Contributor Author

Thank you for the clarification.

For me the option 1 sounds ok: that plugins (like type) also transform the "default" value in the spec namespace (outside the spec namespace, "default" does not have a meaning anyway).

I have doubts that this will have a significant performance impact: In the spec namespace we would simply transform the meta-value of default instead of the value itself (the value is currently never used in the spec namespace).

Another way would be that we store defaults simply in the "spec" keys as values. Then:

  • transformation plugins would simply work as-is
  • ksLookup could return the spec key instead of creating a new key (as the value is already that what we want)

On the negative side we would:

  • lose clarity: more implicit knowledge would be needed (that the value in spec represents the default value is not obvious)

To mitigate the negative side, we could copy the value from "default" to the value (in spec). It would introduce some additional complexity in the spec plugin, though (which is already complex as you know).

@kodebach
Copy link
Member

To mitigate the negative side, we could copy the value from "default" to the value (in spec). It would introduce some additional complexity in the spec plugin, though (which is already complex as you know).

I think this is the best solution. elektraLookupBySpecDefault works on the cascading key created by the spec plugin anyway, so the change shouldn't be hugely complex.

@kodebach
Copy link
Member

Okay new problem....

spec actually already copies the default metavalue into the keyvalue, and elektraLookupBySpecDefault already returns that key including all of the metadata from the spec key. The problem is actually, that the spec plugin is a global plugin. Therefore it is executed after all the non-global plugins (like type).

@markus2330
Copy link
Contributor Author

spec actually already copies the default metavalue into the keyvalue

I did not know about the feature. Where is it implemented?

already returns that key including all of the metadata from the spec key

Yes, but it is not in the spec namespace but a cascading key. And no plugin is executed for cascading keys.

The problem is actually, that the spec plugin is a global plugin. Therefore it is executed after all the non-global plugins (like type).

This does not need to be the case like this and actually we already want to introduce further positions for global plugins. postgetstorage/before/once (but before the non-global plugins) should work? (see doc/decisions/global_plugins.md) (@mpranj any status here?)

@mpranj I accidentally pushed b07ea95 which reverts "cache: temporary disable from default config". Should I revert it again or will we soon get a fix?

@kodebach
Copy link
Member

I did not know about the feature. Where is it implemented?

else if (keyGetMeta (specKey,
"default")) // hardcoded for now because only "default" from "default/*" currently exists
{
Key * newKey = keyNew (strchr (keyName (specKey), '/'), KEY_CASCADING_NAME, KEY_VALUE,
keyString (keyGetMeta (specKey, "default")), KEY_END);
copyMeta (newKey, specKey, parentKey);
ksAppendKey (returned, keyDup (newKey));
keyDel (newKey);
}

Yes, but it is not in the spec namespace but a cascading key. And no plugin is executed for cascading keys.

The plugins wouldn't be executed for the spec namespace either, because normally you wouldn't mount your spec mountpoint with e.g. the type plugin. (Because most validation plugins don't allow empty keys, but spec keys are almost always empty)

@markus2330
Copy link
Contributor Author

libelektra/src/plugins/spec/spec.c
Lines 761 to 769 in f6b1662

This creates a cascading key, not a spec key.

The plugins wouldn't be executed for the spec namespace either, because normally you wouldn't mount your spec mountpoint with e.g. the type plugin.

I think we should start to mount the "type" plugin per default because it implements essential semantics (in particular the booleans).

So the big question is if we can fix the problem with the global positions (see also #1499) or do we need to improve the kdbGet logic ( see also #1291) and bring some order in which the different namespaces are executed (first get spec, then the other namespaces).

@mpranj
Copy link
Member

mpranj commented May 29, 2019

@mpranj I accidentally pushed b07ea95 which reverts "cache: temporary disable from default config". Should I revert it again or will we soon get a fix?

I won't have any time today. It might be that the bugs are only related to multiresolver. Maybe we can leave the cache enabled but I can disable it only for the multifile resolver. That I can do today.

@markus2330
Copy link
Contributor Author

It is best to revert again then (I already did so now). Thank you for the quick response.

@kodebach
Copy link
Member

I think we should start to mount the "type" plugin per default because it implements essential semantics (in particular the booleans).

Good idea, but doesn't solve the problem. Mounting the type plugin for the spec namespace, doesn't make sense. Spec keys don't have values, so the type plugin would always fail. The better solution would be to pass the cascading keys created by spec to the other plugins.

@markus2330
Copy link
Contributor Author

Good idea, [..] so the type plugin would always fail

Good objection. We could only add it by default to mountpoints outside of spec. Using spec-mount, the type plugin will be there anyway (writing a spec without using "type" would be challenging). I created #2733.

The better solution would be to pass the cascading keys created by spec to the other plugins.

Yes, but we should use this change as opportunity to introduce a "default" namespace. Btw. is the type plugin called for the proc namespace? (If not it would make sense to fix this together.)

@kodebach
Copy link
Member

Yes, but we should use this change as opportunity to introduce a "default" namespace.

A default namespace would make sense, but would represent quite a big challenge and might conflict with the default metakey. (Is "default" the root key of the default namespace or is it the key default in the meta namespace?).

Btw. is the type plugin called for the proc namespace? (If not it would make sense to fix this together.)

That is a good question, I am not sure. I believe it would be called for keys comming from a procstorage (global) plugin at least.

@markus2330
Copy link
Contributor Author

A default namespace would make sense, but would represent quite a big challenge and might conflict with the default metakey. (Is "default" the root key of the default namespace or is it the key default in the meta namespace?).

Yes, it is a big change. We could call it "def" or similar.

That is a good question, I am not sure. I believe it would be called for keys comming from a procstorage (global) plugin at least.

Would be good to have some tests of command-line options that violate the specs.

@markus2330 markus2330 added this to the 2.0.0 milestone Apr 12, 2020
@markus2330
Copy link
Contributor Author

I close this issue for 2.0 and created a new docu issue for now: #3397

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants