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

Error object v2 #1371

Merged
merged 21 commits into from
Aug 28, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
55ccbde
DO NOT COMMIT - playing with structured exception
amorton Jul 16, 2024
393323d
More playing
amorton Jul 20, 2024
18a60b8
outline for templating errors
amorton Jul 21, 2024
bd0028a
Merge remote-tracking branch 'refs/remotes/origin/main' into ajm/exce…
Hazel-Datastax Jul 29, 2024
0ebe9e3
Add YAML loading for error template to Error Objects V2 (#1316)
Hazel-Datastax Aug 15, 2024
668069a
Added support for optional http_response_override yaml (#1362)
amorton Aug 27, 2024
2cb7b05
Change error.yaml format to use kebab-case (#1368)
amorton Aug 27, 2024
a645103
Ajm/error object v2 rebase (#1370)
amorton Aug 28, 2024
e579b5a
Merge branch 'main' into feature/error-object-v2
tatu-at-datastax Aug 28, 2024
4326117
Fix pom.xml wrt dependency adds
tatu-at-datastax Aug 28, 2024
4144986
Fix an unintended Netty depeendency, use constant for HTTP ok
tatu-at-datastax Aug 28, 2024
35eef34
tiny javadoc fix
tatu-at-datastax Aug 28, 2024
9ce22bc
Minor clean up
tatu-at-datastax Aug 28, 2024
b3ebe8b
Remove unneeded constants
tatu-at-datastax Aug 28, 2024
41e022e
Comment update
tatu-at-datastax Aug 28, 2024
45d780b
Some more minor clean up
tatu-at-datastax Aug 28, 2024
9a9bd2a
Reduce visibility of accessor methods (but still test accessible)
tatu-at-datastax Aug 28, 2024
83a9427
Remove unnecessary null check/coercion (null checked by caller already)
tatu-at-datastax Aug 28, 2024
9599278
Merge branch 'main' into feature/error-object-v2
tatu-at-datastax Aug 28, 2024
02ff680
Remove/reduce unnecessary access
tatu-at-datastax Aug 28, 2024
24f4ae4
Reduce visibility for tests some more
tatu-at-datastax Aug 28, 2024
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
21 changes: 19 additions & 2 deletions pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,10 @@
<groupId>io.quarkus</groupId>
<artifactId>quarkus-hibernate-validator</artifactId>
</dependency>
<dependency>
amorton marked this conversation as resolved.
Show resolved Hide resolved
<groupId>io.quarkus</groupId>
<artifactId>quarkus-logging-json</artifactId>
</dependency>
<dependency>
<groupId>jakarta.validation</groupId>
<artifactId>jakarta.validation-api</artifactId>
Expand All @@ -143,6 +147,11 @@
<groupId>org.apache.commons</groupId>
<artifactId>commons-lang3</artifactId>
</dependency>
<dependency>
<groupId>org.apache.commons</groupId>
<artifactId>commons-text</artifactId>
<version>1.12.0</version>
</dependency>
<dependency>
<groupId>com.bpodgursky</groupId>
<artifactId>jbool_expressions</artifactId>
Expand Down Expand Up @@ -189,10 +198,18 @@
<artifactId>java-driver-query-builder</artifactId>
<version>${driver.version}</version>
</dependency>
<!-- Jackson YAML module for config file loading
-->
<dependency>
<groupId>io.quarkus</groupId>
<artifactId>quarkus-logging-json</artifactId>
<groupId>com.fasterxml.jackson.dataformat</groupId>
<artifactId>jackson-dataformat-yaml</artifactId>
</dependency>
<dependency>
<!-- Support for Optional used when loading error config seehttps://github.com/FasterXML/jackson-modules-java8/tree/2.18/datatypes -->
<groupId>com.fasterxml.jackson.datatype</groupId>
<artifactId>jackson-datatype-jdk8</artifactId>
</dependency>

<!-- Dependencies for _id auto-generation -->
<dependency>
<!-- for generating UUIDs of versions not supported by JDK -->
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ public record VectorizeConfig(
@Nullable
@Schema(
description =
"Optional parameters that match the template provided for the provider",
"Optional parameters that match the messageTemplate provided for the provider",
tatu-at-datastax marked this conversation as resolved.
Show resolved Hide resolved
type = SchemaType.OBJECT)
@JsonProperty("parameters")
@JsonInclude(JsonInclude.Include.NON_NULL)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
import static io.stargate.sgv2.jsonapi.config.constants.HttpConstants.AUTHENTICATION_TOKEN_HEADER_NAME;
import static io.stargate.sgv2.jsonapi.config.constants.HttpConstants.DEPRECATED_AUTHENTICATION_TOKEN_HEADER_NAME;

import io.netty.handler.codec.http.HttpResponseStatus;
import io.quarkus.security.identity.IdentityProviderManager;
import io.quarkus.security.identity.SecurityIdentity;
import io.quarkus.security.identity.request.AuthenticationRequest;
Expand All @@ -32,6 +31,7 @@
import io.stargate.sgv2.jsonapi.api.security.challenge.impl.ErrorChallengeSender;
import io.vertx.ext.web.RoutingContext;
import jakarta.enterprise.inject.Instance;
import jakarta.ws.rs.core.Response;
import java.util.Collections;
import java.util.Optional;
import java.util.Set;
Expand Down Expand Up @@ -84,7 +84,7 @@ public Uni<SecurityIdentity> authenticate(
@Override
public Uni<ChallengeData> getChallenge(RoutingContext context) {
return Uni.createFrom()
.item(new ChallengeData(HttpResponseStatus.UNAUTHORIZED.code(), null, null));
.item(new ChallengeData(Response.Status.UNAUTHORIZED.getStatusCode(), null, null));
}

/**
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,130 @@
package io.stargate.sgv2.jsonapi.exception.playing;
amorton marked this conversation as resolved.
Show resolved Hide resolved

import jakarta.ws.rs.core.Response;
import java.util.Objects;
import java.util.Optional;
import java.util.UUID;
import java.util.function.Supplier;

/**
* Base for all exceptions returned from the API for external use (as opposed to ones only used
* internally)
*
* <p>All errors are of a {@link ErrorFamily}, this class should not be used directly, one of the
* subclasses should be used. There are further categorised to be errors have an optional {@link
* ErrorScope}, that groups errors of a similar source together. Finally, the error has an {@link
* ErrorCode} that is unique within the family and scope.
*
* <p>To facilitate better error messages we template the messages in a {@link ErrorTemplate} that
* is loaded from a properties file. The body for the error may change with each instance of the
* exception, for example to include the number of filters that were included in a request.
*
* <p>The process of adding a new Error Code is:
*
* <p>
*
* <ul>
* <li>Decide what {@link ErrorFamily} the code belongs to.
* <li>Decide if the error has a {@link ErrorScope}, such as errors with Embedding Providers, if
* it does not then use {@link ErrorScope#NONE}.
* <li>Decide on the {@link ErrorCode}, it should be unique within the Family and Scope
* combination.
* <li>Add the error to file read by {@link ErrorTemplate} to define the title and templated body
* body.
* <li>Add the error code to the Code enum for the Exception class, such as {@link
* FilterException.Code} or {@link RequestException.Code} with the same name. When the enum is
* instantiated at JVM start the error template is loaded.
* <li>Create instances using the methods on {@link ErrorCode}.
* </ul>
*
* To get the Error to be returned in the {@link
* io.stargate.sgv2.jsonapi.api.model.command.CommandResult} call the {@link #get()} method to get a
* {@link CommandResponseError} that contains the subset of information we want to return.
*/
public abstract class APIException extends RuntimeException
implements Supplier<CommandResponseError> {

// All errors default to 200 HTTP status code, because we have partial failure modes.
// There are some overrides, e.g. a server timeout may be a 500, this is managed in the
// error config. See ErrorTemplate.
public static final int DEFAULT_HTTP_RESPONSE = Response.Status.OK.getStatusCode();

/**
* HTTP Response code for this error. NOTE: Not using enum from quarkus because do not want
* references to the HTTP framework this deep into the command processing
*/
public final int httpResponse;
tatu-at-datastax marked this conversation as resolved.
Show resolved Hide resolved

/** Unique identifier for this error instance. */
public final UUID errorId;

/** The family of the error. */
public final ErrorFamily family;

/**
* Optional scope of the error, inside the family.
*
* <p>Never {@code null}, uses "" for no scope. See {@link ErrorScope}
*/
public final String scope;
Copy link
Contributor

Choose a reason for hiding this comment

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

Related to oddity of ErrorScope, I am not sure why we use untyped String, then functional interface for ErrorScope? Why not plain old Enum value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the scope list is different for each family so there is no single enum with them all. i.e. we do not use the FILTER scope with a SERVER family.


/** Unique code for this error, codes should be unique within the Family and Scope combination. */
public final String code;

/** Title of this exception, the same title is used for all instances of the error code. */
public final String title;

/**
* Message body for this instance of the error.
*
* <p>Messages may be unique for each instance of the error code, they are typically driven from
* the {@link ErrorTemplate}.
*
* <p>This is the processed body to be returned to the client. NOT called body to avoid confusion
* with getMessage() on the RuntimeException
*/
public final String body;

public APIException(ErrorInstance errorInstance) {
Objects.requireNonNull(errorInstance, "ErrorInstance cannot be null");

this.errorId = errorInstance.errorId();
this.family = errorInstance.family();
this.scope = errorInstance.scope().scope();
this.code = errorInstance.code();
this.title = errorInstance.title();
this.body = errorInstance.body();
Objects.requireNonNull(
errorInstance.httpResponseOverride(), "httpResponseOverride cannot be null");
this.httpResponse = errorInstance.httpResponseOverride().orElse(DEFAULT_HTTP_RESPONSE);
}

public APIException(
ErrorFamily family, ErrorScope scope, String code, String title, String body) {
this(new ErrorInstance(UUID.randomUUID(), family, scope, code, title, body, Optional.empty()));
}

@Override
public CommandResponseError get() {
return null;
amorton marked this conversation as resolved.
Show resolved Hide resolved
}

/**
* Overrides to return the {@link #body} of the error. Using the body as this is effectively the
* message, the structure we want to return to the in the API is the {@link CommandResponseError}
* from {@link #get()}
*
* @return
*/
@Override
public String getMessage() {
return body;
}

@Override
public String toString() {
return String.format(
"%s{errorId=%s, family=%s, scope='%s', code='%s', title='%s', body='%s'}",
getClass().getSimpleName(), errorId, family, scope, code, title, body);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
package io.stargate.sgv2.jsonapi.exception.playing;

// TODO has all the fields we want to return in the error object in the request
// TODO: where do we implement error coalescing , here or in in the APIException
public record CommandResponseError(
String family, String scope, String code, String title, String message) {}
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
package io.stargate.sgv2.jsonapi.exception.playing;

public class DatabaseException extends ServerException {
public DatabaseException(ErrorInstance errorInstance) {
super(errorInstance);
}

public enum Code implements ErrorCode<DatabaseException> {
FAKE;

private final ErrorTemplate<DatabaseException> template;

Code() {
template =
ErrorTemplate.load(DatabaseException.class, ErrorFamily.SERVER, Scope.DATABASE, name());
}

@Override
public ErrorTemplate<DatabaseException> template() {
return template;
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
package io.stargate.sgv2.jsonapi.exception.playing;

public class EmbeddingProviderException extends ServerException {
public EmbeddingProviderException(ErrorInstance errorInstance) {
super(errorInstance);
}

public enum Code implements ErrorCode<EmbeddingProviderException> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the idea it's ok to have same "code" (like CLIENT_ERROR) for multiple scopes? I guess that is the case.

One concern is that to know which problem you have, it is no longer possible to group failures solely by Code, as that will conflate Codes with different Scopes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will add a fully qualified code to the API exception, we can then use it later for metrics etc

CLIENT_ERROR,
SERVER_ERROR;

private final ErrorTemplate<EmbeddingProviderException> template;

Code() {
template =
ErrorTemplate.load(
EmbeddingProviderException.class,
ErrorFamily.SERVER,
Scope.EMBEDDING_PROVIDER,
name());
}

@Override
public ErrorTemplate<EmbeddingProviderException> template() {
return template;
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
package io.stargate.sgv2.jsonapi.exception.playing;

import com.google.common.base.Preconditions;
import java.util.HashMap;
import java.util.Map;

/**
* Interface for any enum that represents an error code to implement.
*
* <p>This interface is used because multiple ENUM's define the scopes, the interface creates a way
* to treat the values from the different ENUM's in a consistent way.
*
* <p>The interface makes it easy for code to get an instance of the exception the code represents,
* built using the templates error information in {@link ErrorTemplate} that is loaded at startup.
*
* <p>With this interface code creates an instance of the exception by calling any of the {@link
* #get()} overloads for example:
*
* <pre>
* FilterException.Code.MULTIPLE_ID_FILTER.get("variable_name", "value");
* RequestException.Code.FAKE_CODE.get();
* </pre>
*
* @param <T> Type of the {@link APIException} the error code creates.
*/
public interface ErrorCode<T extends APIException> {
amorton marked this conversation as resolved.
Show resolved Hide resolved

/**
* Gets an instance of the {@link APIException} the error code represents without providing any
* substitution values for the error body.
*
* @return Instance of {@link APIException} the error code represents.
*/
default T get() {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not a good method name. Get what?

So suggesting we rename to something like "toApiException()" or "asApiException()"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will look for better name - discussed

return get(Map.of());
}

/**
* Gets an instance of the {@link APIException} the error code represents, providing substitution
* values for the error body as a param array.
*
* @param values Substitution values for the error body. The array length must be a multiple of 2,
* each pair of strings is treated as a key-value pair for example ["key-1", "value-1",
* "key-2", "value-2"]
* @return Instance of {@link APIException} the error code represents.
*/
default T get(String... values) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this method? We already have overload with Map, why/where do we need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there to make it easy when there is only one or two variables to pass. The idea was to make it as easy as possible to instantiate exceptions

Preconditions.checkArgument(
values.length % 2 == 0, "Length of the values must be a multiple of 2");
Map<String, String> valuesMap = new HashMap<>(values.length / 2);
for (int i = 0; i < values.length; i += 2) {
valuesMap.put(values[i], values[i + 1]);
}
return get(valuesMap);
}

/**
* Gets an instance of the {@link APIException} the error code represents, providing substitution
* values for the error body as a param array.
*
* @param values May of substitution values for the error body.
* @return Instance of {@link APIException} the error code represents.
*/
default T get(Map<String, String> values) {
return template().toException(values);
}

/**
* ENUM Implementers must return a non-null {@link ErrorTemplate} that is used to build an
* instance of the Exception the code represents.
*
* @return {@link ErrorTemplate} for the error code.
*/
ErrorTemplate<T> template();
}
Loading