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

Remove OperationsConfig.extendError in the config file #1683

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
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
Expand Up @@ -13,7 +13,7 @@

/**
* Use the {@link CommandResultBuilder} to create a {@link CommandResult} for a command response,
* for creation see {@link #singleDocumentBuilder(boolean, boolean)} and the other factory methods.
* for creation see {@link #singleDocumentBuilder(boolean)} and the other factory methods.
*
* <p>Comments on {@link CommandResultBuilder} explain future work here.
*/
Expand Down Expand Up @@ -65,24 +65,18 @@ public record CommandResult(
* Also the {@link io.stargate.sgv2.jsonapi.service.operation.OperationAttemptPageBuilder} is how
* things will turn out.
*/
public static CommandResultBuilder singleDocumentBuilder(
boolean useErrorObjectV2, boolean debugMode) {
return new CommandResultBuilder(
CommandResultBuilder.ResponseType.SINGLE_DOCUMENT, useErrorObjectV2, debugMode);
public static CommandResultBuilder singleDocumentBuilder(boolean debugMode) {
return new CommandResultBuilder(CommandResultBuilder.ResponseType.SINGLE_DOCUMENT, debugMode);
}

/** See {@link #singleDocumentBuilder(boolean, boolean)} */
public static CommandResultBuilder multiDocumentBuilder(
boolean useErrorObjectV2, boolean debugMode) {
return new CommandResultBuilder(
CommandResultBuilder.ResponseType.MULTI_DOCUMENT, useErrorObjectV2, debugMode);
/** See {@link #singleDocumentBuilder(boolean)} */
public static CommandResultBuilder multiDocumentBuilder(boolean debugMode) {
return new CommandResultBuilder(CommandResultBuilder.ResponseType.MULTI_DOCUMENT, debugMode);
}

/** See {@link #singleDocumentBuilder(boolean, boolean)} */
public static CommandResultBuilder statusOnlyBuilder(
boolean useErrorObjectV2, boolean debugMode) {
return new CommandResultBuilder(
CommandResultBuilder.ResponseType.STATUS_ONLY, useErrorObjectV2, debugMode);
/** See {@link #singleDocumentBuilder(boolean)} */
public static CommandResultBuilder statusOnlyBuilder(boolean debugMode) {
return new CommandResultBuilder(CommandResultBuilder.ResponseType.STATUS_ONLY, debugMode);
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,23 +37,19 @@ public enum ResponseType {
// If the debug mode is enabled, errors include the errorclass
private final boolean debugMode;

// Flagged true to include the new error object v2
private final boolean useErrorObjectV2;

// Created in the Ctor
private final APIExceptionCommandErrorBuilder apiExceptionToError;

// another builder, because if we add a warning we want to use the V2 error object
// but may not be returning V2 errors in the result
private final APIExceptionCommandErrorBuilder apiWarningToError;

CommandResultBuilder(ResponseType responseType, boolean useErrorObjectV2, boolean debugMode) {
CommandResultBuilder(ResponseType responseType, boolean debugMode) {
this.responseType = responseType;
this.useErrorObjectV2 = useErrorObjectV2;
this.debugMode = debugMode;

this.apiExceptionToError = new APIExceptionCommandErrorBuilder(debugMode, useErrorObjectV2);
this.apiWarningToError = new APIExceptionCommandErrorBuilder(debugMode, true);
this.apiExceptionToError = new APIExceptionCommandErrorBuilder(debugMode);
this.apiWarningToError = new APIExceptionCommandErrorBuilder(debugMode);
}

public CommandResultBuilder addStatus(CommandStatus status, Object value) {
Expand Down Expand Up @@ -103,9 +99,7 @@ public CommandResultBuilder nextPageState(String nextPageState) {
return this;
}

/**
* Gets the appropriately formatted error given {@link #useErrorObjectV2} and {@link #debugMode}.
*/
/** Gets the appropriately formatted error given {@link #debugMode}. */
public CommandResult.Error throwableToCommandError(Throwable throwable) {

if (throwable instanceof APIException apiException) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,7 @@ public ErrorChallengeSender(ObjectMapper objectMapper) {
CommandResult.Error error =
new CommandResult.Error(
message, Collections.emptyMap(), Collections.emptyMap(), Response.Status.UNAUTHORIZED);
commandResult =
CommandResult.statusOnlyBuilder(false, false).addCommandResultError(error).build();
commandResult = CommandResult.statusOnlyBuilder(false).addCommandResultError(error).build();
}

/** {@inheritDoc} */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,12 +66,6 @@ public interface OperationsConfig {
@WithDefault("false")
boolean enableEmbeddingGateway();

/**
* @return Flag to extend error response with additional information.
*/
@WithDefault("true")
boolean extendError();

/**
* @return Defines the maximum limit of document read to perform in memory sorting <code>10000
* </code>.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,20 +18,15 @@
public class APIExceptionCommandErrorBuilder {

private final boolean debugEnabled;
private final boolean returnErrorObjectV2;

/**
* Create a new instance that will create a {@link CommandResult.Error} with the given options.
*
* @param debugEnabled if <code>true</code> the {@link CommandResult.Error} will include the
* {@link ErrorObjectV2Constants.Fields#EXCEPTION_CLASS} field.
* @param returnErrorObjectV2 if <code>true</code> will include the fields for the V2 error object
* such as family etc
*/
public APIExceptionCommandErrorBuilder(boolean debugEnabled, boolean returnErrorObjectV2) {

public APIExceptionCommandErrorBuilder(boolean debugEnabled) {
this.debugEnabled = debugEnabled;
this.returnErrorObjectV2 = returnErrorObjectV2;
}

/**
Expand All @@ -57,12 +52,11 @@ public CommandResult.Error buildLegacyCommandResultError(APIException apiExcepti
// errorFields.put(ErrorObjectV2Constants.Fields.MESSAGE, apiException.body);
errorFields.put(ErrorObjectV2Constants.Fields.CODE, apiException.code);

if (returnErrorObjectV2) {
errorFields.put(ErrorObjectV2Constants.Fields.FAMILY, apiException.family.name());
errorFields.put(ErrorObjectV2Constants.Fields.SCOPE, apiException.scope);
errorFields.put(ErrorObjectV2Constants.Fields.TITLE, apiException.title);
errorFields.put(ErrorObjectV2Constants.Fields.ID, apiException.errorId);
}
errorFields.put(ErrorObjectV2Constants.Fields.FAMILY, apiException.family.name());
errorFields.put(ErrorObjectV2Constants.Fields.SCOPE, apiException.scope);
errorFields.put(ErrorObjectV2Constants.Fields.TITLE, apiException.title);
errorFields.put(ErrorObjectV2Constants.Fields.ID, apiException.errorId);

if (debugEnabled) {
errorFields.put(
ErrorObjectV2Constants.Fields.EXCEPTION_CLASS, apiException.getClass().getSimpleName());
Expand All @@ -83,16 +77,6 @@ public CommandResult.Error buildLegacyCommandResultError(APIException apiExcepti
* @return a {@link CommandErrorV2} that represents the <code>apiException</code>.
*/
public CommandErrorV2 buildCommandErrorV2(APIException apiException) {
if (!returnErrorObjectV2) {
// aaron - oct 9 - I know this seems silly, we are in the process on moving all the errors to
// the V2
// i added this function to be used with WARNING errors, once we have rolled out the use of
// CommandErrorV2
// everywhere we wont need this test and there will be one build function
throw new IllegalStateException(
"Cannot build a V2 error object when returnErrorObjectV2 is false");
}

var builder = CommandErrorV2.builderV2();

if (debugEnabled) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ public CommandResult get() {
message = errorCode.getMessage();
}

var builder = CommandResult.statusOnlyBuilder(false, false);
var builder = CommandResult.statusOnlyBuilder(false);

// construct and return
builder.addCommandResultError(getCommandResultError(message, httpStatus));
Expand Down Expand Up @@ -163,26 +163,21 @@ public CommandResult.Error getCommandResultError(String message, Response.Status
}
DebugModeConfig debugModeConfig = config.getConfigMapping(DebugModeConfig.class);
final boolean debugEnabled = debugModeConfig.enabled();
final boolean extendError = config.getConfigMapping(OperationsConfig.class).extendError();
Map<String, Object> fields;

if (extendError) {
fields =
new HashMap<>(
Map.of(
"id",
id,
"errorCode",
errorCode.name(),
"family",
errorFamily,
"scope",
errorScope,
"title",
title));
} else {
fields = new HashMap<>(Map.of("errorCode", errorCode.name()));
}
fields =
new HashMap<>(
Map.of(
"id",
id,
"errorCode",
errorCode.name(),
"family",
errorFamily,
"scope",
errorScope,
"title",
title));

if (debugEnabled) {
fields.put("exceptionClass", this.getClass().getSimpleName());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ public RestResponse<CommandResult> constraintViolationException(
ConstraintViolationException exception) {
// map all violations to errors
Set<ConstraintViolation<?>> violations = exception.getConstraintViolations();
var builder = CommandResult.statusOnlyBuilder(false, false);
var builder = CommandResult.statusOnlyBuilder(false);
violations.stream()
.map(ConstraintViolationExceptionMapper::getError)
.distinct()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ public record ThrowableCommandResultSupplier(Throwable t) implements Supplier<Co
@Override
public CommandResult get() {

var builder = CommandResult.statusOnlyBuilder(false, false);
var builder = CommandResult.statusOnlyBuilder(false);

// resolve message
builder.addCommandResultError(ThrowableToErrorMapper.getMapperFunction().apply(t));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,11 +40,9 @@ public RestResponse<CommandResult> webApplicationExceptionMapper(WebApplicationE
// V2 Error are returned as APIException, this is required to translate the exception to
// CommandResult if the exception thrown as part of command deserialization
if (toReport instanceof APIException ae) {
var errorBuilder =
new APIExceptionCommandErrorBuilder(
debugModeConfig.enabled(), operationsConfig.extendError());
var errorBuilder = new APIExceptionCommandErrorBuilder(debugModeConfig.enabled());
return RestResponse.ok(
CommandResult.statusOnlyBuilder(false, false)
CommandResult.statusOnlyBuilder(false)
.addCommandResultError(errorBuilder.buildLegacyCommandResultError(ae))
.build());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,7 @@ public DeleteAttemptPage<SchemaT> getOperationPage() {
// returning a document
// e.g. for findOneAndDelete, for now it is always status only

return new DeleteAttemptPage<>(
attempts, CommandResult.statusOnlyBuilder(useErrorObjectV2, debugMode));
return new DeleteAttemptPage<>(attempts, CommandResult.statusOnlyBuilder(debugMode));
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -177,9 +177,7 @@ public Builder<SchemaT> returnDocumentResponses(boolean returnDocumentResponses)
public InsertAttemptPage<SchemaT> getOperationPage() {

return new InsertAttemptPage<>(
attempts,
CommandResult.statusOnlyBuilder(useErrorObjectV2, debugMode),
returnDocumentResponses);
attempts, CommandResult.statusOnlyBuilder(debugMode), returnDocumentResponses);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ public InsertOperationPage(
this.failedInsertions = new ArrayList<>(allAttemptedInsertions.size());
this.debugMode = debugMode;
this.useErrorObjectV2 = useErrorObjectV2;
this.apiExceptionToError = new APIExceptionCommandErrorBuilder(debugMode, useErrorObjectV2);
this.apiExceptionToError = new APIExceptionCommandErrorBuilder(debugMode);
}

enum InsertionStatus {
Expand All @@ -102,7 +102,7 @@ public CommandResult get() {
// TODO AARON used to only sort the success list when not returning detailed responses, check OK
Collections.sort(successfulInsertions);

var builder = CommandResult.statusOnlyBuilder(useErrorObjectV2, debugMode);
var builder = CommandResult.statusOnlyBuilder(debugMode);
return returnDocumentResponses ? perDocumentResult(builder) : nonPerDocumentResult(builder);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,10 +67,7 @@ public Builder<SchemaT> usingCommandStatus(CommandStatus statusKey) {

public Supplier<CommandResult> getOperationPage() {
return new MetadataAttemptPage<>(
attempts,
CommandResult.statusOnlyBuilder(useErrorObjectV2, debugMode),
showSchema,
statusKey) {};
attempts, CommandResult.statusOnlyBuilder(debugMode), showSchema, statusKey) {};
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ public abstract class OperationAttemptPageBuilder<
SchemaT extends SchemaObject, AttemptT extends OperationAttempt<AttemptT, SchemaT>>
extends OperationAttemptAccumulator<SchemaT, AttemptT> {

protected boolean useErrorObjectV2 = false;
protected boolean debugMode = false;

/**
Expand All @@ -29,16 +28,6 @@ public abstract class OperationAttemptPageBuilder<
*/
public abstract Supplier<CommandResult> getOperationPage();

/**
* Sets if the error object v2 formatting should be used when building the {@link CommandResult}.
*/
@SuppressWarnings("unchecked")
public <SubT extends OperationAttemptPageBuilder<SchemaT, AttemptT>> SubT useErrorObjectV2(
boolean useErrorObjectV2) {
this.useErrorObjectV2 = useErrorObjectV2;
return (SubT) this;
}

/**
* Set if API is running in debug mode, this adds additional info to the response. See {@link
* io.stargate.sgv2.jsonapi.api.model.command.CommandResultBuilder}.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -139,8 +139,8 @@ public ReadAttemptPage<SchemaT> getOperationPage() {

var resultBuilder =
singleResponse
? CommandResult.singleDocumentBuilder(useErrorObjectV2, debugMode)
: CommandResult.multiDocumentBuilder(useErrorObjectV2, debugMode);
? CommandResult.singleDocumentBuilder(debugMode)
: CommandResult.multiDocumentBuilder(debugMode);

return new ReadAttemptPage<>(
attempts, resultBuilder, pagingState, includeSortVector, sortVector);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,8 @@ public CommandResult get() {

var builder =
singleResponse
? CommandResult.singleDocumentBuilder(false, false)
: CommandResult.multiDocumentBuilder(false, false);
? CommandResult.singleDocumentBuilder(false)
: CommandResult.multiDocumentBuilder(false);

if (includeSortVector) {
builder.addStatus(CommandStatus.SORT_VECTOR, vector);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,7 @@ public static class Builder<SchemaT extends SchemaObject>

@Override
public SchemaAttemptPage<SchemaT> getOperationPage() {
return new SchemaAttemptPage<>(
attempts, CommandResult.statusOnlyBuilder(useErrorObjectV2, debugMode));
return new SchemaAttemptPage<>(attempts, CommandResult.statusOnlyBuilder(debugMode));
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,7 @@ public static class Builder<SchemaT extends TableBasedSchemaObject>
@Override
public TruncateAttemptPage<SchemaT> getOperationPage() {

return new TruncateAttemptPage<>(
attempts, CommandResult.statusOnlyBuilder(useErrorObjectV2, debugMode));
return new TruncateAttemptPage<>(attempts, CommandResult.statusOnlyBuilder(debugMode));
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,7 @@ public UpdateAttemptPage<SchemaT> getOperationPage() {
// when we refactor collections to use the OperationAttempt this will need to support
// returning a document
// e.g. for findOneAndDelete, for now it is always status only
return new UpdateAttemptPage<>(
attempts, CommandResult.statusOnlyBuilder(useErrorObjectV2, debugMode));
return new UpdateAttemptPage<>(attempts, CommandResult.statusOnlyBuilder(debugMode));
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,7 @@ public record CountOperationPage(long count, boolean moreData) implements Suppli
public CommandResult get() {

var builder =
CommandResult.statusOnlyBuilder(false, false)
.addStatus(CommandStatus.COUNTED_DOCUMENT, count);
CommandResult.statusOnlyBuilder(false).addStatus(CommandStatus.COUNTED_DOCUMENT, count);
if (moreData) {
builder.addStatus(CommandStatus.MORE_DATA, true);
}
Expand Down
Loading
Loading