Enable typed nil in versioning ramp #1599
Conversation
| @@ -419,8 +422,10 @@ func (r *VersioningAssignmentRule) validateRule() error { | |||
| } | |||
| switch ramp := r.Ramp.(type) { | |||
| case *VersioningRampByPercentage: | |||
| if err := ramp.validateRamp(); err != nil { | |||
| return err | |||
| if ramp != nil { | |||
There was a problem hiding this comment.
Can me move the nil check out of the switch so when we add more Ramp types we don't need to duplicate this check?
There was a problem hiding this comment.
Ramp is an interface and to know that a non-nil interface contains a nil pointer I need to type cast. Can't think of a way to do that without repeating the type cast for each concrete type, kind of defeating the purpose of lifting the check...
| @@ -139,3 +139,23 @@ func Test_VersioningIntent(t *testing.T) { | |||
| }) | |||
| } | |||
| } | |||
|
|
|||
| func Test_WorkerVersioningRules_typed_nil(t *testing.T) { | |||
| ramp := &VersioningRampByPercentage{ | |||
There was a problem hiding this comment.
| ramp := &VersioningRampByPercentage{ | |
| var ramp *VersioningRampByPercentage |
I think this should be equivalent and show what your trying to test a bit clearer
There was a problem hiding this comment.
Yes, done, I was doing cut/paste from another example...
| RampPercentage: r.Percentage, | ||
| }, | ||
| // Ramp is optional, checking for typed `nil` | ||
| if r != nil { |
There was a problem hiding this comment.
If the type switch passes but this is nil, I think this the Ramp value is incorrectly being set. The Ramp value should be nil or a non-nil instance, it should not be a non-nil interface with nil value. Code is going to struggle to use this if rule.Ramp == nil does not work.
There was a problem hiding this comment.
This was reported by a customer that wrote some innocent looking code that was failing:
r := &VersioningRampByPercentage{
...
}
if (some condition) {
r = nil
}
I think this should be valid code, otherwise we are really going to confuse people...
But yes, coming from other languages it took me by surprise that nil has type...
There was a problem hiding this comment.
that wrote some innocent looking code that was failing:
It's not that code exactly. This only happens if you are taking a pointer to a nil variable and assigning it to an interface. See https://go.dev/play/p/F_uAKDduIau.
nilhas type...
It does not usually have a type unless it is not being set properly on the interface. What's happening is the interface is non-nil but its implementation is nil. This is a common Go confusion. We don't usually explicitly handle this.
What will happen today if you provide a pointer to a nil *ScheduleWorkflowAction in the Schedule.Action parameter? It will panic, like this code does, and like all of our other places where we accept an interface and people improperly set it to a non-nil value w/ a nil impl.
Your change can work for this one case for that one user that improperly sets the interface to a non-nil-yet-invalid value, but it's inconsistent with all other handling of interfaces we do.
There was a problem hiding this comment.
This is the example shared by the customer https://play.golang.com/p/hhUikD1wDvs , yes, you need to do a bit more than that...
There was a problem hiding this comment.
Right, this is taking a pointer of a nil variable and assigning it to an interface. This is a known Go situation and in the places we use interfaces, we don't test for nil implementations, just actual nil values. This same issue would happen if a nil struct pointer were provided to something expecting an interface in many parts of our SDK such as schedules or interceptors or something, not just here.
There was a problem hiding this comment.
Do we document this somewhere, or this is just standard " good Go API behavior"? The nil struct pointer passed to a function that takes an interface looks like a foot gun...
There was a problem hiding this comment.
I think this is a standard Go confusion/problem and a standard Go footgun. It is definitely not specific to Temporal and surprising to many users that encounter it the first time. If we want, and @Quinn-With-Two-Ns is ok with, handling this specific interface nilness in this case we can, but in general we never have and most libraries do not and most libraries do not document this Go behavior because it is known (even if it is a footfun).
There was a problem hiding this comment.
@cretz Said customer from above here.
It's not that code exactly.
It is exactly that code which lead to the nil access prompting my original report. The ramp := &VersioningRampByPercentage creates the variable with *VersioningRampByPercentage type (which becomes a typed nil once passed within VersioningAssignmentRule if it was nil).
In the original report ( https://temporalio.slack.com/archives/CTRCR8RBP/p1723805024798129?thread_ts=1714989745.540629&cid=CTRCR8RBP ) I also mentioned "And I cannot declare ramp of type internal.VersioningRamp to avoid that, because it's not re-exported by client afaics."
What was changed
This fixes a versioning bug in the latest implementation.
See #1598 for details
Typed nil versioning ramp panics #1598
Unit test