From a6d4cb7938722381ca0c4b689272a666dc687b00 Mon Sep 17 00:00:00 2001 From: Pritham Marupaka Date: Mon, 21 Oct 2024 10:05:43 -0400 Subject: [PATCH 1/3] adding custom ConjureDefined exception types --- .../java/api/errors/ConjureDefinedError.java | 16 ++++++++++++++++ .../errors/ConjureDefinedRemoteException.java | 9 +++++++++ .../conjure/java/api/errors/RemoteException.java | 2 +- .../java/api/errors/SerializableError.java | 16 +++++++++++++++- .../java/api/errors/ServiceException.java | 2 +- 5 files changed, 42 insertions(+), 3 deletions(-) create mode 100644 errors/src/main/java/com/palantir/conjure/java/api/errors/ConjureDefinedError.java create mode 100644 errors/src/main/java/com/palantir/conjure/java/api/errors/ConjureDefinedRemoteException.java diff --git a/errors/src/main/java/com/palantir/conjure/java/api/errors/ConjureDefinedError.java b/errors/src/main/java/com/palantir/conjure/java/api/errors/ConjureDefinedError.java new file mode 100644 index 000000000..95d31179b --- /dev/null +++ b/errors/src/main/java/com/palantir/conjure/java/api/errors/ConjureDefinedError.java @@ -0,0 +1,16 @@ +package com.palantir.conjure.java.api.errors; + +import com.palantir.logsafe.Arg; +import com.palantir.logsafe.SafeLoggable; + +// TODO(pm): according to java docs on Error an "Error - indicates serious problems that a reasonable application should +// not try to catch." Perhaps this isn't a good name, but we've already called something SerializableError and the +// "error" matches the conjure concept so this seems fine but check if folks care. +// TODO(pm): copy-paste the ServiceException logic. I don't like extending ServiceException because we have +// (instanceof ServiceException) checks scattered across a few repos. Enumerate them, and see if they need to be +// handled differently compared to ServiceException. +public final class ConjureDefinedError extends ServiceException implements SafeLoggable { + public ConjureDefinedError(ErrorType errorType, Arg... parameters) { + super(errorType, parameters); + } +} diff --git a/errors/src/main/java/com/palantir/conjure/java/api/errors/ConjureDefinedRemoteException.java b/errors/src/main/java/com/palantir/conjure/java/api/errors/ConjureDefinedRemoteException.java new file mode 100644 index 000000000..d8b77d720 --- /dev/null +++ b/errors/src/main/java/com/palantir/conjure/java/api/errors/ConjureDefinedRemoteException.java @@ -0,0 +1,9 @@ +package com.palantir.conjure.java.api.errors; + +// TODO(pm): copy-paste RemoteException logic. we have instanceof checks scattered across a few repos that we should be +// intentional about. +public final class ConjureDefinedRemoteException extends RemoteException { + public ConjureDefinedRemoteException(SerializableError error, int status) { + super(error, status); + } +} diff --git a/errors/src/main/java/com/palantir/conjure/java/api/errors/RemoteException.java b/errors/src/main/java/com/palantir/conjure/java/api/errors/RemoteException.java index ad742f519..ff51d4b14 100644 --- a/errors/src/main/java/com/palantir/conjure/java/api/errors/RemoteException.java +++ b/errors/src/main/java/com/palantir/conjure/java/api/errors/RemoteException.java @@ -24,7 +24,7 @@ import java.util.List; /** An exception thrown by an RPC client to indicate remote/server-side failure. */ -public final class RemoteException extends RuntimeException implements SafeLoggable { +public class RemoteException extends RuntimeException implements SafeLoggable { private static final long serialVersionUID = 1L; private static final String ERROR_INSTANCE_ID = "errorInstanceId"; private static final String ERROR_CODE = "errorCode"; diff --git a/errors/src/main/java/com/palantir/conjure/java/api/errors/SerializableError.java b/errors/src/main/java/com/palantir/conjure/java/api/errors/SerializableError.java index 7b14a9d5c..aad7c551a 100644 --- a/errors/src/main/java/com/palantir/conjure/java/api/errors/SerializableError.java +++ b/errors/src/main/java/com/palantir/conjure/java/api/errors/SerializableError.java @@ -26,12 +26,13 @@ import com.fasterxml.jackson.databind.annotation.JsonSerialize; import com.palantir.logsafe.Arg; import com.palantir.logsafe.exceptions.SafeIllegalStateException; +import org.immutables.value.Value; + import java.io.IOException; import java.io.Serializable; import java.util.Map; import java.util.Objects; import java.util.Optional; -import org.immutables.value.Value; /** * A JSON-serializable representation of an exception/error, represented by its error code, an error name identifying @@ -129,6 +130,19 @@ public static SerializableError forException(ServiceException exception) { return builder.build(); } + public static SerializableError forExceptionWithSerializedArgs(ServiceException exception) { + Builder builder = new Builder() + .errorCode(exception.getErrorType().code().name()) + .errorName(exception.getErrorType().name()) + .errorInstanceId(exception.getErrorInstanceId()); + + for (Arg arg : exception.getArgs()) { + builder.putParameters(arg.getName(), Objects.toString(arg.getValue())); + } + + return builder.build(); + } + // TODO(rfink): Remove once all error producers have switched to errorCode/errorName. public static final class Builder extends ImmutableSerializableError.Builder {} diff --git a/errors/src/main/java/com/palantir/conjure/java/api/errors/ServiceException.java b/errors/src/main/java/com/palantir/conjure/java/api/errors/ServiceException.java index a2e7a7d57..83bd262d4 100644 --- a/errors/src/main/java/com/palantir/conjure/java/api/errors/ServiceException.java +++ b/errors/src/main/java/com/palantir/conjure/java/api/errors/ServiceException.java @@ -27,7 +27,7 @@ import javax.annotation.Nullable; /** A {@link ServiceException} thrown in server-side code to indicate server-side {@link ErrorType error states}. */ -public final class ServiceException extends RuntimeException implements SafeLoggable { +public class ServiceException extends RuntimeException implements SafeLoggable { private final ErrorType errorType; private final List> args; // unmodifiable From 3a81fad1900f9e94fd76f17a4aa7798ac9665a19 Mon Sep 17 00:00:00 2001 From: Pritham Marupaka Date: Tue, 22 Oct 2024 11:52:20 -0400 Subject: [PATCH 2/3] Add SerializableConjureDefinedError and related classes --- .../api/errors/CheckedServiceException.java | 87 +++++++++++++++ .../java/api/errors/ConjureDefinedError.java | 16 --- .../errors/ConjureDefinedRemoteException.java | 9 -- .../java/api/errors/RemoteException.java | 2 +- .../java/api/errors/SafeLoggableError.java | 27 +++++ .../api/errors/SafeLoggableErrorUtils.java | 101 ++++++++++++++++++ .../SerializableConjureDefinedError.java | 74 +++++++++++++ .../SerializableConjureErrorParameter.java | 49 +++++++++ .../java/api/errors/SerializableError.java | 16 +-- .../java/api/errors/ServiceException.java | 88 ++------------- .../SerializableConjureDefinedErrorTest.java | 79 ++++++++++++++ 11 files changed, 427 insertions(+), 121 deletions(-) create mode 100644 errors/src/main/java/com/palantir/conjure/java/api/errors/CheckedServiceException.java delete mode 100644 errors/src/main/java/com/palantir/conjure/java/api/errors/ConjureDefinedError.java delete mode 100644 errors/src/main/java/com/palantir/conjure/java/api/errors/ConjureDefinedRemoteException.java create mode 100644 errors/src/main/java/com/palantir/conjure/java/api/errors/SafeLoggableError.java create mode 100644 errors/src/main/java/com/palantir/conjure/java/api/errors/SafeLoggableErrorUtils.java create mode 100644 errors/src/main/java/com/palantir/conjure/java/api/errors/SerializableConjureDefinedError.java create mode 100644 errors/src/main/java/com/palantir/conjure/java/api/errors/SerializableConjureErrorParameter.java create mode 100644 errors/src/test/java/com/palantir/conjure/java/api/errors/SerializableConjureDefinedErrorTest.java diff --git a/errors/src/main/java/com/palantir/conjure/java/api/errors/CheckedServiceException.java b/errors/src/main/java/com/palantir/conjure/java/api/errors/CheckedServiceException.java new file mode 100644 index 000000000..93d228562 --- /dev/null +++ b/errors/src/main/java/com/palantir/conjure/java/api/errors/CheckedServiceException.java @@ -0,0 +1,87 @@ +/* + * (c) Copyright 2024 Palantir Technologies Inc. All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package com.palantir.conjure.java.api.errors; + +import com.palantir.logsafe.Arg; +import java.util.List; +import javax.annotation.Nullable; + +public abstract class CheckedServiceException extends Exception implements SafeLoggableError { + private static final String EXCEPTION_NAME = "CheckedServiceException"; + + private final ErrorType errorType; + private final List> args; // unmodifiable + + private final String errorInstanceId; + private final String unsafeMessage; + private final String noArgsMessage; + + /** + * Creates a new exception for the given error. All {@link com.palantir.logsafe.Arg parameters} are propagated to + * clients. + */ + public CheckedServiceException(ErrorType errorType, Arg... parameters) { + this(errorType, null, parameters); + } + + /** As above, but additionally records the cause of this exception. */ + public CheckedServiceException(ErrorType errorType, @Nullable Throwable cause, Arg... args) { + super(cause); + this.errorInstanceId = SafeLoggableErrorUtils.generateErrorInstanceId(cause); + this.errorType = errorType; + this.args = SafeLoggableErrorUtils.copyToUnmodifiableList(args); + this.unsafeMessage = SafeLoggableErrorUtils.renderUnsafeMessage(EXCEPTION_NAME, errorType, args); + this.noArgsMessage = SafeLoggableErrorUtils.renderNoArgsMessage(EXCEPTION_NAME, errorType); + } + + /** The {@link ErrorType} that gave rise to this exception. */ + @Override + public ErrorType getErrorType() { + return errorType; + } + + /** A unique identifier for (this instance of) this error. */ + @Override + public String getErrorInstanceId() { + return errorInstanceId; + } + + /** + * Java doc. + */ + @Override + public String getMessage() { + // Including all args here since any logger not configured with safe-logging will log this message. + return unsafeMessage; + } + + /** + * Java doc. + */ + @Override + public String getLogMessage() { + // Not returning safe args here since the safe-logging framework will log this message + args explicitly. + return noArgsMessage; + } + + /** + * Java doc. + */ + @Override + public List> getArgs() { + return args; + } +} diff --git a/errors/src/main/java/com/palantir/conjure/java/api/errors/ConjureDefinedError.java b/errors/src/main/java/com/palantir/conjure/java/api/errors/ConjureDefinedError.java deleted file mode 100644 index 95d31179b..000000000 --- a/errors/src/main/java/com/palantir/conjure/java/api/errors/ConjureDefinedError.java +++ /dev/null @@ -1,16 +0,0 @@ -package com.palantir.conjure.java.api.errors; - -import com.palantir.logsafe.Arg; -import com.palantir.logsafe.SafeLoggable; - -// TODO(pm): according to java docs on Error an "Error - indicates serious problems that a reasonable application should -// not try to catch." Perhaps this isn't a good name, but we've already called something SerializableError and the -// "error" matches the conjure concept so this seems fine but check if folks care. -// TODO(pm): copy-paste the ServiceException logic. I don't like extending ServiceException because we have -// (instanceof ServiceException) checks scattered across a few repos. Enumerate them, and see if they need to be -// handled differently compared to ServiceException. -public final class ConjureDefinedError extends ServiceException implements SafeLoggable { - public ConjureDefinedError(ErrorType errorType, Arg... parameters) { - super(errorType, parameters); - } -} diff --git a/errors/src/main/java/com/palantir/conjure/java/api/errors/ConjureDefinedRemoteException.java b/errors/src/main/java/com/palantir/conjure/java/api/errors/ConjureDefinedRemoteException.java deleted file mode 100644 index d8b77d720..000000000 --- a/errors/src/main/java/com/palantir/conjure/java/api/errors/ConjureDefinedRemoteException.java +++ /dev/null @@ -1,9 +0,0 @@ -package com.palantir.conjure.java.api.errors; - -// TODO(pm): copy-paste RemoteException logic. we have instanceof checks scattered across a few repos that we should be -// intentional about. -public final class ConjureDefinedRemoteException extends RemoteException { - public ConjureDefinedRemoteException(SerializableError error, int status) { - super(error, status); - } -} diff --git a/errors/src/main/java/com/palantir/conjure/java/api/errors/RemoteException.java b/errors/src/main/java/com/palantir/conjure/java/api/errors/RemoteException.java index ff51d4b14..ad742f519 100644 --- a/errors/src/main/java/com/palantir/conjure/java/api/errors/RemoteException.java +++ b/errors/src/main/java/com/palantir/conjure/java/api/errors/RemoteException.java @@ -24,7 +24,7 @@ import java.util.List; /** An exception thrown by an RPC client to indicate remote/server-side failure. */ -public class RemoteException extends RuntimeException implements SafeLoggable { +public final class RemoteException extends RuntimeException implements SafeLoggable { private static final long serialVersionUID = 1L; private static final String ERROR_INSTANCE_ID = "errorInstanceId"; private static final String ERROR_CODE = "errorCode"; diff --git a/errors/src/main/java/com/palantir/conjure/java/api/errors/SafeLoggableError.java b/errors/src/main/java/com/palantir/conjure/java/api/errors/SafeLoggableError.java new file mode 100644 index 000000000..75de47a9c --- /dev/null +++ b/errors/src/main/java/com/palantir/conjure/java/api/errors/SafeLoggableError.java @@ -0,0 +1,27 @@ +/* + * (c) Copyright 2024 Palantir Technologies Inc. All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.palantir.conjure.java.api.errors; + +import com.palantir.logsafe.SafeLoggable; + +public interface SafeLoggableError extends SafeLoggable { + /** The {@link ErrorType} that gave rise to this exception. */ + ErrorType getErrorType(); + + /** A unique identifier for (this instance of) this error. */ + String getErrorInstanceId(); +} diff --git a/errors/src/main/java/com/palantir/conjure/java/api/errors/SafeLoggableErrorUtils.java b/errors/src/main/java/com/palantir/conjure/java/api/errors/SafeLoggableErrorUtils.java new file mode 100644 index 000000000..dbed08616 --- /dev/null +++ b/errors/src/main/java/com/palantir/conjure/java/api/errors/SafeLoggableErrorUtils.java @@ -0,0 +1,101 @@ +/* + * (c) Copyright 2024 Palantir Technologies Inc. All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.palantir.conjure.java.api.errors; + +import com.palantir.logsafe.Arg; +import com.palantir.tritium.ids.UniqueIds; +import java.util.ArrayList; +import java.util.Collections; +import java.util.IdentityHashMap; +import java.util.List; +import java.util.Set; +import javax.annotation.Nullable; + +final class SafeLoggableErrorUtils { + private SafeLoggableErrorUtils() {} + + /** + * Finds the errorInstanceId of the most recent cause if present, otherwise generates a new random identifier. Note + * that this only searches {@link Throwable#getCause() causal exceptions}, not {@link Throwable#getSuppressed() + * suppressed causes}. + */ + static String generateErrorInstanceId(@Nullable Throwable cause) { + return generateErrorInstanceId(cause, Collections.newSetFromMap(new IdentityHashMap<>())); + } + + static String generateErrorInstanceId( + @Nullable Throwable cause, + // Guard against cause cycles, see Throwable.printStackTrace(PrintStreamOrWriter) + Set dejaVu) { + if (cause == null || !dejaVu.add(cause)) { + // we don't need cryptographically secure random UUIDs + return UniqueIds.pseudoRandomUuidV4().toString(); + } + if (cause instanceof ServiceException) { + return ((ServiceException) cause).getErrorInstanceId(); + } + if (cause instanceof CheckedServiceException) { + return ((CheckedServiceException) cause).getErrorInstanceId(); + } + if (cause instanceof RemoteException) { + return ((RemoteException) cause).getError().errorInstanceId(); + } + return generateErrorInstanceId(cause.getCause(), dejaVu); + } + + static List copyToUnmodifiableList(T[] elements) { + if (elements == null || elements.length == 0) { + return Collections.emptyList(); + } + List list = new ArrayList<>(elements.length); + for (T item : elements) { + if (item != null) { + list.add(item); + } + } + return Collections.unmodifiableList(list); + } + + static String renderUnsafeMessage(String exceptionName, ErrorType errorType, Arg... args) { + String message = renderNoArgsMessage(exceptionName, errorType); + + if (args == null || args.length == 0) { + return message; + } + + StringBuilder builder = new StringBuilder(); + boolean first = true; + builder.append(message).append(": {"); + for (Arg arg : args) { + if (arg != null) { + if (first) { + first = false; + } else { + builder.append(", "); + } + builder.append(arg.getName()).append("=").append(arg.getValue()); + } + } + builder.append("}"); + + return builder.toString(); + } + + static String renderNoArgsMessage(String exceptionName, ErrorType errorType) { + return exceptionName + ": " + errorType.code() + " (" + errorType.name() + ")"; + } +} diff --git a/errors/src/main/java/com/palantir/conjure/java/api/errors/SerializableConjureDefinedError.java b/errors/src/main/java/com/palantir/conjure/java/api/errors/SerializableConjureDefinedError.java new file mode 100644 index 000000000..647064017 --- /dev/null +++ b/errors/src/main/java/com/palantir/conjure/java/api/errors/SerializableConjureDefinedError.java @@ -0,0 +1,74 @@ +/* + * (c) Copyright 2024 Palantir Technologies Inc. All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.palantir.conjure.java.api.errors; + +import com.fasterxml.jackson.annotation.JsonIgnoreProperties; +import com.fasterxml.jackson.annotation.JsonProperty; +import com.fasterxml.jackson.databind.annotation.JsonDeserialize; +import com.fasterxml.jackson.databind.annotation.JsonSerialize; +import java.io.Serializable; +import java.util.List; +import org.immutables.value.Value; + +@SuppressWarnings("ImmutablesStyle") +@JsonDeserialize(builder = SerializableConjureDefinedError.Builder.class) +@JsonSerialize(as = ImmutableSerializableConjureDefinedError.class) +@Value.Immutable +@Value.Style(overshadowImplementation = false) +@JsonIgnoreProperties(ignoreUnknown = true) +public abstract class SerializableConjureDefinedError implements Serializable { + + @JsonProperty("errorCode") + public abstract String errorCode(); + + @JsonProperty("errorName") + public abstract String errorName(); + + @JsonProperty("errorInstanceId") + @Value.Default + @SuppressWarnings("checkstyle:designforextension") + public String errorInstanceId() { + return ""; + } + + /** A set of parameters that further explain the error. It's up to the creator of this object to serialize the value + * in SerializableConjureErrorParameter. + **/ + public abstract List parameters(); + + // In Conjure-Java - ConjureExceptions.java we'd create this object: + // public static SerializableConjureDefinedError forException(CheckedServiceException exception) { + // return builder() + // .errorCode(exception.getErrorType().code().name()) + // .errorName(exception.getErrorType().name()) + // .errorInstanceId(exception.getErrorInstanceId()) + // .parameters(exception.getArgs().stream() + // .map(arg -> SerializableConjureErrorParameter.builder() + // .name(arg.getName()) + // .serializedValue() // Serialize the parameter + // .isSafeForLogging(arg.isSafeForLogging()) + // .build()) + // .toList()) + // .build(); + // } + + public static final class Builder extends ImmutableSerializableConjureDefinedError.Builder {} + + public static Builder builder() { + return new Builder(); + } +} diff --git a/errors/src/main/java/com/palantir/conjure/java/api/errors/SerializableConjureErrorParameter.java b/errors/src/main/java/com/palantir/conjure/java/api/errors/SerializableConjureErrorParameter.java new file mode 100644 index 000000000..c15dd7941 --- /dev/null +++ b/errors/src/main/java/com/palantir/conjure/java/api/errors/SerializableConjureErrorParameter.java @@ -0,0 +1,49 @@ +/* + * (c) Copyright 2024 Palantir Technologies Inc. All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.palantir.conjure.java.api.errors; + +import com.fasterxml.jackson.annotation.JsonIgnoreProperties; +import com.fasterxml.jackson.annotation.JsonProperty; +import com.fasterxml.jackson.databind.annotation.JsonDeserialize; +import com.fasterxml.jackson.databind.annotation.JsonSerialize; +import java.io.Serializable; +import org.immutables.value.Value; + +@SuppressWarnings("ImmutablesStyle") +@JsonDeserialize(builder = SerializableConjureErrorParameter.Builder.class) +@JsonSerialize(as = ImmutableSerializableConjureErrorParameter.class) +@Value.Immutable +// This ensures that build() will return the concrete ImmutableSer... and not the abstract type. +@Value.Style(overshadowImplementation = false) +@JsonIgnoreProperties(ignoreUnknown = true) +public abstract class SerializableConjureErrorParameter implements Serializable { + + @JsonProperty("name") + public abstract String name(); + + @JsonProperty("serializedValue") + public abstract String serializedValue(); + + @JsonProperty("isSafeForLogging") + public abstract boolean isSafeForLogging(); + + public static final class Builder extends ImmutableSerializableConjureErrorParameter.Builder {} + + public static Builder builder() { + return new Builder(); + } +} diff --git a/errors/src/main/java/com/palantir/conjure/java/api/errors/SerializableError.java b/errors/src/main/java/com/palantir/conjure/java/api/errors/SerializableError.java index aad7c551a..7b14a9d5c 100644 --- a/errors/src/main/java/com/palantir/conjure/java/api/errors/SerializableError.java +++ b/errors/src/main/java/com/palantir/conjure/java/api/errors/SerializableError.java @@ -26,13 +26,12 @@ import com.fasterxml.jackson.databind.annotation.JsonSerialize; import com.palantir.logsafe.Arg; import com.palantir.logsafe.exceptions.SafeIllegalStateException; -import org.immutables.value.Value; - import java.io.IOException; import java.io.Serializable; import java.util.Map; import java.util.Objects; import java.util.Optional; +import org.immutables.value.Value; /** * A JSON-serializable representation of an exception/error, represented by its error code, an error name identifying @@ -130,19 +129,6 @@ public static SerializableError forException(ServiceException exception) { return builder.build(); } - public static SerializableError forExceptionWithSerializedArgs(ServiceException exception) { - Builder builder = new Builder() - .errorCode(exception.getErrorType().code().name()) - .errorName(exception.getErrorType().name()) - .errorInstanceId(exception.getErrorInstanceId()); - - for (Arg arg : exception.getArgs()) { - builder.putParameters(arg.getName(), Objects.toString(arg.getValue())); - } - - return builder.build(); - } - // TODO(rfink): Remove once all error producers have switched to errorCode/errorName. public static final class Builder extends ImmutableSerializableError.Builder {} diff --git a/errors/src/main/java/com/palantir/conjure/java/api/errors/ServiceException.java b/errors/src/main/java/com/palantir/conjure/java/api/errors/ServiceException.java index 83bd262d4..103fa3db2 100644 --- a/errors/src/main/java/com/palantir/conjure/java/api/errors/ServiceException.java +++ b/errors/src/main/java/com/palantir/conjure/java/api/errors/ServiceException.java @@ -17,18 +17,12 @@ package com.palantir.conjure.java.api.errors; import com.palantir.logsafe.Arg; -import com.palantir.logsafe.SafeLoggable; -import com.palantir.tritium.ids.UniqueIds; -import java.util.ArrayList; -import java.util.Collections; -import java.util.IdentityHashMap; import java.util.List; -import java.util.Set; import javax.annotation.Nullable; /** A {@link ServiceException} thrown in server-side code to indicate server-side {@link ErrorType error states}. */ -public class ServiceException extends RuntimeException implements SafeLoggable { - +public final class ServiceException extends RuntimeException implements SafeLoggableError { + private static final String EXCEPTION_NAME = "ServiceException"; private final ErrorType errorType; private final List> args; // unmodifiable @@ -49,20 +43,22 @@ public ServiceException(ErrorType errorType, @Nullable Throwable cause, Arg.. // TODO(rfink): Memoize formatting? super(cause); - this.errorInstanceId = generateErrorInstanceId(cause); + this.errorInstanceId = SafeLoggableErrorUtils.generateErrorInstanceId(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.unsafeMessage = renderUnsafeMessage(errorType, args); - this.noArgsMessage = renderNoArgsMessage(errorType); + this.args = SafeLoggableErrorUtils.copyToUnmodifiableList(args); + this.unsafeMessage = SafeLoggableErrorUtils.renderUnsafeMessage(EXCEPTION_NAME, errorType, args); + this.noArgsMessage = SafeLoggableErrorUtils.renderNoArgsMessage(EXCEPTION_NAME, errorType); } /** The {@link ErrorType} that gave rise to this exception. */ + @Override public ErrorType getErrorType() { return errorType; } /** A unique identifier for (this instance of) this error. */ + @Override public String getErrorInstanceId() { return errorInstanceId; } @@ -93,72 +89,4 @@ public List> getArgs() { public List> getParameters() { return getArgs(); } - - private static List copyToUnmodifiableList(T[] elements) { - if (elements == null || elements.length == 0) { - return Collections.emptyList(); - } - List list = new ArrayList<>(elements.length); - for (T item : elements) { - if (item != null) { - list.add(item); - } - } - return Collections.unmodifiableList(list); - } - - private static String renderUnsafeMessage(ErrorType errorType, Arg... args) { - String message = renderNoArgsMessage(errorType); - - if (args == null || args.length == 0) { - return message; - } - - StringBuilder builder = new StringBuilder(); - boolean first = true; - builder.append(message).append(": {"); - for (Arg arg : args) { - if (arg != null) { - if (first) { - first = false; - } else { - builder.append(", "); - } - builder.append(arg.getName()).append("=").append(arg.getValue()); - } - } - builder.append("}"); - - return builder.toString(); - } - - private static String renderNoArgsMessage(ErrorType errorType) { - return "ServiceException: " + errorType.code() + " (" + errorType.name() + ")"; - } - - /** - * Finds the errorInstanceId of the most recent cause if present, otherwise generates a new random identifier. Note - * that this only searches {@link Throwable#getCause() causal exceptions}, not {@link Throwable#getSuppressed() - * suppressed causes}. - */ - private static String generateErrorInstanceId(@Nullable Throwable cause) { - return generateErrorInstanceId(cause, Collections.newSetFromMap(new IdentityHashMap<>())); - } - - private static String generateErrorInstanceId( - @Nullable Throwable cause, - // Guard against cause cycles, see Throwable.printStackTrace(PrintStreamOrWriter) - Set dejaVu) { - if (cause == null || !dejaVu.add(cause)) { - // we don't need cryptographically secure random UUIDs - return UniqueIds.pseudoRandomUuidV4().toString(); - } - if (cause instanceof ServiceException) { - return ((ServiceException) cause).getErrorInstanceId(); - } - if (cause instanceof RemoteException) { - return ((RemoteException) cause).getError().errorInstanceId(); - } - return generateErrorInstanceId(cause.getCause(), dejaVu); - } } diff --git a/errors/src/test/java/com/palantir/conjure/java/api/errors/SerializableConjureDefinedErrorTest.java b/errors/src/test/java/com/palantir/conjure/java/api/errors/SerializableConjureDefinedErrorTest.java new file mode 100644 index 000000000..13073d67a --- /dev/null +++ b/errors/src/test/java/com/palantir/conjure/java/api/errors/SerializableConjureDefinedErrorTest.java @@ -0,0 +1,79 @@ +/* + * (c) Copyright 2024 Palantir Technologies Inc. All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.palantir.conjure.java.api.errors; + +import static org.assertj.core.api.AssertionsForClassTypes.assertThat; + +import com.palantir.logsafe.SafeArg; +import com.palantir.logsafe.UnsafeArg; +import java.util.Objects; +import org.junit.jupiter.api.Test; + +public final class SerializableConjureDefinedErrorTest { + @Test + public void forException_should_keep_both_safe_and_unsafe_args() throws Exception { + record FancyContent(int fancyValue, String fancyString) { + static FancyContent of() { + return new FancyContent(42, "hello"); + } + } + + ErrorType errorType = ErrorType.FAILED_PRECONDITION; + CheckedServiceException exception = new CheckedServiceException( + errorType, + SafeArg.of("safeKey", 42), + UnsafeArg.of("sensitiveInfo", "some user-entered content"), + SafeArg.of("fancyContent", FancyContent.of())) {}; + + SerializableConjureDefinedError expected = SerializableConjureDefinedError.builder() + .errorCode(errorType.code().name()) + .errorName(errorType.name()) + .errorInstanceId(exception.getErrorInstanceId()) + .addParameters(SerializableConjureErrorParameter.builder() + .name("safeKey") + .serializedValue("42") + .isSafeForLogging(true) + .build()) + .addParameters(SerializableConjureErrorParameter.builder() + .name("sensitiveInfo") + .serializedValue("some user-entered content") + .isSafeForLogging(false) + .build()) + .addParameters(SerializableConjureErrorParameter.builder() + .name("fancyContent") + .serializedValue("FancyContent[fancyValue=42, fancyString=hello]") + .isSafeForLogging(true) + .build()) + .build(); + assertThat(forException(exception)).isEqualTo(expected); + } + + private static SerializableConjureDefinedError forException(CheckedServiceException exception) { + return SerializableConjureDefinedError.builder() + .errorCode(exception.getErrorType().code().name()) + .errorName(exception.getErrorType().name()) + .errorInstanceId(exception.getErrorInstanceId()) + .parameters(exception.getArgs().stream() + .map(arg -> SerializableConjureErrorParameter.builder() + .name(arg.getName()) + .serializedValue(Objects.toString(arg.getValue())) + .isSafeForLogging(arg.isSafeForLogging()) + .build()) + .toList()) + .build(); + } +} From 4d11565c7753eac41d23cd44ed9faa0117340b75 Mon Sep 17 00:00:00 2001 From: Pritham Marupaka Date: Tue, 22 Oct 2024 14:41:55 -0400 Subject: [PATCH 3/3] CheckedRemoteException --- .../api/errors/CheckedRemoteException.java | 105 ++++++++++++++++++ 1 file changed, 105 insertions(+) create mode 100644 errors/src/main/java/com/palantir/conjure/java/api/errors/CheckedRemoteException.java diff --git a/errors/src/main/java/com/palantir/conjure/java/api/errors/CheckedRemoteException.java b/errors/src/main/java/com/palantir/conjure/java/api/errors/CheckedRemoteException.java new file mode 100644 index 000000000..f5c051f43 --- /dev/null +++ b/errors/src/main/java/com/palantir/conjure/java/api/errors/CheckedRemoteException.java @@ -0,0 +1,105 @@ +/* + * (c) Copyright 2024 Palantir Technologies Inc. All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.palantir.conjure.java.api.errors; + +import com.palantir.logsafe.Arg; +import com.palantir.logsafe.SafeArg; +import com.palantir.logsafe.SafeLoggable; +import java.util.Arrays; +import java.util.Collections; +import java.util.List; + +// TODO(pm): Move common methods into utils class. +public final class CheckedRemoteException extends Exception implements SafeLoggable { + // TODO(pm): what is the purpose of serialVersionUID? + + private static final String ERROR_INSTANCE_ID = "errorInstanceId"; + private static final String ERROR_CODE = "errorCode"; + private static final String ERROR_NAME = "errorName"; + + private final String stableMessage; + private final SerializableConjureDefinedError error; + private final int status; + private final List> args; + // Lazily evaluated based on the stableMessage, errorInstanceId, and args. + @SuppressWarnings("MutableException") + private String unsafeMessage; + + /** Returns the error thrown by a remote process which caused an RPC call to fail. */ + public SerializableConjureDefinedError getError() { + return error; + } + + /** The HTTP status code of the HTTP response conveying the remote exception. */ + public int getStatus() { + return status; + } + + public CheckedRemoteException(SerializableConjureDefinedError error, int status) { + this.stableMessage = error.errorCode().equals(error.errorName()) + ? "CheckedRemoteException: " + error.errorCode() + : "CheckedRemoteException: " + error.errorCode() + " (" + error.errorName() + ")"; + this.error = error; + this.status = status; + this.args = Collections.unmodifiableList(Arrays.asList( + SafeArg.of(ERROR_INSTANCE_ID, error.errorInstanceId()), + SafeArg.of(ERROR_NAME, error.errorName()), + SafeArg.of(ERROR_CODE, error.errorCode()))); + } + + @Override + public String getMessage() { + // This field is not used in most environments so the cost of computation may be avoided. + String messageValue = unsafeMessage; + if (messageValue == null) { + messageValue = renderUnsafeMessage(); + unsafeMessage = messageValue; + } + return messageValue; + } + + private String renderUnsafeMessage() { + StringBuilder builder = new StringBuilder() + .append(stableMessage) + .append(" with instance ID ") + .append(error.errorInstanceId()); + if (!error.parameters().isEmpty()) { + builder.append(": {"); + error.parameters().forEach(errorParameter -> builder.append(errorParameter.name()) + .append('=') + .append(errorParameter.serializedValue()) + .append(", ")); + // remove the trailing space + builder.setLength(builder.length() - 1); + // replace the trailing comma with a close curly brace + builder.setCharAt(builder.length() - 1, '}'); + } + return builder.toString(); + } + + @Override + public String getLogMessage() { + return stableMessage; + } + + @Override + public List> getArgs() { + // CheckedRemoteException explicitly does not support arguments because they have already been recorded + // on the service which produced the causal SerializableError. + return args; + } +}