Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

json: Don't panic for nil Encode{Time, Duration} #835

Merged
merged 1 commit into from
Jun 10, 2020

Conversation

abhinav
Copy link
Collaborator

@abhinav abhinav commented Jun 4, 2020

Fixes #834

The JSON encoder assumes that encoders for time.Time and
time.Duration are always specified, which causes nil pointer
dereference panics.

Fix this by treating nil encoders for time and duration as no-ops. This
will fall back to existing logic in the JSON encoder that handles no-op
time and duration encoders.

Fixes #834

The JSON encoder assumes that encoders for `time.Time` and
`time.Duration` are always specified, which causes nil pointer
dereference panics.

Fix this by treating nil encoders for time and duration as no-ops. This
will fall back to existing logic in the JSON encoder that handles no-op
time and duration encoders.
@abhinav abhinav requested a review from prashantv June 4, 2020 16:43
@codecov
Copy link

codecov bot commented Jun 4, 2020

Codecov Report

Merging #835 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #835   +/-   ##
=======================================
  Coverage   98.33%   98.33%           
=======================================
  Files          43       43           
  Lines        2347     2349    +2     
=======================================
+ Hits         2308     2310    +2     
  Misses         32       32           
  Partials        7        7           
Impacted Files Coverage Δ
zapcore/json_encoder.go 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e931a6b...c6cdc02. Read the comment docs.

Copy link
Collaborator

@prashantv prashantv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks related to #791 where we chose to error out on creation if the time key is set, but there was no time encoder.

I think the behavioue we'll end up with is:

  • If the time key is specified, a time encoder must be specified.
  • If no time key is specified, and there's no time encoder, then times will be encoded with a default

Should we have the same default behaviour even when a time key is set?

@abhinav
Copy link
Collaborator Author

abhinav commented Jun 9, 2020

Before we change the default behavior, I want to clarify that we have
two API contracts under consideration here.

  • For encoders registered with zap.RegisterEncoder, the expectation
    since EncoderConfig and AtomicLevel validation added #791 is that zap.Config.Build will not invoke the constructor
    with a nil EncodeTime if TimeKey is not set.
  • The breakage fixed in this change is a bug in
    zapcore.NewJSONEncoder: it did not handle a nil EncodeTime or
    EncodeDuration regardless of the value of TimeKey.

We can change Config.Build's behavior to set EncodeTime to a non-
nil value when TimeKey is set, and to set EncodeDuration to a non-
nil value if unset. However, this default would apply to all encoders,
including custom encoders. Are we sure we want this to be the default
behavior for all encoders?

Copy link
Collaborator

@prashantv prashantv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good point, seems reasonable that different encoders may have different defaults.

@abhinav abhinav merged commit 3640f92 into master Jun 10, 2020
@abhinav abhinav deleted the abg/json-time-duration-panics branch June 10, 2020 18:21
cgxxv pushed a commit to cgxxv/zap that referenced this pull request Mar 25, 2022
Fixes uber-go#834

The JSON encoder assumes that encoders for `time.Time` and
`time.Duration` are always specified, which causes nil pointer
dereference panics.

Fix this by treating nil encoders for time and duration as no-ops. This
will fall back to existing logic in the JSON encoder that handles no-op
time and duration encoders.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants