fix(config): unrecognized config properties don't cause config error #4547

brahmlower · 2022-10-30T07:38:19Z

Fixes an issue where the config was not being loaded when ignored properties are present. This still prints a warning log indicating a possible misconfiguration.

Description

The deserializer wrapper has been updated such that deserialization of ignored properties does not return an error, and instead returns the visitor.visit_none(). This function now includes a warning log statement since the log statement for reporting the error as shown in the bug ticket is no longer reached. (this described the initial implementation before PR review)

The ValueDeserializer wrapper has been updated so that warnings can be either ignored or cause an Err result. At this time, the only warning that gets emitted is the warning from ignored properties during deserialization. This can be used via the new with_allow_unknown_keys function.

The configurable warning behavior is used within the generic from_config implementation of ModuleConfig, wherein the deserializer wrapper is used to load the config. If it reports an "Unknown key" error, then the config is loaded again and deserialized without reporting warnings. This results in the warning be reported without preventing the otherwise valid config being loaded and returned.

Motivation and Context

Closes #4481

Screenshots (if appropriate):

In this screenshot, I'm showing that as I initialize starship with these changes, we can see I have not set a custom format (the line in the config is commented out), and the unexpected property "go" is detected and a warning is printed. After updating the config to enable the format using sed, the shell format is updated with the new prompt without resolving the unexpected config field.

How Has This Been Tested?

I have tested using MacOS
I have tested using Linux
I have tested using Windows

set ~/.config/starship.toml with an unexpected key like the following:

format = 'Hello! $character'

# This should be "golang".
[go]
disabled = true

[shell]
disabled = false

re-initialize starship using the build with eval "$(cargo run init zsh)" (I'm using zsh, but the shell type doesn't matter in this case)
Expect to see the prompt "Hello!" rather than the default shell name (in my case zsh ❯

Checklist:

I have updated the documentation accordingly. (n/a)
I have updated the tests accordingly.

davidkna · 2022-10-30T10:11:17Z

I don't think we currently use Either<A, B> or any enums like that, but I did it like that on purpose. For instance, if a config item has multiple variants, and if one of them fails to parse, this could print warnings; even if one of the other variants ends up being successful and doesn't have any extra keys.

brahmlower · 2022-10-30T16:30:10Z

Ah okay, that's good foresight. Unless I've misunderstood though, I believe a scenario using Either<A, B> without my changes in this PR would still result in deserialize_ignored_any returning an Err if B was provided and A was checked first, meaning deserialization would still fail? I'm not sure how to rectify this. As I see it, we have a couple core facts:

We shouldn't emit logs during deserialization
We can't return warnings without using Err
Warnings shouldn't cause deserialization to fail

So it would seem like we can't support the "Unknown key{did_you_mean}" feature without logging during serialization and without causing deserialization to fail.

One thought was we could give ValueDeserializer an extra config such that warnings do cause deserialization failures so that we could log them, and then we could just run it again without warnings as errors. That wouldn't solve the case you brought up though, because it would still potentially cause a warning to be emitted for an Either typed config.

Do you have any preferences or direction for this?

davidkna · 2022-10-30T16:36:52Z

Ah okay, that's good foresight. Unless I've misunderstood though, I believe a scenario using Either<A, B> without my changes in this PR would still result in deserialize_ignored_any returning an Err if B was provided and A was checked first, meaning deserialization would still fail? I

If I remember correctly, serde(untagged) works by trying each variant in turn, until the first deserialization succeeds (does not return an error).

One thought was we could give ValueDeserializer an extra config such that warnings do cause deserialization failures so that we could log them, and then we could just run it again without warnings as errors. That wouldn't solve the case you brought up though, because it would still potentially cause a warning to be emitted for an Either typed config.

I guess that would be okay if it's simple to implement.

brahmlower · 2022-10-30T16:48:20Z

I guess that would be okay if it's simple to implement

Yeah I def agree on the "if it's simple" part. The existing interface is really nice, and I don't want to undermine that 😅

I don't think I'll have a chance to get back to the computer this this evening, so I won't have any code updates for another 12 hours or so

Edit: oh, I'll also brush up on the serde untagged behavior, because I wasn't aware that's how it handled parse errors 👍

brahmlower · 2022-10-31T07:17:56Z

Alrighty! I've got the deserialization retry behavior implemented, so now we'll only print the warning when intended 👍

brahmlower · 2022-10-31T18:36:06Z

Hey @davidkna, sorry to bother you with this but would you be comfortable adding the "hacktoberfest-accepted" label to this PR today? I don't at all want to rush review of these changes and I definitely intend to see this through to a state you're comfortable with, but this is my 4th and final PR for the event 🙏 Adding the label will give me credit without you having to approve or merge the changes in their current form

davidkna · 2022-10-31T18:44:28Z

Sure.

brahmlower · 2022-10-31T19:15:02Z

Awesome, thank you! I realized I still need to update the description to reflect the revised approach we came up with yesterday. I can take care of that this evening 👍

davidkna

I have made two suggestions, but apart from that this looks good to me.

src/serde_utils.rs

davidkna · 2022-11-02T13:34:54Z

src/serde_utils.rs

+        }
+    }
+
+    pub fn with_no_ignored_error(self) -> Self {


Please find a better name for this function, the current name sounds a bit like it would make the deserializer not ignore errors.

I definitely struggled to find a good name for this- iirc I flip flopped between a few names while working on it 😅

How does without_warnings sound?

How about with_allow_ignored_keys(), with_allow_unknown_keys() or with_ignored_noerror()?

ah yeah these are much better suggestions than mine 😅 Lets go with with_allow_unknown_keys()- I feel like that works best as part of the public interface for the struct whereas the other two lean more heavily on verbiage from the internal implementation 🤔

I'll make these changes and get them pushed in a minute

brahmlower · 2022-11-05T03:24:27Z

Looks like that lint error is due to some clippy changes for rust 1.65.0, since that job is targeting stable

brahmlower · 2022-11-05T04:27:49Z

Hmm, I tried fixing the lint errors but it turned into a bit of a slippery slope that seemed a little out of scope for this PR, so I've just rebased those commits out. 😬 I still have them locally if you'd like me to open a separate PR fixing the lint issues

andytom · 2022-11-05T13:27:52Z

Hmm, I tried fixing the lint errors but it turned into a bit of a slippery slope that seemed a little out of scope for this PR, so I've just rebased those commits out. grimacing I still have them locally if you'd like me to open a separate PR fixing the lint issues

I've merged the latest clippy fixes so if you rebase your branch that should (hopefully) fix them.

…sent

from review feedback Co-authored-by: David Knaack <davidkna@users.noreply.github.com>

brahmlower · 2022-11-05T18:45:51Z

Thanks for fixing those lint errors Thomas! With this branch rebased, we're all set for final review 👍

davidkna · 2022-11-06T09:31:49Z

src/config.rs

+                log::warn!("{}", err);
+                let deserializer2 = ValueDeserializer::new(config).with_allow_unknown_keys();
+                T::deserialize(deserializer2)


Didn't you add or edit and existing test? Did it get lost during rebasing?

I just dug through my reflog to confirm, and it looks like I had only updated the test in serde_utils.rs. I thought the coverage reports showed this code branch was covered by that test change, but maybe I'm misremembering 🤔 I'll do some further testing/experimenting locally in the morning to see what's up

In that case, would you mind adding a test that uses this path?

Hey! Sorry for the crazy delay here- I caught the flu a couple weeks ago and it ended up wiping me out pretty bad. I'm slowly getting back in the saddle here though 😅 I just wrapped up work today and will get this last test added this evening

Had to step away for a while while CI was running, but looks like that test has properly covered that block now 👍

andytom · 2022-11-20T17:28:09Z

Thank you for your contribution @brahmlower and thanks for reviewing @davidkna.

…tarship#4547) * Fix starship#4481, config does not error when unrecognized properties are present * cleanup: use stuct update syntax to improve readability from review feedback Co-authored-by: David Knaack <davidkna@users.noreply.github.com> * cleanup: renamed ValueDeserializer func w/ better name * cleanup: added test to cover unknown key retry condition Co-authored-by: David Knaack <davidkna@users.noreply.github.com>

brahmlower changed the title ~~Fix #4481, config does not error when unrecognized properties are present~~ fix(config) Resolve #4481, unrecognized config properties don't cause config error Oct 30, 2022

davidkna added the hacktoberfest-accepted label Oct 31, 2022

davidkna requested changes Nov 2, 2022

View reviewed changes

brahmlower and others added 3 commits November 5, 2022 11:05

Fix #4481, config does not error when unrecognized properties are pre…

b5ff7cd

…sent

cleanup: use stuct update syntax to improve readability

98b0987

from review feedback Co-authored-by: David Knaack <davidkna@users.noreply.github.com>

cleanup: renamed ValueDeserializer func w/ better name

bd126a7

davidkna reviewed Nov 6, 2022

View reviewed changes

andytom changed the title ~~fix(config) Resolve #4481, unrecognized config properties don't cause config error~~ fix(config): unrecognized config properties don't cause config error Nov 6, 2022

cleanup: added test to cover unknown key retry condition

5bba474

davidkna approved these changes Nov 16, 2022

View reviewed changes

andytom merged commit 1b03ef2 into starship:master Nov 20, 2022

github-actions bot mentioned this pull request Nov 20, 2022

chore(master): release 1.12.0 #4485

Merged

brahmlower deleted the fix-4481-ignore-unrecognized-config-fields branch November 20, 2022 20:58

github-actions bot mentioned this pull request Dec 13, 2022

chore(master): release 1.10.0 #4714

Closed

This was referenced Feb 25, 2023

chore(master): release 1.10.0 #4933

Closed

chore(master): release 1.10.0 #4934

Closed

chore(master): release 1.10.0 #4938

Closed

github-actions bot mentioned this pull request Jun 6, 2023

chore(master): release 1.16.0 #5224

Closed

github-actions bot mentioned this pull request Jul 30, 2023

chore(master): release 1.17.0 #5348

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(config): unrecognized config properties don't cause config error #4547

fix(config): unrecognized config properties don't cause config error #4547

brahmlower commented Oct 30, 2022 •

edited

Loading

davidkna commented Oct 30, 2022 •

edited

Loading

brahmlower commented Oct 30, 2022

davidkna commented Oct 30, 2022

brahmlower commented Oct 30, 2022 •

edited

Loading

brahmlower commented Oct 31, 2022

brahmlower commented Oct 31, 2022

davidkna commented Oct 31, 2022

brahmlower commented Oct 31, 2022

davidkna left a comment

davidkna Nov 2, 2022

brahmlower Nov 3, 2022

davidkna Nov 4, 2022

brahmlower Nov 5, 2022

brahmlower commented Nov 5, 2022

brahmlower commented Nov 5, 2022

andytom commented Nov 5, 2022

brahmlower commented Nov 5, 2022

davidkna Nov 6, 2022

brahmlower Nov 6, 2022

davidkna Nov 13, 2022

brahmlower Nov 16, 2022

brahmlower Nov 16, 2022

andytom commented Nov 20, 2022

fix(config): unrecognized config properties don't cause config error #4547

fix(config): unrecognized config properties don't cause config error #4547

Conversation

brahmlower commented Oct 30, 2022 • edited Loading

Description

Motivation and Context

Screenshots (if appropriate):

How Has This Been Tested?

Checklist:

davidkna commented Oct 30, 2022 • edited Loading

brahmlower commented Oct 30, 2022

davidkna commented Oct 30, 2022

brahmlower commented Oct 30, 2022 • edited Loading

brahmlower commented Oct 31, 2022

brahmlower commented Oct 31, 2022

davidkna commented Oct 31, 2022

brahmlower commented Oct 31, 2022

davidkna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brahmlower commented Nov 5, 2022

brahmlower commented Nov 5, 2022

andytom commented Nov 5, 2022

brahmlower commented Nov 5, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andytom commented Nov 20, 2022

brahmlower commented Oct 30, 2022 •

edited

Loading

davidkna commented Oct 30, 2022 •

edited

Loading

brahmlower commented Oct 30, 2022 •

edited

Loading