Check --config for dotted keys only

[jonhoo]

This addresses the remaining unresolved issue for config-cli (#7722).

[rust-highfive]

r? @Eh2406

(rust-highfive has picked a reviewer for you, use r? to override)

[Eh2406]

The code looks good to me, but @ehuss knows the desired behavior much better than I do.

@bors r? @ehuss

[alexcrichton]

Personally I don't think that this is a great solution for the problem which is to write an inline toml parser for the keys here. I think it would be better to expose more APIs from the toml crate itself (or something like that) to avoid duplicating all the code here.

[jonhoo]

@alexcrichton I personally disagree here — it seems odd to me for toml to specifically expose parsers for sub-parts of the spec. Feels like it would expose too much of the implementation details of the library unnecessarily. Even making Deserializer::key_value or just Deserializer::dotted_key leaks details like Line and Span, neither of which currently need to be public. It's true that we're re-implementing part of the parser here, but it's a very small part, and it's simplified by the fact that we only need to do validation and not actual deserialization into a value. Ultimately it's up to you all though of course. I'd be happy to start pushing on a contribution to toml instead to expose the necessary APIs if you have an idea of what that might look like, and then follow up on using that here.

[ehuss]

I also would prefer to not have a partial TOML parser here.

Unfortunately at the moment the TOML story is in flux. My original intent was to expose a raw interface in the toml crate to make this easy. That interface was intended to then be the basis of editing support (the low-level interface looked like this). However, we're now looking to instead use toml_edit entirely. Perhaps you could discuss with @epage the feasibility of exposing something that could provide lower-level access to the tokenizer? I'm not sure what that would look like or if they would be willing to provide something like that, but I think it would be good to discuss first.

[jonhoo]

Would love to hear your thoughts @epage!

A secondary observation here is that we could always swap out the custom validation logic here for something provided by toml or toml_edit down the line, assuming the current validation is pessimistic in what it accepts (which I believe it is). The motivation being that it would unblock --config in the meantime if we expect the necessary changes to toml/toml_edit to be on a relatively long time horizon.

[epage]

Let me see if I understand the problem being solved.

We are adding a --config flag that works as either

• --config <path>
• --config KEY=VALUE

In doing so, we are trying to parse KEY as toml key and VALUE as toml value

In using toml, we want to restrict the syntax so someone can't inject more complex toml expressions (tables, etc).

Is that the gist?

We have a impl FromStr for Key which might do the trick. It had some unnecessary additional logic. toml-rs/toml#273 is removing that so it more directly handles keys as toml would expect (and people can build their own logic on top).

[jonhoo]

I think we specifically want to support any single TOML dotted-key expression in --config, but yes, that's the gist of it. I don't think it's actually important that we parse the full thing in Cargo — the current approach of checking that the argument to --config is a dotted key followed by =, and if it is parsing it with the full parser seems totally fine as a (perhaps simpler) alternative.

From the docs, it looks like Key is just a single key, and not a dotted key, is that right? Is there an type equivalent to a dotted key?

[jonhoo]

Oh, and for the value I think we want to support any value that's not an inline table. Arrays, for example, are fine.

[epage]

From the docs, it looks like Key is just a single key, and not a dotted key, is that right? Is there an type equivalent to a dotted key?

You are right, I forgot that it was only a simple key and not a full key path. I think its reasonable for us to extend this so people can parse keys, see toml-rs/toml#274.

I think we specifically want to support any single TOML dotted-key expression in --config, but yes, that's the gist of it. I don't think it's actually important that we parse the full thing in Cargo — the current approach of checking that the argument to --config is a dotted key followed by =, and if it is parsing it with the full parser seems totally fine as a (perhaps simpler) alternative.

This is making me wonder, why not parse the key=value as a TOML document? That seems the simplest. You can do whatever error handling on top to restrict what is supported. Granted, if you are concerned about whitespace and comments, that can be annoying to detect though it will be easier when switching to toml_edit since that is preserved and errors can be generated just like with table handling.

[jonhoo]

This is making me wonder, why not parse the key=value as a TOML document?

I don't immediately see a way to inspect a Document to see that it only uses dotted keys for example? It just parses the whole thing and produces a Map for you, which means I can't actually check the input syntax.

I'm working on an update to this PR now that moves to toml_edit's parsing, and I noticed https://docs.rs/toml_edit/latest/src/toml_edit/de/mod.rs.html#70-76 which I believe should be using serde::DeserializedOwned as a bound rather than Deserialize<'static>. That will at least make them much more useful 😅

[jonhoo]

My current draft looks something like this:

// We only want to allow "dotted key" (see https://toml.io/en/v1.0.0#keys)
// expressions followed by a value that's not an "inline table"
// (https://toml.io/en/v1.0.0#inline-table). There isn't a way to constrain the
// toml_edit parser to _just_ an expression of that form, but we can get pretty far
// by checking if any `=`-separated prefix parses as the `toml_edit::Key` type. If
// there are multiple `=` in there, we need to split at each one until parsing
// succeeds, because the first `=` could be in a "-quoted string for example.
let mut separator = None;
for (ind, c) in arg.char_indices() {
    if c != '=' {
        continue;
    }
    if let Ok(_) = arg[..ind].parse::<toml_edit::Key>() {
        separator = Some(ind);
        break;
    }
}
let separator = if let Some(s) = separator {
    s
} else {
    bail!(
        "--config argument `{}` was not a TOML dotted key expression (a.b.c = _)",
        arg
    );
};
let toml_v = arg[(separator + 1)..]
    .trim()
    .parse::<toml_edit::Value>()
    .with_context(|| {
        format!("failed to parse value from --config argument `{}`", arg)
    })?;
if let toml_edit::Value::InlineTable(_) = toml_v {
    bail!(
        "--config argument `{}` uses inline table values, which are not accepted",
        arg,
    );
}

// Rather than trying to piece the key/value back into a toml::Value, we just parse
// the whole argument again now that we know it has the right format.
let toml_v = toml::from_str(arg).with_context(|| {
    format!("failed to parse value from --config argument `{}`", arg)
})?;

Though it fails on dotted key expressions as expected.

[epage]

Though it fails on dotted key expressions as expected.

Release is now out with Key::parse

I don't immediately see a way to inspect a Document to see that it only uses dotted keys for example? It just parses the whole thing and produces a Map for you, which means I can't actually check the input syntax.

For toml_edit, you can check TableLike::is_dotted() to make sure a table only exists via dotted keys. You can also check either table type's length to ensure its one and the value and table's Decor for comments.

I'm working on an update to this PR now that moves to toml_edit's parsing, and I noticed https://docs.rs/toml_edit/latest/src/toml_edit/de/mod.rs.html#70-76 which I believe should be using serde::DeserializedOwned as a bound rather than Deserialize<'static>. That will at least make them much more useful sweat_smile

Haven't messed with DeserializedOwned, I'll take a look

[jonhoo]

I'm fiddling with getting to use TableLike::is_dotted now, because it seems like a good path to go down, but realized that Item::as_table doesn't actually recurse into inline tables (through Value), just through top-level tables. Is that intentional?

Edit: ah, I want as_table_like!

[jonhoo]

Fixed up following the merge of #10086.

[ehuss]

Looks good!

Would it be possible to also reject comments or other decoration?

[epage]

Would it be possible to also reject comments or other decoration?

Parts to check

• Document::trailing
• Table::decor
• Table::iter_mut to access KeyMut::decor
• Value::decor

Alternatively,. this could be implemented via visit. Looks like we don't expose whats needed for dealing with keys. I might get a change out real quick for that.

[jonhoo]

Working on a fix to check for non-empty decorators (whitespace should be allowed I think), but am running into the fact that Formatted<T> only has .decor() when T: toml_edit::repr::ValueRepr, but while ValueRepr is pub, it's in a non-pub module and not re-exported, so I can't write a generic wrapper around it. Probably worth fixing @epage.

[epage]

Working on a fix to check for non-empty decorators (whitespace should be allowed I think), but am running into the fact that Formatted<T> only has .decor() when T: toml_edit::repr::ValueRepr, but while ValueRepr is pub, it's in a non-pub module and not re-exported, so I can't write a generic wrapper around it. Probably worth fixing @epage.

How come you are dealing with Formatted rather than Value::decor?

[jonhoo]

I wanted to write a helper to deal with the non-recursive variants Value. I've since ended up writing it differently (using Value::decor), but figured I'd point out this "private type in public interface" since I spotted it :)

[jonhoo]

Would love if you have the time to look over the way I implemented this too @epage and see if it matches what you expected to see?

[epage]

Decor handling looks good.

[ehuss]

Thanks! ❤️

@bors r+

[bors]

📌 Commit 02e071c has been approved by ehuss

[bors]

⌛ Testing commit 02e071c with merge 018acfd...

[bors]

☀️ Test successful - checks-actions
Approved by: ehuss
Pushing 018acfd to master...