Default value should not be nil #2995
Conversation
types.Marshal expects vars to have non-nil values, this change set empty string as the default value for a referenced aggregate value. This should prevent nil-reference panic in types.Marshal.
|
This is breaking one of the tests. |
|
Yes, I'm still working on that. Thanks |
Value var aggregations use nil values for an optimization, so don't assign a value to defaults.
The marshaling of default value vars to string causes panic because the incoming value is nil. This affects a narrow case so we check that in fact it's a default value var and marshal to empty string. In theory nothing should be nil by the time we Marshal, but it's an edge case.
There was a problem hiding this comment.
https://play.golang.org/p/7Q3UfxhXE11
Let's switch all the conversions to use the second argument, so instead of asserting a certain type, Go would check and respond whether the assert was successful or not.
Reviewable status: 0 of 1 files reviewed, all discussions resolved
|
I can make that change in |
If we are trying to mashal a nil value, return the zero value instead of crashing on nil reference.
|
Much better solution now in place. |
There was a problem hiding this comment.
Reviewed 1 of 1 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @srfrog)
types/conversion.go, line 344 at r3 (raw file):
res := &to.Value // This is a default value from sg.fillVars, don't convert it's empty.
For completeness, can you also check that to is not nil. And if it is, return an error.
There was a problem hiding this comment.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @manishrjain)
types/conversion.go, line 344 at r3 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
For completeness, can you also check that to is not nil. And if it is, return an error.
Done.
* query/query.go: default value should not be nil types.Marshal expects vars to have non-nil values, this change set empty string as the default value for a referenced aggregate value. This should prevent nil-reference panic in types.Marshal. * query/query.go: retract default value change it breaks aggs. Value var aggregations use nil values for an optimization, so don't assign a value to defaults. * types/conversion.go: add exception in Marshal for default->string The marshaling of default value vars to string causes panic because the incoming value is nil. This affects a narrow case so we check that in fact it's a default value var and marshal to empty string. In theory nothing should be nil by the time we Marshal, but it's an edge case. * wrong issue referenced in comments * types/conversion.go: general case in nil value mashaling If we are trying to mashal a nil value, return the zero value instead of crashing on nil reference. * types/conversion.go: replace the original to-value * types/conversion.go: added check for Marshal to nil object
types.Marshal()expects vars to have non-nil values, this changeset empty string as the default value for a referenced empty
var. This should prevent nil-reference panic in
types.Marshal().This change is