Skip to content

Use standard object writer for errors#24

Merged
RaHery merged 1 commit intomasterfrom
standard_writer_for_errors
Jun 25, 2019
Merged

Use standard object writer for errors#24
RaHery merged 1 commit intomasterfrom
standard_writer_for_errors

Conversation

@clementdenis
Copy link

No description provided.

@clementdenis clementdenis requested review from alarribeau and llbrt June 19, 2019 13:45
@codecov-io
Copy link

codecov-io commented Jun 19, 2019

Codecov Report

Merging #24 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master      #24      +/-   ##
============================================
+ Coverage     80.95%   80.97%   +0.02%     
- Complexity     1721     1723       +2     
============================================
  Files           159      159              
  Lines          6395     6397       +2     
  Branches        830      831       +1     
============================================
+ Hits           5177     5180       +3     
  Misses          907      907              
+ Partials        311      310       -1
Impacted Files Coverage Δ Complexity Δ
.../server/spi/response/RestResponseResultWriter.java 100% <100%> (ø) 6 <0> (ø) ⬇️
...rver/spi/response/ServletResponseResultWriter.java 95.29% <100%> (+0.11%) 19 <0> (+1) ⬆️
.../server/spi/discovery/CommonPathPrefixBuilder.java 100% <0%> (+5.55%) 9% <0%> (+1%) ⬆️

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 0cc6b91...09f5089. Read the comment docs.

Copy link

@RaHery RaHery left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @alarribeau, @clementdenis, and @llbrt)


endpoints-framework/src/main/java/com/google/api/server/spi/response/ServletResponseResultWriter.java, line 96 at r1 (raw file):

    }
    this.objectWriter = configureWriter(objectWriter);
    this.errorObjectWriter = objectWriter;

The method configureObject(..) is not a pure function: it may change the state of the input parameter objectWriter. If it does, the errorObjectWriter would inherit the side effect of this change.
It would be safer to assign a different object to errorObjectWriter instead of reusing the same instance.

@clementdenis
Copy link
Author

From the ObjectWriter Javadoc:

new instances are constructed for different configurations

So com.fasterxml.jackson.databind.ObjectWriter instances are effectively immutable, their state can't be changed, so the logic here should be OK.

Copy link

@RaHery RaHery left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @alarribeau, @clementdenis, and @llbrt)

Copy link

@llbrt llbrt left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @alarribeau, @clementdenis, and @llbrt)

Copy link

@llbrt llbrt left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @alarribeau and @clementdenis)

Copy link

@RaHery RaHery left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @alarribeau)

@RaHery RaHery merged commit 748a05a into master Jun 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants