Correctly validate the nil check for interfaces.

[vijtrip2]

I found this bug while testing newly generated apigatewayv2 controller where the update code path was not triggered because no delta was being reported for an updated api resource.
The root cause was that string pointers were not being checked correctly for nil value.

---

To perform nil check on interfaces, only doing i == nil is not sufficient as pointed out in

• https://tour.golang.org/methods/12
• https://mangatmodi.medium.com/go-check-nil-interface-the-right-way-d142776edef1

---

• I updated the nil check to check for value stored in the interface type as suggested by links above and then confirmed that delta path for apigatewayv2 controller was correctly working.
• I also added some unit tests for nil checks on interface types that can have nil values.

[a-hilaly]

We totally missed that Go have typed nils.. Great catch on this @vijtrip2 ! but i still think that we should avoid using reflect as much as possible. Especially in an area that is related to the controllers reconciliation. I believe that we might see decreasing performance in ACK controllers in the future.

IMO it would be better to solve this issue at the "delta code generation" level, by generating nil checking functions for each type, and implementating nil checking functions for built-in types in the runtime/compare package.

Also there is a change that Go 1.17 will bring generics :) which can be a very good solution for this issue.

[a-hilaly]

A small experiment to compare the performance of == nil with reflect.ValueOf().IsNil():

package main_test

import (
	"reflect"
	"testing"
)

func isNilReflect(i interface{}) bool { return reflect.ValueOf(i).IsNil() }
func isNilEqualOp(s *string) bool     { return s == nil }

func Benchmark_reflection(b *testing.B) {
	var s *string
	v := interface{}(s)
	for i := 0; i < b.N; i++ {
		isNilReflect(v)
	}
}
func Benchmark_equalOp(b *testing.B) {
	var s *string
	for i := 0; i < b.N; i++ {
		isNilEqualOp(s)
	}
}

goos: linux
goarch: amd64
Benchmark_reflection-24         305637628                3.76 ns/op
Benchmark_equalOp-24            1000000000               0.247 ns/op
PASS
ok      command-line-arguments  1.834s

[a-hilaly]

The benchmark results are not very concerning i believe, also i just found out that reflect.ValueOf doesn't do any memory allocations. I'm Ok with merging this 👍 and seeing the mem/cpu profiling results in the soaking tests

[jaypipes]

is this function really necessary? :)

[vijtrip2]

I thought it read well this way. I personally do not like logical operators being consecutive to each other
like && !isNil()
Saying that I also do not have strong preference to keep it this way.

[jaypipes]

Funnily enough, I'd originally written the pkg/compare/nil.go code to have a set of HasNilStringDifference, HasNilIntDifference, etc functions one for each type and then combined them all into this HasNilDifference method when I realized that other than the function parameter types, the function implementation was identical :)

Seems I should have stuck to that original idea!

[ack-bot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: A-Hilaly, jaypipes, RedbackThomson, vijtrip2

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment