Error object v2 #1371

amorton · 2024-08-28T08:26:10Z

What this PR does:

Initial error object v2

This is still not in use, it is dead code. Once we get it in we will move from the "playing" package into parent "exceptions" package once we start using it.

This PR is to get it into the main, next setps are:

start using it
start making it return the errors on the API

Which issue(s) this PR fixes:
Fixes #

Checklist

[*] Changes manually tested
Automated Tests added/updated
[ * Documentation added/updated
CLA Signed: DataStax CLA

…ption-playing # Conflicts: # src/main/java/io/stargate/sgv2/jsonapi/service/embedding/operation/EmbeddingProvider.java

Co-authored-by: Aaron Morton <aaron.morton@datastax.com>

Co-authored-by: YuqiDu <istimdu@gmail.com> Co-authored-by: Tatu Saloranta <tatu.saloranta@datastax.com> Co-authored-by: Mahesh Rajamani <99678631+maheshrajamani@users.noreply.github.com> Co-authored-by: Jeffrey Carpenter <jeffrey.carpenter@datastax.com> Co-authored-by: Hazel <hazel.he@datastax.com> Co-authored-by: Kathiresan Selvaraj <96088452+kathirsvn@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Yuqi-Du <138714577+Yuqi-Du@users.noreply.github.com>

pom.xml

src/main/java/io/stargate/sgv2/jsonapi/api/model/command/impl/CreateCollectionCommand.java

src/main/java/io/stargate/sgv2/jsonapi/exception/playing/APIException.java

tatu-at-datastax · 2024-08-28T16:23:38Z

src/main/java/io/stargate/sgv2/jsonapi/exception/playing/UnresolvedErrorTemplateVariable.java

+ * and this exception is raised if the template and the code do not match. Note that it is OK for
+ * the code to provide more variables than the template uses, but not the other way around.
+ */
+public class UnresolvedErrorTemplateVariable extends RuntimeException {


thought: we should probably have a root-level ultimate base exception for validity problems -- something like DataApiValidityException (or whatever name is) extending RuntimeException, being extended by this and other types (but not APIException).

This is just a follow-up change idea, no need to change for this PR.

discussed, maybe later we can look at more base exceptions

src/main/java/io/stargate/sgv2/jsonapi/exception/playing/ErrorCode.java

tatu-at-datastax · 2024-08-28T16:37:24Z

src/main/java/io/stargate/sgv2/jsonapi/exception/playing/ServerException.java

+ */
+public class ServerException extends APIException {
+
+  public static final ErrorFamily FAMILY = ErrorFamily.SERVER;


This is problematic:

Should not really be public -- why would it be needed from outside (probably protected for sub-classes)

Name is very vague, esp. when referred to from sub-classes. Maybe SERVER_EXCEPTION_FAMILY

... or is this constant needed at all? Just use ErrorFamily.SERVER directly in sub-class.

idea was for subclasses to all reference the same definition of the family during construction, e.g. in the ctor for the Code enum

tatu-at-datastax · 2024-08-28T17:47:42Z

src/main/java/io/stargate/sgv2/jsonapi/exception/playing/DatabaseException.java

+
+public class DatabaseException extends ServerException {
+
+  public static final Scope SCOPE = Scope.DATABASE;


Same as with other constants:

Should not be public?

Name is very generic/vague, probably DEFAULT_SCOPE.

... but is this constant actually needed at all? Seems like its only used from sub-class.

Not 100% sure what to do with it, but just seems wrong. (1 and 2 easy enough to change, but eliminating need would be best)

idea was for the Code enum to be able to refence a central definition while it is constructing

tatu-at-datastax · 2024-08-28T17:49:05Z

src/main/java/io/stargate/sgv2/jsonapi/exception/playing/DatabaseException.java

+    private final ErrorTemplate<DatabaseException> template;
+
+    Code() {
+      template = ErrorTemplate.load(DatabaseException.class, FAMILY, SCOPE, name());


Loading from Enum instance constructors is extremely fragile -- I think it happens at class loading time -- and could cause really gnarly failure modes.

But then again, as long as our test coverage is good, maybe that's not a big issue.

Ohhhh. MAybe that is worse -- this is called once per each Enum value? Does it really try to load something, or is that misnamed method? (to me, loading means reading YAML config file).

The ErrorTemplate will cause the ErrorConfig to load, which happens once and loads all of the YAML config at once and only once.

Then this call to load() will fail if there is an error code defined in code but not in the yaml, so we want that failure to happen.

tatu-at-datastax · 2024-08-28T20:12:29Z

src/main/java/io/stargate/sgv2/jsonapi/exception/playing/ErrorScope.java

+ * <p>See {@link APIException}
+ */
+@FunctionalInterface
+public interface ErrorScope {


I don't understand this -- why not just a simple Enum? What is the point in this contraption?

discussed - make it clear this is here because the scope is disjoint sets for the diff families

tatu-at-datastax · 2024-08-28T20:13:43Z

src/main/java/io/stargate/sgv2/jsonapi/exception/playing/APIException.java

+   *
+   * <p>Never {@code null}, uses "" for no scope. See {@link ErrorScope}
+   */
+  public final String scope;


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?

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.

tatu-at-datastax · 2024-08-28T20:26:34Z

src/main/java/io/stargate/sgv2/jsonapi/exception/playing/EmbeddingProviderException.java

+    super(errorInstance);
+  }
+
+  public enum Code implements ErrorCode<EmbeddingProviderException> {


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.

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

tatu-at-datastax · 2024-08-28T20:34:16Z

src/main/java/io/stargate/sgv2/jsonapi/exception/playing/ErrorCode.java

+   *
+   * @return Instance of {@link APIException} the error code represents.
+   */
+  default T get() {


This is not a good method name. Get what?

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

will look for better name - discussed

tatu-at-datastax · 2024-08-28T20:36:32Z

src/main/java/io/stargate/sgv2/jsonapi/exception/playing/ErrorCode.java

+   *     "key-2", "value-2"]
+   * @return Instance of {@link APIException} the error code represents.
+   */
+  default T get(String... values) {


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

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

tatu-at-datastax · 2024-08-28T20:38:45Z

src/main/java/io/stargate/sgv2/jsonapi/exception/playing/ErrorConfig.java

+ * Then use {@link #getErrorDetail(ErrorFamily, String, String)} if you want the template, or the
+ * use {@link #getSnippetVars()} when running templates.
+ *
+ * <p>If you need more control use the {@link #initializeFromYamlResource(String)} before any code


... not necessarily easy/possible to do, due to class loading/initialization constraints.

note to be - revisit the comment with how this is used in testing

tatu-at-datastax · 2024-08-28T20:41:09Z

src/main/java/io/stargate/sgv2/jsonapi/exception/playing/ErrorConfig.java

+   *
+   * @return
+   */
+  public List<ErrorDetail> requestErrors() {


Why do we have this?

tatu-at-datastax · 2024-08-28T20:41:22Z

src/main/java/io/stargate/sgv2/jsonapi/exception/playing/ErrorConfig.java

+   *
+   * @return
+   */
+  public List<ErrorDetail> serverErrors() {


Why do we have this?

src/main/java/io/stargate/sgv2/jsonapi/exception/playing/ErrorConfig.java

tatu-at-datastax · 2024-08-28T21:02:26Z

@amorton @Hazel-Datastax

Ok, so. I have a few concerns with the code included in here, at implementation details level. But at high level it makes sense; and so with the expectation of thorough testing (included in PR) I will approve this.
But I have specific questions/suggestions/concerns about following things which we should discuss and maybe address either as part of this PR, or follow up.
So:

Why is ErrorScope not a simple Enum? Making it a functional interface to produce String makes no immediate sense to me at all; why complication, loss of type-safety?
(smaller concern) Splitting of Code and Scope allows (I think) duplicates of Code for different error types; and so we no longer have single unique identifier -- formerly this was effectively by using "scope" equivalent as prefix ("SERVER_xxx", "EMBEDDING_xxx").
(smallish renaming) ErrorCode.get() is not a good name: from what it does, should probably be .asApiException() or .toApiException().

(earlier I had this too:

(small visibility concern) Why is ErrorConfig.serverErrors() and .requestErrors() exposed as public accessors?

but I resolved it by removing/reducing access)

tatu-at-datastax

As per my comment above, conditionally approving, just need to address concerns before or after merging.

amorton · 2024-08-28T23:09:46Z

follow up tickets, and there will soon be a PR that makes this usable that will move it from the "playing" package

#1374
#1375
#1377

merging

amorton and others added 8 commits July 16, 2024 12:33

DO NOT COMMIT - playing with structured exception

55ccbde

More playing

393323d

outline for templating errors

18a60b8

Merge remote-tracking branch 'refs/remotes/origin/main' into ajm/exce…

bd0028a

…ption-playing # Conflicts: # src/main/java/io/stargate/sgv2/jsonapi/service/embedding/operation/EmbeddingProvider.java

Add YAML loading for error template to Error Objects V2 (#1316)

0ebe9e3

Co-authored-by: Aaron Morton <aaron.morton@datastax.com>

Added support for optional http_response_override yaml (#1362)

668069a

Change error.yaml format to use kebab-case (#1368)

2cb7b05

amorton requested a review from a team as a code owner August 28, 2024 08:26

amorton requested a review from tatu-at-datastax August 28, 2024 08:26

Merge branch 'main' into feature/error-object-v2

e579b5a