🐛 returned objects of reference type should be unchangeable #1851
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
/retest pull-controller-runtime-test-master |
|
@mars1024: The
The following commands are available to trigger optional jobs:
Use In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/test pull-controller-runtime-test-master |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
| @@ -193,8 +193,8 @@ func indexByField(indexer Informer, field string, extractor client.IndexerFunc) | |||
| rawVals := extractor(obj) | |||
| var vals []string | |||
| if ns == "" { | |||
| // if we're not doubling the keys for the namespaced case, just re-use what was returned to us | |||
| vals = rawVals | |||
| // if we're not doubling the keys for the namespaced case, just create a new slice with same length | |||
There was a problem hiding this comment.
Sorry I don't understand. If you write a function that returns a pointer, you are not supposed to keep a reference to that pointer or its underlying value.
There was a problem hiding this comment.
Can you clarify why you are doing that?
There was a problem hiding this comment.
The story is, I compute and put the indexer values into the object status .status.indexerValues, so the extractor will be
func(obj client.Object) []string {
someObject, _ := obj.(*group.ObjectKind)
return someObject.Status.IndexerValues
}
As we know, the object from client cache should be unchangeable.
Just think another scene,
values := []string{"a", "b", "c"}
do(values)
doAgain(values)
Do we expect values will be changed by do? Then doAgain will get unexpected values.
Though slice(extractor return) is a pointer type here, but it is used because it contains immutable values, so I don't think it should be changed when being used.
There was a problem hiding this comment.
Slice(extractor return) here is more like a container containing immutable values(indexer values).
There was a problem hiding this comment.
Okay, can you then please add a test that covers this scenario?
There was a problem hiding this comment.
Of course, will do it ~
Signed-off-by: Bruce Ma <[email protected]>
k8s-ci-robot
added
size/M
|
test case added, PTAL @alvaroaleman @FillZpp , thanks! |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alvaroaleman, mars1024 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
Signed-off-by: Bruce Ma [email protected]
The returned slice of
extractorfunction is reference type, reusing it will bring unpredictable problems, it should be unchangeable.