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

feat: add support for custom ReflectType encoder #1039

Merged
merged 8 commits into from Dec 15, 2021

Conversation

psrajat
Copy link
Contributor

@psrajat psrajat commented Dec 14, 2021

Hi Team!
This fixes #1034

@CLAassistant
Copy link

CLAassistant commented Dec 14, 2021

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Dec 14, 2021

Codecov Report

Merging #1039 (c65627f) into master (10b1fe4) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1039   +/-   ##
=======================================
  Coverage   98.20%   98.21%           
=======================================
  Files          47       48    +1     
  Lines        2067     2071    +4     
=======================================
+ Hits         2030     2034    +4     
  Misses         29       29           
  Partials        8        8           
Impacted Files Coverage Δ
zapcore/encoder.go 87.09% <ø> (ø)
zapcore/json_encoder.go 100.00% <100.00%> (ø)
zapcore/reflected_encoder.go 100.00% <100.00%> (ø)
zapcore/sampler.go 100.00% <0.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 10b1fe4...c65627f. Read the comment docs.

@@ -331,6 +332,9 @@ type EncoderConfig struct {
// Unlike the other primitive type encoders, EncodeName is optional. The
// zero value falls back to FullNameEncoder.
EncodeName NameEncoder `json:"nameEncoder" yaml:"nameEncoder"`
// Configure the encoder for interface{} type objects
// If not provided, objects are encoded using json.Encoder
NewReflectedEncoder func(io.Writer) ReflectedEncoder `json:"newReflectedEncoder" yaml:"newReflectedEncoder"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this a function that is not JSON/YAML encodeable, we'll have to use - here.

Suggested change
NewReflectedEncoder func(io.Writer) ReflectedEncoder `json:"newReflectedEncoder" yaml:"newReflectedEncoder"`
NewReflectedEncoder func(io.Writer) ReflectedEncoder `json:"-" yaml:"-"`

var newReflectedEncoder func(io.Writer) ReflectedEncoder
if enc.NewReflectedEncoder == nil {
newReflectedEncoder = GetDefaultReflectedEncoder()
} else {
newReflectedEncoder = enc.NewReflectedEncoder
}
Copy link
Contributor

Choose a reason for hiding this comment

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

We should do this in NewJSONEncoder? We can set NewReflectedEncoder in there once if it's nil, and then we don't have to check it on the hot path.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@abhinav
This change was breaking TestJSONEncoderObjectFields test cases in json_encoder_impl_test.go so had to fix it by manually assigning default reflected encoder.
LMK if there are any concerns.

if enc.NewReflectedEncoder == nil {
newReflectedEncoder = GetDefaultReflectedEncoder()
} else {
newReflectedEncoder = enc.NewReflectedEncoder
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you please add a test for the custom reflection encoder case?
We don't want us to introduce a new dependency for that,
so a fake implementation of the function will suffice.

)

// A ReflectedEncoder handles the serialization of Field which have FieldType = ReflectType and writes them to the underlying data stream
// The underlying data stream is provided by zap during initialization. See EncoderConfig.NewReflectedEncoder
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// The underlying data stream is provided by zap during initialization. See EncoderConfig.NewReflectedEncoder
// The underlying data stream is provided by Zap during initialization. See EncoderConfig.NewReflectedEncoder.

// A ReflectedEncoder handles the serialization of Field which have FieldType = ReflectType and writes them to the underlying data stream
// The underlying data stream is provided by zap during initialization. See EncoderConfig.NewReflectedEncoder
type ReflectedEncoder interface {
// Encode encodes and writes to the underlying data stream
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Encode encodes and writes to the underlying data stream
// Encode encodes and writes to the underlying data stream.

Encode(interface{}) error
}

func GetDefaultReflectedEncoder() func(writer io.Writer) ReflectedEncoder {
Copy link
Contributor

Choose a reason for hiding this comment

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

We would prefer not to export this function. We also don't need the wrapper type because json.Encoder satisfies the ReflectedEncoder interface.

So something like the following should suffice:

func defaultReflectedEncoder(w io.Writer) ReflectedEncoder {
  enc := json.NewEncoder(w)
  enc.SetEscapeHTML(false)
  return enc
}

psrajat and others added 4 commits December 15, 2021 00:09
* fix comments
* don't export default ReflectedEncoder fn
* move newReflectEncoder nil check to NewJsonEncoder()
The defaultReflectedEncoder function can satisfy the signature for
`NewReflectedEncoder`, so we don't need to return a closure.

And the returned `json.Encoder` matches the ReflectedEncoder interface
so we don't need the wrapper type either.
assertOutput manually sets up a jsonEncoder.
This means that any config initalization we do in the constructor has to
be replicated in assertOutput.
Instead, use the constructor and cast to `*jsonEncoder`.
zapcore/json_encoder_test.go Outdated Show resolved Hide resolved
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
buf, err := enc.EncodeEntry(zapcore.Entry{
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
buf, err := enc.EncodeEntry(zapcore.Entry{
for _, tt := range tests {
tt := tt
t.Run(tt.name, func(t *testing.T) {
t.Parallel()
buf, err := enc.EncodeEntry(zapcore.Entry{

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, we can do that, although for that the encoder has to be instantiated per-test.
(Encoder isn't safe for concurrent use.)
I'll do it.

abhinav and others added 2 commits December 14, 2021 16:56
@abhinav abhinav merged commit 369c1bd into uber-go:master Dec 15, 2021
7 checks passed
@abhinav
Copy link
Contributor

abhinav commented Dec 15, 2021

Thanks!

@abhinav abhinav mentioned this pull request Jan 4, 2022
abhinav added a commit that referenced this pull request Jan 4, 2022
This release v1.20.0 of Zap with a couple new features for customizing
the JSON encoder. Namely,

- support skipping newlines between log statements (#989)
- support changing the reflection JSON encoder (#1039)

Refs GO-1085
abhinav added a commit that referenced this pull request Jan 4, 2022
This release v1.20.0 of Zap with a couple new features for customizing
the JSON encoder. Namely,

- support skipping newlines between log statements (#989)
- support changing the reflection JSON encoder (#1039)

Refs GO-1085
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.

feat: Custom interface{} object Encoder
4 participants