Handle empty and null sources correctly #93
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We're currently handling empty (or comment-only) and null YAML sources incorrectly. This diff fixes both by simplifying the configuration provider even further. These sources are problematic for two reasons. First, we're not special-casing `io.EOF` errors when we use `yaml.Decoder`. More insidiously, when we unmarshal sources, we're not distinguishing between nil values caused by empty (or comment-only) sources and explicit top-level nulls. The latter is particularly problematic because an explicit null should be higher-priority than all defaults provided via `WithDefault`, even if that null is itself overriden by another YAML source. (For example, if `base.yaml` is just `~` and `production.yaml` is `foo: bar`, the explicit top-level null in `base.yaml` should trump any data supplied by `WithDefault`.) This commit fixes these issues by making three interrelated changes: 1. It changes all YAML options to eagerly consume `io.Reader`s, and changes the internal representation of YAML sources to byte slices instead of readers. This simplifies later changes, since we'll be consuming each source multiple times. 2. It special-cases `io.EOF` errors where appropriate, taking care to distinguish empty sources and explicit nulls. 3. It changes `WithDefault`'s internals to match the documentation: the implementation literally uses the user-supplied value as the lowest-priority search and deep-merges it with all other sources.
Codecov Report
@@ Coverage Diff @@
## master #93 +/- ##
=========================================
- Coverage 95.54% 94.35% -1.2%
=========================================
Files 8 8
Lines 382 390 +8
=========================================
+ Hits 365 368 +3
- Misses 11 14 +3
- Partials 6 8 +2
Continue to review full report at Codecov.
|
abhinav
approved these changes
Jul 6, 2018
option.go
Outdated
| all, err := ioutil.ReadAll(f) | ||
| if err != nil { | ||
| return failed(err) | ||
| } |
There was a problem hiding this comment.
should we close the file here after reading from it instead of relying on the closers?
There was a problem hiding this comment.
Great point, will fix.
Since we're eagerly reading configuration files now, we don't need to keep a slice of `io.Closers`. This also simplifies `NewYAML`.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We're currently handling empty (or comment-only) and null YAML sources
incorrectly. This diff fixes both by simplifying the configuration
provider even further.
These sources are problematic for two reasons. First, we're not
special-casing
io.EOFerrors when we useyaml.Decoder. Moreinsidiously, when we unmarshal sources, we're not distinguishing between
nil values caused by empty (or comment-only) sources and explicit
top-level nulls. The latter is particularly problematic because an
explicit null should be higher-priority than all defaults provided via
WithDefault, even if that null is itself overriden by another YAMLsource. (For example, if
base.yamlis just~andproduction.yamlis
foo: bar, the explicit top-level null inbase.yamlshould trumpany data supplied by
WithDefault.)This commit fixes these issues by making three interrelated changes:
io.Readers, andchanges the internal representation of YAML sources to byte slices
instead of readers. This simplifies later changes, since we'll be
consuming each source multiple times.
io.EOFerrors where appropriate, taking care todistinguish empty sources and explicit nulls.
WithDefault's internals to match the documentation: theimplementation literally uses the user-supplied value as the
lowest-priority search and deep-merges it with all other sources.