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

adding custom ConjureDefined exception types #1246

Draft
wants to merge 3 commits into
base: develop
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Original file line number Diff line number Diff line change
@@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be a checked exception? Extending ServiceException means we're also extending RuntimeException, which means it's unchecked.

public ConjureDefinedError(ErrorType errorType, Arg<?>... parameters) {
super(errorType, parameters);
}
}
Original file line number Diff line number Diff line change
@@ -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);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -129,6 +130,19 @@ public static SerializableError forException(ServiceException exception) {
return builder.build();
}

public static SerializableError forExceptionWithSerializedArgs(ServiceException exception) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't necessarily need to reuse the SerializableError type, it's the serialized form as defined here that we want: https://palantir.github.io/conjure/#/docs/spec/wire?id=_55-conjure-errors

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 {}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<Arg<?>> args; // unmodifiable
Expand Down