Fix: check if target is valid by mtojek · Pull Request #160 · elastic/go-ucfg

mtojek · 2020-04-09T16:18:28Z

This PR fixes the issue reported in #159 .

Check if the target is valid, before setting the value. The target field will stay nil.

urso · 2020-04-10T11:43:10Z

reify.go

-	to.Set(pointerize(to.Type(), v.Type(), v))
+	if v.IsValid() {
+		to.Set(pointerize(to.Type(), v.Type(), v))
+	}


Given the test case I don't see why v can be invalid. Do we have an original issue with sample code? v should always be assignable to to, and return a zero value at least.

As reifyGetField is an internal function processing a sub-tree only, I wonder if the issue can occur in other edge cases as well.

Do we have an original issue with sample code?

This is the exact extracted part from the linked PR in the issue. Just unpack the given YAML file into a structure with interface{}

What I'm wondering about is, if this or a similar error can occur when we unpack into some alternative type. For example if one unpacks into a map, then the call to.SetMapIndex(key, v) at line 224 would be the equivalent for maps. For arrays/slices the equivalent call is to.Index(idx).Set(v) in reifyDoArray. Both calls do not check for an invalid reflect.Value yet. Given the bug at hand, I doubt the other 2 cases have tests that might show an error.

I've added more test cases, but couldn't reproduce. If you see any potential weird structure that can cause an error, I can add a test case.

Other way round is to wrap all to.Set() calls with a condition checking if the target is valid.

If the tests show that we are fine, I'm ok with that.

maybe we can add these combinations for maps as well:

map[string]interface{}{} , `{"a": []}` map[string]interface{}{"a": nil} , `{"a": []}` map[string][]interface{}{}, `{"a": []}` map[string][]interface{}{"a": nil}, `{"a": []}`

and for arrays:

[]interface{} , `[[]]` [][]interface{}, `[[]]`

I've updated test cases, but I think we need to review/accept the final unmarshaled forms (I'm referring specifically to this case: map[string]interface{}{"a": nil} where the field disappears).

Hm, the field a should not just disappear. If the input configuration does not contain the field, or if it is an empty array, then it should not be overwritten. We want to merge the configuration into the object given. Yet one can discuss what it means to merge an empty array... But it kinda looks like we just found another bug.
I don't want increase the scope for this PR any further. How about skipping this tests that do remove a and create a follow up issue?

I think I've just fixed this case by checking if the target is valid:

if v.IsValid() { to.SetMapIndex(key, v) }

urso · 2020-04-10T11:44:08Z

yaml/yaml_test.go

+	assert.Nil(t, verify.Vars[0].Default)
+	assert.Nil(t, verify.Vars[1].Default)
+	assert.Equal(t, uint64(3), verify.Vars[2].Default)
+	assert.Len(t, verify.Vars[3].Default.([]interface{}), 3)


Can we split this up into 4 test cases (e.g. table driven test), or is the error only triggered by this setup?

Was the error triggered for each of these cases?

Let's add a sample test with an empty array as has been shown in the issue.

Good catch. It seems that an empty array is also an issue. I simplified the test case.

urso · 2020-04-10T11:45:14Z

yaml/yaml_test.go

+
+type variable struct {
+	Default interface{}
+}


these types are unique to the test case only. Consider moving the type declaration into TestStructEmptyArray

urso · 2020-04-10T11:48:30Z

Is this error reproducible if we just unpack into an array, instead of an array within a struct?

mtojek · 2020-04-10T12:20:58Z

Is this error reproducible if we just unpack into an array, instead of an array within a struct?

I replied above. It's reproducible - I simplified the test case, a single line seems to be enough.

ruflin · 2020-04-14T06:50:58Z

Should we add a chanelog entry?

urso · 2020-04-14T07:57:07Z

Should we add a chanelog entry?

Yes, please.

mtojek · 2020-04-14T08:28:12Z

Should we add a chanelog entry?

Yes, please.

Fixed.

mtojek · 2020-04-14T10:23:43Z

I think we might have hit also this: golang/go@201cb04

codecov-io · 2020-04-14T10:49:19Z

Codecov Report

Merging #160 into master will decrease coverage by 0.10%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #160      +/-   ##
==========================================
- Coverage   78.63%   78.52%   -0.11%     
==========================================
  Files          24       24              
  Lines        2822     2827       +5     
==========================================
+ Hits         2219     2220       +1     
- Misses        440      445       +5     
+ Partials      163      162       -1

Impacted Files	Coverage Δ
reify.go	`69.69% <100.00%> (+0.35%)`	⬆️
types.go	`73.92% <0.00%> (-0.93%)`	⬇️
merge.go	`79.01% <0.00%> (-0.62%)`	⬇️
variables.go	`80.28% <0.00%> (+0.35%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a138d3e...9ca7066. Read the comment docs.

This removes testify/assert from only one test file in the ucfg package, and it brings in go-spew to improve the readability of test failures.

urso · 2020-04-14T16:45:27Z

error_test.go

+	adjusted = strings.Replace(adjusted, "\" in duration \"", " in duration ", 1)
+	adjusted = strings.Replace(adjusted, "\" accessing", " accessing", 1)
+	return adjusted
+}


Which go version failed here? For go tip only? If we need it for tip and it is a known error in tip to be fixed soon I would ignore it for now. We check with tip to see if we might be hit by a change in reflection in the stdlib early in time.

If it fails for a released go version let's add a conditional.

Oh, it's a bug in go1.14 :(

Checking the travis file, I noticed 1.14 is still missing.

In this case it was "devel". I can adjust the Travis file too, unless you think this should go in a separate PR.

Hm, if it's just tip I'd remove the function altogether. Although I'd prefer to add go 1.14 in another PR, I'd like to have some assurance that 1.14 does not contain the bug :)

Let me prepare a PR for this.

urso · 2020-04-14T16:58:38Z

yaml/yaml_test.go

+	err = c.Unpack(&verify)
+	assert.Nil(t, err)
+	assert.Equal(t, map[string]interface{}{}, verify)
+}


there is quite some repetition here. How about moving the test cases into a table like so?

cases := map[string]struct{ input string to interface{} want interface{} }{ "empty map into map of interfaces": { input: "{}", to: &map[string]interface{}{}, want: &map[string]interface{}{}, }, ... } for name, test := range cases { t.Run(name, func(t *testing.T) { c, err := MustNewConfig(byte(test.input)) mustUnpack(t, c, test.to) assert.Equal(t, test.want, test.to) }) }

mustUnpack should be available already if you rebase.

Rebased and refactored as it wasn't exposed.

urso · 2020-04-15T11:22:48Z

error_test.go

 			golden := string(tmp)
-			assert.Equal(t, golden, message)
+			message = adjustMessageFormat(message)
+			assert.Equal(t, golden, message, "Go runtime version: %s", runtime.Version())


reify.go

cfgtest/unpack.go

This adds a special case for empty arrays when reifying configuration objects during Unpack. This is a breaking change compared to existing behaviour as visible in the test cases I adjusted. The original issue #160 that introduced said tests is however still addressed. Closes #188

Fix: check if target is valid

b87b280

mtojek requested review from a team and urso April 9, 2020 16:18

mtojek self-assigned this Apr 9, 2020

urso reviewed Apr 10, 2020

View reviewed changes

Simplify test case

aeb042e

mtojek requested a review from urso April 10, 2020 12:21

andresrc added [zube]: Inbox Team:Elastic-Agent Label for the Agent team [zube]: In Review and removed [zube]: Inbox labels Apr 13, 2020

Update CHANGELOG

4926667

Add more test cases

f54eb94

mtojek added 2 commits April 14, 2020 12:37

Fix: adjust error message format

b2647ef

Fix: adjust error message format

ecef790

Remove testify/assert from Validator Tests (elastic#158)

59731a8

This removes testify/assert from only one test file in the ucfg package, and it brings in go-spew to improve the readability of test failures.

urso reviewed Apr 14, 2020

View reviewed changes

mtojek added 2 commits April 15, 2020 13:07

Prepare test cases as table

f1122d8

Print runtime version

75502b8

urso reviewed Apr 15, 2020

View reviewed changes

mtojek added 12 commits April 15, 2020 13:39

Fix: include devel runtime

2b208dc

Fix: check if target is valid

3a1258b

Simplify test case

f425c75

Update CHANGELOG

77ff455

Add more test cases

9c99b0a

Fix: adjust error message format

dca63cc

Fix: adjust error message format

0ec50a4

Prepare test cases as table

c4a6cd8

Print runtime version

1b6b177

Fix: include devel runtime

a3344a1

Merge branch 'fix-159' of https://github.com/mtojek/go-ucfg into fix-159

cd553cd

Reuse mustUnpack

76adfaa

houndci-bot reviewed Apr 15, 2020

View reviewed changes

reify.go Show resolved Hide resolved

cfgtest/unpack.go Show resolved Hide resolved

cfgtest/unpack.go Show resolved Hide resolved

mtojek added 2 commits April 15, 2020 14:13

Fix: hound

52bc843

Fix: do not override property

5268169

mtojek requested a review from urso April 15, 2020 12:27

urso approved these changes Apr 15, 2020

View reviewed changes

mtojek added 2 commits April 15, 2020 15:13

Fix: adjust error message in devel only

9ca7066

Fix: don't store runtime error messages in golden files

07b25b0

mtojek merged commit 1232bd4 into elastic:master Apr 15, 2020

zube bot added [zube]: Done and removed [zube]: In Review labels Apr 15, 2020

andresrc removed the [zube]: Done label Apr 27, 2020

pebrc mentioned this pull request Oct 15, 2021

Preserve empty untyped arrays when reifying #189

Merged

Conversation

mtojek commented Apr 9, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

urso commented Apr 10, 2020

Uh oh!

mtojek commented Apr 10, 2020

Uh oh!

ruflin commented Apr 14, 2020

Uh oh!

urso commented Apr 14, 2020

Uh oh!

mtojek commented Apr 14, 2020

Uh oh!

mtojek commented Apr 14, 2020

Uh oh!

codecov-io commented Apr 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

codecov-io commented Apr 14, 2020 •

edited

Loading