Skip to content

[fix][break] fix #148: ServiceException.getMessage includes unsafe args #158

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Mar 12, 2019
Merged

[fix][break] fix #148: ServiceException.getMessage includes unsafe args #158

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f81d07a
[fix][break] fix #148: ServiceException.getMessage includes unsafe args
carterkozak Nov 15, 2018
ce40405
Unnecessary to format the unsafe message twice
carterkozak Jan 4, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 12 additions & 14 deletions errors/src/main/java/com/palantir/conjure/java/api/errors/ServiceException.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ public final class ServiceException extends RuntimeException implements SafeLogg
private final List<Arg<?>> args; // unmodifiable

private final String errorInstanceId = UUID.randomUUID().toString();
private final String safeMessage;
private final String unsafeMessage;
private final String noArgsMessage;

/**
Expand All @@ -45,12 +45,12 @@ public ServiceException(ErrorType errorType, Arg<?>... parameters) {
/** As above, but additionally records the cause of this exception. */
public ServiceException(ErrorType errorType, @Nullable Throwable cause, Arg<?>... args) {
// TODO(rfink): Memoize formatting?
super(renderSafeMessage(errorType, args), cause);
super(cause);

this.errorType = errorType;
// Note that instantiators cannot mutate List<> args since it comes through copyToList in all code paths.
this.args = copyToUnmodifiableList(args);
this.safeMessage = renderSafeMessage(errorType, args);
this.unsafeMessage = renderUnsafeMessage(errorType, args);
this.noArgsMessage = renderNoArgsMessage(errorType);
}

Expand All @@ -66,8 +66,8 @@ public String getErrorInstanceId() {

@Override
public String getMessage() {
// Including safe args here since any logger not configured with safe-logging will log this message.
return safeMessage;
// Including all args here since any logger not configured with safe-logging will log this message.
return unsafeMessage;
}

@Override
Expand Down Expand Up @@ -97,7 +97,7 @@ private static <T> List<T> copyToUnmodifiableList(T[] elements) {
return Collections.unmodifiableList(list);
}

private static String renderSafeMessage(ErrorType errorType, Arg<?>... args) {
private static String renderUnsafeMessage(ErrorType errorType, Arg<?>... args) {
String message = renderNoArgsMessage(errorType);

if (args.length == 0) {
Expand All @@ -106,15 +106,13 @@ private static String renderSafeMessage(ErrorType errorType, Arg<?>... args) {

StringBuilder builder = new StringBuilder();
builder.append(message).append(": {");
int consumedArgs = 0;
for (Arg<?> arg : args) {
if (arg.isSafeForLogging()) {
if (consumedArgs++ > 0) {
builder.append(", ");
}

builder.append(arg.getName()).append("=").append(arg.getValue());
for (int i = 0; i < args.length; i++) {
Arg<?> arg = args[i];
if (i > 0) {
builder.append(", ");
}

builder.append(arg.getName()).append("=").append(arg.getValue());
}
builder.append("}");

Expand Down
4 changes: 2 additions & 2 deletions errors/src/test/java/com/palantir/conjure/java/api/errors/ServiceExceptionTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ public void testExceptionMessageContainsNoArgs_safeLogMessageContainsSafeArgsOnl
ServiceException ex = new ServiceException(ERROR, args);

assertThat(ex.getLogMessage()).isEqualTo(EXPECTED_ERROR_MSG);
assertThat(ex.getMessage()).isEqualTo(EXPECTED_ERROR_MSG + ": {arg1=foo}");
assertThat(ex.getMessage()).isEqualTo(EXPECTED_ERROR_MSG + ": {arg1=foo, arg2=2, arg3=null}");
}

@Test
Expand All @@ -51,7 +51,7 @@ public void testExceptionMessageWithDuplicateKeys() {
@Test
public void testExceptionMessageWithUnsafeArgs() {
ServiceException ex = new ServiceException(ERROR, UnsafeArg.of("arg1", 1), SafeArg.of("arg2", 2));
assertThat(ex.getMessage()).isEqualTo(EXPECTED_ERROR_MSG + ": {arg2=2}");
assertThat(ex.getMessage()).isEqualTo(EXPECTED_ERROR_MSG + ": {arg1=1, arg2=2}");
}

@Test
Expand Down