fix(core): Fix some NPEs in validation and config generation #31
Conversation
In a few places in the config validation and translation we were dereferencing a potentially nil pointer. This happens when we we need to reach more than a level deep in the HalConfig and that top-level field is nil. A while back we discussed how to handle this, and considered seeing if we could deserialize as values instead of pointers so we could always rely on the zero value. But if I recall we decided against that because: (1) It is actually useful to have the nil value because it indicates something was not present in the config and should not be serialized to the service config (2) This problem will only happen in cases where we're reaching into the hal config, so the amount of places we need to do this nil check should be reasonable. Looking at the generated protos, it looks like there are actually nil-safe accessor generated. If you call x.GetY() it will first check if x is nil, and if so return nil; if not it will return x.y. This took a minute for me to wrap my Java brain around, because the (bad) analogy is basically an instance method on an object checking whether the object is nil. But somehow this works for receivers (I guess because type inference means we know the type of the receiver so which function to call, and we just get the receiver as a possibly-nil object). But this is really nice because it lets you write nil-safe chains as x.GetY().GetZ() that return nil at the end if anything was nil but never throw a nil-pointer panic. Also add some test to the validation and parsing modules to validate correct behavior in various edge cases (empty hal config, hal config with providers field but no providers, etc.)
For a while we weren't sure whether to pass by value or by reference. Given that we're passing around structs whose nested fields are all themselves passed around by reference, it seems logical to do the same at the top level (as it's confusing that the top-level thing is sometimes passed around by value). This also resolves a bunch of 'go vet' warnings because the generated structs contain a mutex somewhere, which really should not be copied, and generates a warning when we pass these around by value. If I didn't think that it probably makes sense to pass by value anyway I'd probably spend more time seeing if that warning shoudl be suppressed, but given that I think it makes sense anyway to pass by value, let's do that.
Wow, amazing discovery! Maybe Typescript optional chaining is a more useful analogy?
Just want to make sure you meant "reference" instead of value in the last sentence of the second commit; otherwise I understand nothing 🤡 |
|
Ha, good catch on my typo---yes I did mean that we should pass by "reference" so you are understanding correctly! |
| "github.com/spinnaker/kleat/api/client" | ||
| ) | ||
|
|
||
| func TestEmptyHalConfigToClouddriver(t *testing.T) { |
There was a problem hiding this comment.
I wonder if we'd ever want to refactor these as table driven tests just to emphasize what's actually different about each one and make it clear which cases we'd expect to be added for each new service. On the other hand we'd lose your very nice description of each case. 🤷
There was a problem hiding this comment.
That's a good thought! Let me read about table driven tests! These tests were mostly written by seeing what you did for the end-to-end tests in this repo then working from there, so I have a lot to learn about testing in go.
fix(core): Fix some NPEs in validation and config generation
In a few places in the config validation and translation we were dereferencing a potentially nil pointer. This happens when we we need to reach more than a level deep in the HalConfig and that top-level field is nil.
A while back we discussed how to handle this, and considered seeing if we could deserialize as values instead of pointers so we could always rely on the zero value. But if I recall we decided against that because:
(1) It is actually useful to have the nil value because it indicates something was not present in the config and should not be serialized to the service config
(2) This problem will only happen in cases where we're reaching into the hal config, so the amount of places we need to do this nil check should be reasonable.
Looking at the generated protos, it looks like there are actually nil-safe accessor generated. If you call x.GetY() it will first check if x is nil, and if so return nil; if not it will return x.y. This took a minute for me to wrap my Java brain around, because the (bad) analogy is basically an instance method on an object checking whether the object is nil. But somehow this works for receivers (I guess because type inference means we know the type of the receiver so which function to call, and we just get the receiver as a possibly-nil object). But this is really nice because it lets you write nil-safe chains as x.GetY().GetZ() that return nil at the end if anything was nil but never throw a nil-pointer panic.
Also add some test to the validation and parsing modules to validate correct behavior in various edge cases (empty hal config, hal config with providers field but no providers, etc.)
refactor(core): Standardize on using pointers
For a while we weren't sure whether to pass by value or by reference. Given that we're passing around structs whose nested fields are all themselves passed around by reference, it seems logical to do the same at the top level (as it's confusing that the top-level thing is sometimes passed around by value).
This also resolves a bunch of 'go vet' warnings because the generated structs contain a mutex somewhere, which really should not be copied, and generates a warning when we pass these around by value. If I didn't think that it probably makes sense to pass by value anyway I'd probably spend more time seeing if that warning should be suppressed, but given that I think it makes sense anyway to pass by reference, let's do that.