Skip to content

Commit

Permalink
Revert "REST: implement handling of OAuth error responses (apache#5698)…
Browse files Browse the repository at this point in the history 
…" (apache#5810)

This reverts commit c293af2.
  • Loading branch information
@danielcweeks
danielcweeks authored Sep 20, 2022
1 parent 53e056a commit 230dee0
Show file tree
Hide file tree
Showing 19 changed files with 136 additions and 538 deletions.
26 changes: 0 additions & 26 deletions core/src/main/java/org/apache/iceberg/rest/ErrorHandler.java
View file

This file was deleted.

122 changes: 27 additions & 95 deletions core/src/main/java/org/apache/iceberg/rest/ErrorHandlers.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
*/
package org.apache.iceberg.rest;

import java.util.function.Consumer;
import org.apache.iceberg.exceptions.AlreadyExistsException;
import org.apache.iceberg.exceptions.BadRequestException;
import org.apache.iceberg.exceptions.CommitFailedException;
Expand All @@ -29,11 +30,7 @@
import org.apache.iceberg.exceptions.RESTException;
import org.apache.iceberg.exceptions.ServiceFailureException;
import org.apache.iceberg.exceptions.ServiceUnavailableException;
import org.apache.iceberg.rest.auth.OAuth2Properties;
import org.apache.iceberg.rest.responses.ErrorResponse;
import org.apache.iceberg.rest.responses.ErrorResponseParser;
import org.apache.iceberg.rest.responses.OAuthErrorResponse;
import org.apache.iceberg.rest.responses.OAuthErrorResponseParser;

/**
* A set of consumers to handle errors for requests for table entities or for namespace entities, to
Expand All @@ -43,33 +40,20 @@ public class ErrorHandlers {

private ErrorHandlers() {}

public static ErrorHandler namespaceErrorHandler() {
return NamespaceErrorHandler.INSTANCE;
public static Consumer<ErrorResponse> namespaceErrorHandler() {
return baseNamespaceErrorHandler().andThen(defaultErrorHandler());
}

public static ErrorHandler tableErrorHandler() {
return TableErrorHandler.INSTANCE;
public static Consumer<ErrorResponse> tableErrorHandler() {
return baseTableErrorHandler().andThen(defaultErrorHandler());
}

public static ErrorHandler tableCommitHandler() {
return CommitErrorHandler.INSTANCE;
public static Consumer<ErrorResponse> tableCommitHandler() {
return baseCommitErrorHandler().andThen(defaultErrorHandler());
}

public static ErrorHandler defaultErrorHandler() {
return DefaultErrorHandler.INSTANCE;
}

public static ErrorHandler oauthErrorHandler() {
return OAuthErrorHandler.INSTANCE;
}

/** Table commit error handler. */
private static class CommitErrorHandler extends DefaultErrorHandler {
private static final ErrorHandler INSTANCE = new CommitErrorHandler();

@Override
public void handle(RESTErrorResponse restError) {
ErrorResponse error = (ErrorResponse) restError;
private static Consumer<ErrorResponse> baseCommitErrorHandler() {
return error -> {
switch (error.code()) {
case 404:
throw new NoSuchTableException("%s", error.message());
Expand All @@ -80,18 +64,15 @@ public void handle(RESTErrorResponse restError) {
throw new CommitStateUnknownException(
new ServiceFailureException("Service failed: %s: %s", error.code(), error.message()));
}

super.handle(restError);
}
};
}

/** Table level error handler. */
private static class TableErrorHandler extends DefaultErrorHandler {
private static final ErrorHandler INSTANCE = new TableErrorHandler();

@Override
public void handle(RESTErrorResponse restError) {
ErrorResponse error = (ErrorResponse) restError;
/**
* Table level error handlers. Should be chained wih the {@link #defaultErrorHandler}, which takes
* care of common cases.
*/
private static Consumer<ErrorResponse> baseTableErrorHandler() {
return error -> {
switch (error.code()) {
case 404:
if (NoSuchNamespaceException.class.getSimpleName().equals(error.type())) {
Expand All @@ -102,18 +83,15 @@ public void handle(RESTErrorResponse restError) {
case 409:
throw new AlreadyExistsException("%s", error.message());
}

super.handle(restError);
}
};
}

/** Request error handler specifically for CRUD ops on namespaces. */
private static class NamespaceErrorHandler extends DefaultErrorHandler {
private static final ErrorHandler INSTANCE = new NamespaceErrorHandler();

@Override
public void handle(RESTErrorResponse restError) {
ErrorResponse error = (ErrorResponse) restError;
/**
* Request error handlers specifically for CRUD ops on namespaces. Should be chained wih the
* {@link #defaultErrorHandler}, which takes care of common cases.
*/
private static Consumer<ErrorResponse> baseNamespaceErrorHandler() {
return error -> {
switch (error.code()) {
case 404:
throw new NoSuchNamespaceException("%s", error.message());
Expand All @@ -122,26 +100,15 @@ public void handle(RESTErrorResponse restError) {
case 422:
throw new RESTException("Unable to process: %s", error.message());
}

super.handle(restError);
}
};
}

/**
* Request error handler that handles the common cases that are included with all responses, such
* as 400, 500, etc.
*/
private static class DefaultErrorHandler implements ErrorHandler {
private static final ErrorHandler INSTANCE = new DefaultErrorHandler();

@Override
public RESTErrorResponse parseResponse(int code, String json) {
return ErrorResponseParser.fromJson(json);
}

@Override
public void handle(RESTErrorResponse restError) {
ErrorResponse error = (ErrorResponse) restError;
public static Consumer<ErrorResponse> defaultErrorHandler() {
return error -> {
switch (error.code()) {
case 400:
throw new BadRequestException("Malformed request: %s", error.message());
Expand All @@ -161,41 +128,6 @@ public void handle(RESTErrorResponse restError) {
}

throw new RESTException("Unable to process: %s", error.message());
}
}

private static class OAuthErrorHandler implements ErrorHandler {
private static final ErrorHandler INSTANCE = new OAuthErrorHandler();
private static final String SERVER_ERROR = "server_error";

@Override
public RESTErrorResponse parseResponse(int code, String json) {
if (code == 400 || code == 401) {
return OAuthErrorResponseParser.fromJson(json);
}
return OAuthErrorResponse.builder()
.withError(SERVER_ERROR)
.withErrorDescription(json)
.build();
}

@Override
public void handle(RESTErrorResponse restError) {
if (restError instanceof OAuthErrorResponse) {
OAuthErrorResponse error = (OAuthErrorResponse) restError;
switch (error.error()) {
case OAuth2Properties.INVALID_CLIENT_ERROR:
throw new NotAuthorizedException("Not authorized: %s", error.errorDescription());
case OAuth2Properties.INVALID_REQUEST_ERROR:
case OAuth2Properties.INVALID_GRANT_ERROR:
case OAuth2Properties.UNAUTHORIZED_CLIENT_ERROR:
case OAuth2Properties.UNSUPPORTED_GRANT_TYPE_ERROR:
case OAuth2Properties.INVALID_SCOPE_ERROR:
throw new BadRequestException("Malformed request: %s", error.errorDescription());
default:
throw new RESTException("Unable to process: %s", error.errorDescription());
}
}
}
};
}
}
27 changes: 16 additions & 11 deletions core/src/main/java/org/apache/iceberg/rest/HTTPClient.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import java.net.URI;
import java.net.URISyntaxException;
import java.util.Map;
import java.util.function.Consumer;
import org.apache.hc.client5.http.classic.methods.HttpUriRequest;
import org.apache.hc.client5.http.classic.methods.HttpUriRequestBase;
import org.apache.hc.client5.http.impl.classic.CloseableHttpClient;
Expand All @@ -45,6 +46,7 @@
import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
import org.apache.iceberg.relocated.com.google.common.collect.Maps;
import org.apache.iceberg.rest.responses.ErrorResponse;
import org.apache.iceberg.rest.responses.ErrorResponseParser;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

Expand Down Expand Up @@ -86,7 +88,7 @@ private static boolean isSuccessful(CloseableHttpResponse response) {
|| code == HttpStatus.SC_NO_CONTENT;
}

private static RESTErrorResponse buildDefaultErrorResponse(CloseableHttpResponse response) {
private static ErrorResponse buildDefaultErrorResponse(CloseableHttpResponse response) {
String responseReason = response.getReasonPhrase();
String message =
responseReason != null && !responseReason.isEmpty()
Expand All @@ -103,12 +105,12 @@ private static RESTErrorResponse buildDefaultErrorResponse(CloseableHttpResponse
// Process a failed response through the provided errorHandler, and throw a RESTException if the
// provided error handler doesn't already throw.
private static void throwFailure(
CloseableHttpResponse response, String responseBody, ErrorHandler errorHandler) {
RESTErrorResponse errorResponse = null;
CloseableHttpResponse response, String responseBody, Consumer<ErrorResponse> errorHandler) {
ErrorResponse errorResponse = null;

if (responseBody != null) {
try {
errorResponse = errorHandler.parseResponse(response.getCode(), responseBody);
errorResponse = ErrorResponseParser.fromJson(responseBody);
} catch (UncheckedIOException | IllegalArgumentException e) {
// It's possible to receive a non-successful response that isn't a properly defined
// ErrorResponse
Expand All @@ -127,7 +129,7 @@ private static void throwFailure(
errorResponse = buildDefaultErrorResponse(response);
}

errorHandler.handle(errorResponse);
errorHandler.accept(errorResponse);

// Throw an exception in case the provided error handler does not throw.
throw new RESTException("Unhandled error: %s", errorResponse);
Expand Down Expand Up @@ -169,7 +171,7 @@ private <T> T execute(
Object requestBody,
Class<T> responseType,
Map<String, String> headers,
ErrorHandler errorHandler) {
Consumer<ErrorResponse> errorHandler) {
if (path.startsWith("/")) {
throw new RESTException(
"Received a malformed path for a REST request: %s. Paths should not start with /", path);
Expand Down Expand Up @@ -225,7 +227,7 @@ private <T> T execute(
}

@Override
public void head(String path, Map<String, String> headers, ErrorHandler errorHandler) {
public void head(String path, Map<String, String> headers, Consumer<ErrorResponse> errorHandler) {
execute(Method.HEAD, path, null, null, null, headers, errorHandler);
}

Expand All @@ -235,7 +237,7 @@ public <T extends RESTResponse> T get(
Map<String, String> queryParams,
Class<T> responseType,
Map<String, String> headers,
ErrorHandler errorHandler) {
Consumer<ErrorResponse> errorHandler) {
return execute(Method.GET, path, queryParams, null, responseType, headers, errorHandler);
}

Expand All @@ -245,13 +247,16 @@ public <T extends RESTResponse> T post(
RESTRequest body,
Class<T> responseType,
Map<String, String> headers,
ErrorHandler errorHandler) {
Consumer<ErrorResponse> errorHandler) {
return execute(Method.POST, path, null, body, responseType, headers, errorHandler);
}

@Override
public <T extends RESTResponse> T delete(
String path, Class<T> responseType, Map<String, String> headers, ErrorHandler errorHandler) {
String path,
Class<T> responseType,
Map<String, String> headers,
Consumer<ErrorResponse> errorHandler) {
return execute(Method.DELETE, path, null, null, responseType, headers, errorHandler);
}

Expand All @@ -261,7 +266,7 @@ public <T extends RESTResponse> T postForm(
Map<String, String> formData,
Class<T> responseType,
Map<String, String> headers,
ErrorHandler errorHandler) {
Consumer<ErrorResponse> errorHandler) {
return execute(Method.POST, path, null, formData, responseType, headers, errorHandler);
}

Expand Down
Loading

0 comments on commit 230dee0

Please sign in to comment.