fix panic when passing nil to time.Time.In() #1100
Conversation
| if scd.sc != nil && scd.sc.cfg != nil { | ||
| return getCurrentLocation(scd.sc.cfg.Params) | ||
| } |
There was a problem hiding this comment.
is it possible that Params is empty or something that will return a bad value for time.Location?
There was a problem hiding this comment.
if getCurrentLocation is called something non nil will be returned, updated the else option to also return time.Now.Location so we dont ever return nil
converter.go
Outdated
| if loc == nil { | ||
| loc = time.Now().Location() | ||
| } |
There was a problem hiding this comment.
should these just be time.Location? i.e. avoid the Now() call.
There was a problem hiding this comment.
afaik time.Location{} if not generated by time.Now() is an empty struct which will panic without the name string
There was a problem hiding this comment.
loc is initialized two lines above - so this condition is always false, am I right?
| @@ -671,10 +671,10 @@ type snowflakeArrowStreamChunkDownloader struct { | |||
| } | |||
|
|
|||
| func (scd *snowflakeArrowStreamChunkDownloader) Location() *time.Location { | |||
| if scd.sc != nil { | |||
| if scd.sc != nil && scd.sc.cfg != nil { | |||
There was a problem hiding this comment.
This change is not a mistake, but I'm wondering - you actually encountered this situation? By looking at the code I can see that when snowflakeConn is initialised, it is always initialised with not nil config.
There was a problem hiding this comment.
I have not experienced this, I can remove it I just wanted to ensure that we dont null pointer dereference in any possible way
| return getCurrentLocation(scd.sc.cfg.Params) | ||
| } | ||
| return nil | ||
| return time.Now().Location() |
There was a problem hiding this comment.
Similarly - looking at the code, we should never reach this line, scd.sc is always initalised...
This line is much more tricky anyway. This location may be completely different than snowflake session timezone, meaning it may break LTZ contracts.
There was a problem hiding this comment.
Ideally we should never hit this line but it seems that we are hitting it. I personally feel like this is a good temporary fix until we figure out why the connection is nil because having a panic seems worse to me but I am open to any other solutions
converter.go
Outdated
| if loc == nil { | ||
| loc = time.Now().Location() | ||
| } |
There was a problem hiding this comment.
loc is initialized two lines above - so this condition is always false, am I right?
Description
SNOW-XXX Please explain the changes you made here.
When time.Time.In is used, if a nil is passed in it will panic. (See docs) In the other functions in converter.go (see stringToValue timestamp_ltz) if the loc is nil a default will be used.
Checklist