[improvement] Safe log missing parameters#125
Merged
Conversation
iamdanfox
reviewed
Dec 11, 2018
| compile 'com.google.guava:guava' | ||
| compile 'com.palantir.conjure.java.api:errors' | ||
| compile 'com.palantir.ri:resource-identifier' | ||
| compile 'com.palantir.safe-logging:preconditions' |
Contributor
There was a problem hiding this comment.
I think we'd need to add this to the :conjure-lib project to make sure the class is available for all users.
iamdanfox
reviewed
Dec 11, 2018
|
|
||
| integrationInputCompile project(':conjure-lib') | ||
| integrationInputCompile 'com.palantir.conjure.java.api:errors' | ||
| integrationInputCompile 'com.palantir.safe-logging:preconditions' |
Contributor
There was a problem hiding this comment.
I don't want people to have to manually add this dependency!
iamdanfox
reviewed
Dec 11, 2018
Contributor
iamdanfox
left a comment
iamdanfox
left a comment
There was a problem hiding this comment.
This looks great! The unnecessary redaction was irritating before.
Contributor
|
Only downside I can see is this does bloat all conjure-generated projects slightly by adding the |
carterkozak
reviewed
Dec 28, 2018
| .hasMessage("Some required fields have not been set: [binary]"); | ||
| assertThatLoggableExceptionThrownBy(() -> mapper.readValue("{}", BinaryExample.class).getBinary()) | ||
| .isInstanceOf(SafeIllegalArgumentException.class) | ||
| .hasMessage("Some required fields have not been set") |
Contributor
There was a problem hiding this comment.
After safe-logging is update this assertion will fail because it checks the rendered throwable message rather than the safe log message.
Contributor
|
Love it |
Contributor
|
I've updated this PR, should be good to go. |
ferozco
approved these changes
Jan 9, 2019
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Before this PR
Errors during object construction would throw an
IllegalArgumentExceptionwith the missing fields included in the message. This message is excluded when the logs are collected.After this PR
Throw a
SafeIllegalArgumentExceptionwith the missing fields in aSafeArgso the missing fields are included during collection.