comment storage / metadata in general

[kodebach]

I looked into handling comments for the toml plugin and found that basically the current approach doesn't work with storing metadata. This lead me to the conclusion that we may need to rethink metadata in general.

Before you comment: The first part is only there to understand my conclusion. Please ignore any errors, misunderstandings, etc. and refrain from commenting on it. I added my main questions at the end. Just comment on those, to avoid unnecessary discussion that goes in circles. Additionally, question 1 is a lot more immediately relevant than the rest, so maybe answer that one first.

Introduction

Currently we have this specification for storing comments on Keys (via meta:/ keys):

libelektra/doc/METADATA.ini

Lines 98 to 169 in f655398

	[comment]
	type = string
	status = deprecated
	usedby/plugin = keytometa
	description = "The user's comment about the key (mostly its value).
	The first line is the comment after a key-value (in the same line)
	further lines are the lines before a key-value.
	This metadata is deprecated because it assumes line-based comments
	and cannot preserve formatting within comments."
	note = "If used within specifications it refers to the specification, not
	the key. Use `description` to refer to the key."
	[comment/#]
	status = implemented
	usedby/plugin = hosts
	type = string
	description = "A comment preceding or in-line with a key.
	- #0 is the inline comment (in the same line).
	- The comment numbering starts from top (#1) to bottom (#n).
	- The comment directly above the key is the last entry in the array (#n).
	- The topmost comment belonging to the key is (#1).
	Comment keys that are not above any key (comments in the last
	lines of files) are added to the parentkey.
	The array of comments allows:
	- to structure comments, which are not line-based
	- to restore formatting, including:
	- inline comments
	- spaces before comments
	- empty lines
	- to distinguish between different kinds of comments
	- to tag every comment, which increases extensibility
	The plugin need to take care that only valid characters are
	present in the comment.
	E.g. on line-based comments, no newlines must be present."
	note = "If used within specifications it refers to the specification, not
	the key. Use `description` to refer to the key."
	[comment/#/start]
	status= implemented
	usedby/plugin= hosts
	type= string
	description= "Determines the character(s) used to start this comment.
	This metakey is empty if its an empty line/comment.
	The default on absence of this key is plugin-specific.
	Some comment start character should be assumed then.
	The plugin needs to make sure that every comment is actually
	commented out. An error needs to be emitted if there is no
	start symbol that will be recognized as comment."
	example= ;
	[comment/#/space]
	status= implemented
	usedby/plugin= hosts
	type= string
	description="The spaces/tabs used between a comment and the beginning of the
	line or after end of payload data (for inline comments).
	If no start key is present, only space and no comment is present.
	The plugin needs to make sure that all spaces present are allowed.
	An error needs to be emitted if non-ignored spaces are present."
	example=

I do have some notes on this, but those are irrelevant, because this system is still flawed.

Problem 1: Not all formats are line-based

The first big flaw is that it still assumes file-based storage and a line-based format. Why is this a problem?

Take JSON5 a superset of JSON that supports comments. So this would be a valid JSON5 file:

{"foo":"val", /* comment 1 */   "bar":123/*
comment2
*/,"boo": false}

I don't see how that could be mapped cleanly onto the current comment/# system. Sure both comments belong to bar, but also both are inline (in fact without comments the whole file would be one line). The first comment also has preceding and following spaces.

All this could be solved, by saying:

1. comment/# is for storing comments.
2. comment/#/text contains the text of comment number N.
3. What the rest of comment/#/_ looks like what "comment number N" means, is up to the storage plugin.
4. Formatting of comments may be lost when switching storage formats, but all formats that support comments (or generic metadata) must preserve all comment text and the order of comment text. Specifically, if you just take all comment/#/text values in array order and print them separated by \n you must always get the same result. Additional formatting stored in other metakeys (e.g. spaces before after comment, etc.) may be lost.

However, there is a bigger problem.

Problem 2: Comments on metakeys

Currently, there is no way to store comments that associated not with a key, but with a single metakey. This is not a real problem for Elektra, since comments on metakeys don't really make sense. What exactly would be the point of putting a comment on e.g. a type metakey?

However, unless a storage format was specifically designed for Elektra (e.g. dump, quickdump, but also ni), there probably won't be a syntactical difference between a key and a metakey. For example take TOML (syntax from #3911):

# standard comment on a key
foo.bar = 456

# looks like a standard comment, but is on a metakey
foo."bar/meta".default = 123

The first comment is totally fine and will be stored somewhere in the comment/# metadata of the key foo/bar. The second comment looks, from a TOML standpoint and to almost any user, syntactically identical to the first one. However, where would that be stored?

Is see only three options here:

1. The second comment results in an error
   • Easiest to implement
   • Can be very confusing to user who just work with the TOML file.
   • Adding comments in spec files (which contain almost exclusively metakeys) becomes very hard.
2. Allow some sort of meta-metakeys
   • Must be restricted to not run into the same problem again, i.e. meta-metakeys can never be stored and are only implicitly generated.
   • Could break a lot of code, because essentially everything assumes metakeys don't have metakeys (even though it is technically possible right now).
3. Introduce a comment:/ namespace
   • This was an idea I originally had. I thought this might solve the problem and it could, but it is not very pretty.
   • Basically, we add another namespace comment:/ that is also used in metadata KeySets.
   • The root key comment:/ is for the key itself, and e.g. comment:/type would be for meta:/type.
   • The problem of course is, that we would again only have a single key to store the comment text itself, as well as the formatting of the comment.
   • A solution could be to say, comment:/ keys are always binary an contain struct values e.g.

     struct {
         char * text;         // the actual comment text
         size_t textAlloc;    // allocated size of text
         const char * plugin; // name of the storage plugin used
         void * info;         // other information specific to the storage plugin
     };

     The plugin name is stored so that a transfer from one format to another doesn't try to cast info into a wrong type.
   • Note: I really don't like this solution, but so far it is the only thing I would consider acceptable other than throwing an error on metakey comments.

Conclusion

All of this leads my to the conclusion: Maybe we need to rethink metakeys themselves.

Currently metakeys serve a few purposes:

1. When attached to spec:/ keys, they define a specification.
   This data must obviously stored. But spec:/ keys should not have a value, so this is much easier to deal with than real metadata. In a sense, this isn't even metadata, since there is no data. You could say that every spec:/ key only contains a keyset (via the meta-keyset) and that this keyset is the actual data.

2. Internal metadata only used at runtime, e.g. error, warnings/#, origvalue, internal/*/*
   In contrast, this metadata should never be stored. Additionally, users should never interact with it. Essentially, here metadata is more or less abused only to keep some information attached to a Key *.

3. Metadata about the underlying storage format, e.g. order, tomltype
   This data is not internal in the same sense as above. While it should never be stored explicitly, it influences the underlying storage (file or otherwise) in a user-visible way. Therefore, some users may want to interact with it, e.g. to change the order of a key or to convert a TOML inline table to a normal one.

4. Metadata about the configuration data itself, e.g. array, type (in a more limited fashion)
   This is actually the only case of true metadata we have, in the sense that this is additional data that must be kept attached to and stored with the actual data. This data must also not necessarily be stored explicitly, but it must be preserved somehow. For example, we don't need to store type explicitly, if the storage format has native type support.
   One important note here is, that if a specification is present, some of this data may be reconstructed from it. But this is not always the case. For example, theoretically a spec could mandate a type = long key, but a type = short key is provided. A more real world (and implemented) example is array. You need to know the intended size of an array, so that default values can be generated by spec.

   array is actually the only metakey that truly falls into this category at the moment. Replacing type=long from a spec with type=short could be supported, but isn't really right now. Of course, if no specification is used, everything falls into this category, but for this I assume that everybody uses a specification.

Note: The distinction between type 3 and 4 is, that loosing type 3 metadata might change some stored bytes somewhere, but it will not influence the behaviour of any application. Type 4 metadata, however, must be preserved to guarantee applications keep working. For example, the order of keys in a storage file should never influence an application, but the type of a key could (e.g. long vs. short).

Questions

My questions now are:

1. Should comments on metakeys be allowed, or should we just discard them, throw an error, otherwise deal with them without a guaranteed preservation?
2. Should we decide that spec:/ is a completely separate thing and maybe rethink how deal with it? I more and more think that specification as metakeys was a big mistake.
3. Should we introduce a separate namespace for internal metadata, to more clearly indicate that it is runtime-only and not meant for user interaction? e.g. internal:/
4. Is the spec plugin really a good idea? Should we copy specification from the spec:/ namespace onto other keys? Wouldn't it better, if plugins iterated the spec:/ instead?
5. This leads to: Is it a good idea that you could define check/range=7-10 in spec:/foo/bar, but only mount the range plugin on system:/foo, but user:/foo? Shouldn't we enforce mounted plugins to be the same for one mountpoint path (key w/o namespace) regardless of namespace?
6. Finally: Should spec really (try to) generate all keys in advance? I know this is an attempt to make ksLookup perform close to O(1), but generating all keys in advance barely works (for #) or doesn't work at all (_) and uses very complicated, kinda slow code. Wouldn't it be better to let ksLookup generate default keys from # and _ specs? Even if this means recursively breaking apart the lookup key to find potential # and _ spec keys.

In know these are all very big questions and most of them put the very basis of Elektra into question. However, I'm beginning to feel that without addressing these issues, we may reach 1.0 only to realise that 1.0 doesn't actually work well and we need to start working on 2.0 immediately.

Nonetheless, the only question that actually needs an answer right now is question 1 (concerning metakey comments).

[mpranj]

Regarding question 1:

In short, I would definitely care about comments I have in my config files. I would assume other users will want comments to work as well (and not be discarded).

Many config files consist of more comments than actual data (e.g. apache config). I think discarding the comments would be the worst solution of all.

If I understand Problem 2 correctly, the main problem is where to store this data. If I would have to make a solution right now, I would store the comments as first class citizens (the same way as names and values) in the Key struct. One remaining problem with this approach would be how to handle comments without any key (e.g. a config file containing a single line which is a comment). A very quick solution could be a key flag to indicate that it is only a comment.

The meta-metakey and comment:/ namespace approaches seem quite complex to me.

[kodebach]

In short, I would definitely care about comments I have in my config files. I would assume other users will want comments to work as well (and not be discarded).

I see it the same way, that's why I started thinking about this.

If I understand Problem 2 correctly, the main problem is where to store this data. If I would have to make a solution right now, I would store the comments as first class citizens (the same way as names and values) in the Key struct.

Separate fields in struct _Key is a good idea, but there is still the issue of how to store formatting information. A single char * field is definitely not enough, since there can be more than one comment. So the comment text itself has to be an array. Using a char ** (plus a size/alloc field) would still be fine, but we still need information about other formatting like surrounding spaces, comment start char (if there are multiple options), etc. This information is different for each storage format, so we need a generic solution.

One remaining problem with this approach would be how to handle comments without any key (e.g. a config file containing a single line which is a comment). A very quick solution could be a key flag to indicate that it is only a comment.

This issue already exists with the current solution. I think currently we would store those on the mountpoint parent (as a key in the KeySet, not as parentKey). A file that just contains a comment is rare, but yes, we should probably add a flag to indicate that.

The meta-metakey and comment:/ namespace approaches seem quite complex to me.

Maybe a combined approach where we allow comment:/ keys as metakeys on all non-comment:/ keys could work. We still have to deal with a kind of meta-metadata, but it is intrinsically limited. At the same time we have a whole namespace of keys to store formatting and other information.

[markus2330]

Thank you both for thinking hard about these problems. I did not have time to read everything, but here the answers to the questions.

Preserving the format in general is a complex topic (if you want a data structure useful to the user, i.e., not designed to preserve format). See also #3297 for a very similar problem in arrays. We should not aim for perfection but for good usability.

Should comments on metakeys be allowed, or should we just discard them, throw an error, otherwise deal with them without a guaranteed preservation?

We should preserve them but not always exactly at the place where they were written.

Should we decide that spec:/ is a completely separate thing and maybe rethink how deal with it? I more and more think that specification as metakeys was a big mistake.

spec:/ is a totally separate thing, that is why I constantly talk about a "meta" option to toml/ni or to use "ni" as default for spec:/. spec:/ does not have values but only metadata, so formats for spec:/ can be simplified (not storing values but only metadata in sections) and also formats of other namespaces can be simplified (not storing arbitrary metadata)

Should we introduce a separate namespace for internal metadata, to more clearly indicate that it is runtime-only and not meant for user interaction? e.g. internal:/

This is a huge topic on its own, see #3563. We would probably need a decision meeting for it. For now I would leave it as-is but we should decide it before we rewrite the spec plugin.

Is the spec plugin really a good idea? Should we copy specification from the spec:/ namespace onto other keys? Wouldn't it better, if plugins iterated the spec:/ instead?

I think it is a good idea as it makes the most common form of plugins (the checker plugins) much easier to write.

This leads to: Is it a good idea that you could define check/range=7-10 in spec:/foo/bar, but only mount the range plugin on system:/foo, but user:/foo? Shouldn't we enforce mounted plugins to be the same for one mountpoint path (key w/o namespace) regardless of namespace?

"cascading mounts" and spec-mount already make sure that the correct plugins are present for all namespaces. In #3693 we can rethink what a "cascading mount" actually means.

Finally: Should spec really (try to) generate all keys in advance? I know this is an attempt to make ksLookup perform close to O(1), but generating all keys in advance barely works (for #) or doesn't work at all (_) and uses very complicated, kinda slow code. Wouldn't it be better to let ksLookup generate default keys from # and _ specs? Even if this means recursively breaking apart the lookup key to find potential # and _ spec keys.

We should say much more clearly that Elektra requires kdbSet to be used directly after any modification or addition of keys. This simplifies the whole problem drastically, as then we can have much better support for such features using plugins.

Separate fields in struct _Key is a good idea

It is something we already had: it makes the _Key too fat. We should slim _Key down and not increase the size further.

[markus2330]

The problem you outline is mostly because spec+config is intermixed, which is not recommended. So probably we should not support the mixing. Removing all comments just because there are some cases where comments are at a "wrong" place is to throw the baby out with the bathwater. We have to be clear that comment-preserving is best-effort and comments might be reordered or duplicated (in 3-way merge). Even with reordering there are plenty of possibilities to give nice usability.

Second: I don't like the idea of a "meta mode" for toml. Either the plugin reads/writes proper TOML, or it doesn't.

Of course it would be TOML, but interpreted differently: In the metamode key=value would always refer to meta-data and sections would refer to keys. Afaik TOML does not specify how the emitted data structure looks like, so it should be perfectly valid to do this?

[kodebach]

The problem you outline is mostly because spec+config is intermixed, which is not recommended.

I don't know where you got that idea from. My example is purely a specification for the key foo (below some mountpoint). There is absolutely no config data in the example. That's why I started with the ni version, which is our go-to format for specification.

Removing all comments just because there are some cases where comments are at a "wrong" place is to throw the baby out with the bathwater. We have to be clear that comment-preserving is best-effort and comments might be reordered or duplicated (in 3-way merge). Even with reordering there are plenty of possibilities to give nice usability.

We might get into personal preferences here (objectively both options are bad), but I'd much prefer it, if comments are just not supported and discarded or some unspecified reordering/duplicating. I can work around the fact, that certain comments in well-defined cases get discarded (in the example above I would know to write, e.g. # TODO: check/range is redundant, remove in next version). But "best-effort preservation" and an some unspecified reordering would be very suspicious to me. Best-effort basically means "we don't actually know for certain, but mostly it should be fine".

For switching from one storage format to another a best-effort approach would be fine. I don't expect a perfect mapping between e.g. TOML and Java Properties. But within one format, I would expect a well-defined and predictable behaviour. So if there is reordering it should be clearly defined how and when stuff is reordered. Personally, I don't want to think about all the possible edge case to write this definition, so I prefer just discarding comments that can't be stored.

Sidenote: "in a 3-way merge" can be ignored here IMO. What happens in a 3-way merge is not in the control of storage plugins and during an automatic conflict resolution duplicated, missing or otherwise broken config should be expected (or the merge should just fail to avoid this).

Of course it would be TOML,

I answered in #3911 (comment). TL;DR no it wouldn't be TOML, the TOML spec defines semantics as well as syntax.

[markus2330]

There is absolutely no config data in the example.

Values are config data.

That's why I started with the ni version, which is our go-to format for specification.

ni currently also supports intermixing of configs/specs

We might get into personal preferences here (objectively both options are bad)

Obviously this is mostly about personal preferences but lets try to get to the objective core. There are at least two properties:

1. no reordering of comments
2. no losing of comments

I do not like the idea of not having neither property 1 nor 2. For me personally 2. seems to be more important but that is obviously objective.

Currently, in most cases (like comments before/after =) we simply do not parse files where comments cannot be mapped, probably we should do the same for the meta-comments?

In #3297, however, we simply lose the comment.

[kodebach]

Values are config data.

What values are you talking about? I only defined the type and check/range metakeys for the key foo.

Do you mean the foo = /foo = "" lines? That is the result of the the comment merging process. We can forbid that and put the comments above the [foo]/["foo/meta"] lines, but that isn't any better:

# TODO: redundant, remove in next version
["foo/meta"]
type = "unsigned_short"
check.range = "0-255"

# TODO: redundant, remove in next version
[foo]
type  = unsigned_short
check/range = 0-255

In fact this may be worse, because it now implies that all the metadata for foo is somehow redundant, when before it could have been interpreted as the foo = line being redundant.

ni currently also supports intermixing of configs/specs

Yes, but I didn't use that in my initial example.

I do not like the idea of not having neither property 1 nor 2. For me personally 2. seems to be more important but that is obviously objective.

Stated like this, I would also prefer 2 over 1, but "reordering" isn't really the correct term for what is happening IMO. I'd also say a comment that is move to a different location can also be called "lost". So 2 without 1 could be seen as comments remain with the same TOML key-value pair, but they could move up/down or become (non-)inline.

These properties describe the situation better I think:

A. comment text is preserved
B. comment location is preserved

In this case, I would say I'd rather not have comments at all then comments that move around. I'd also say having B without A makes zero sense.

we simply do not parse files where comments cannot be mapped, probably we should do the same for the meta-comments?

This is probably the best compromise, yes.

[markus2330]

This is probably the best compromise, yes.

👍