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.

Sign up for GitHub

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

415 for invalid Content-Type header, remove redundant Token Filter #1298

Merged
merged 5 commits into from
Jul 24, 2024
Merged

415 for invalid Content-Type header, remove redundant Token Filter #1298

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
37bc0e8
init
Yuqi-Du Jul 23, 2024
fbea907
Merge branch 'main' into validate-content-type-header
Yuqi-Du Jul 23, 2024
d5ad168
fix comments
Yuqi-Du Jul 24, 2024
6b997ba
Merge remote-tracking branch 'origin/validate-content-type-header' in…
Yuqi-Du Jul 24, 2024
66ac32e
Merge branch 'main' into validate-content-type-header
Yuqi-Du Jul 24, 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
32 changes: 0 additions & 32 deletions src/main/java/io/stargate/sgv2/jsonapi/api/request/token/TokenFilter.java
View file
Open in desktop

This file was deleted.

2 changes: 2 additions & 0 deletions src/main/java/io/stargate/sgv2/jsonapi/exception/ErrorCode.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,8 @@ public enum ErrorCode {

INVALID_USAGE_OF_VECTORIZE("`$vectorize` and `$vector` can't be used together"),

INVALID_CONTENT_TYPE_HEADER("Invalid Content-Type header"),

UNSUPPORTED_PROJECTION_PARAM("Unsupported projection parameter"),

UNSUPPORTED_UPDATE_DATA_TYPE("Unsupported update data type"),
Expand Down
12 changes: 12 additions & 0 deletions src/main/java/io/stargate/sgv2/jsonapi/exception/mappers/ThrowableToErrorMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import io.stargate.sgv2.jsonapi.api.model.command.CommandResult;
import io.stargate.sgv2.jsonapi.exception.ErrorCode;
import io.stargate.sgv2.jsonapi.exception.JsonApiException;
import jakarta.ws.rs.NotSupportedException;
import jakarta.ws.rs.core.Response;
import java.util.Collection;
import java.util.Collections;
Expand Down Expand Up @@ -69,6 +70,17 @@ public final class ThrowableToErrorMapper {
return handleDriverException((DriverException) throwable, message);
}

// handle an invalid Content-Type header
if (throwable instanceof NotSupportedException
&& throwable
.getMessage()
.contains("The content-type header value did not match the value")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this check needed? Wouldn't we always convert this exception to 415?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, let me change that.
I was just try to make sure.

// validate the Content-Type header, 415 if failed
return ErrorCode.INVALID_CONTENT_TYPE_HEADER
.toApiException()
.getCommandResultError(Response.Status.UNSUPPORTED_MEDIA_TYPE);
Copy link
Contributor

Choose a reason for hiding this comment

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

It's too bad actual incorrect Content-Type seems not available from NotSupportedException -- if it was, would be good to include in exception message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that is not included, unfortunately.

}

// handle all other exceptions
return handleUnrecognizedException(throwable, message);
};
Expand Down
7 changes: 7 additions & 0 deletions src/main/java/io/stargate/sgv2/jsonapi/exception/mappers/WebApplicationExceptionMapper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import io.stargate.sgv2.jsonapi.exception.JsonApiException;
import jakarta.ws.rs.NotAllowedException;
import jakarta.ws.rs.NotFoundException;
import jakarta.ws.rs.NotSupportedException;
import jakarta.ws.rs.WebApplicationException;
import org.jboss.resteasy.reactive.RestResponse;
import org.jboss.resteasy.reactive.server.ServerExceptionMapper;
Expand Down Expand Up @@ -42,6 +43,12 @@ public RestResponse<CommandResult> webApplicationExceptionMapper(WebApplicationE
if (e instanceof NotFoundException) {
return RestResponse.status(RestResponse.Status.NOT_FOUND, commandResult);
}
// Return 415 for invalid Content-Type
if (e instanceof NotSupportedException
Copy link
Contributor

Choose a reason for hiding this comment

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

It's odd we have to do this in 2 different places -- do you know which one is necessary for the test to pass? Is it possible to test both handlers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is because our current Exception structure design.
So this failure happens before our command processing, and it is captured by this WebApplicationExceptionMapper, but we still need want to convert the Throwable to CommandResult, so the same logic goes to ThrowableToErrorMapper.
Here we just want the status 415 to be added.

&& e.getMessage().contains("The content-type header value did not match the value")) {
return RestResponse.status(RestResponse.Status.UNSUPPORTED_MEDIA_TYPE, commandResult);
}

return RestResponse.ok(commandResult);
}
}
21 changes: 21 additions & 0 deletions src/test/java/io/stargate/sgv2/jsonapi/api/v1/HttpStatusCodeIntegrationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,27 @@ public void regularError() {
.body("errors[0].exceptionClass", is("JsonApiException"));
}

@Test
public void invalidContentType() {
given()
.headers(getHeaders())
.contentType(ContentType.HTML)
.body(
"""
{
"findCollections": { }
}
""")
.when()
.post(NamespaceResource.BASE_PATH, namespaceName)
.then()
.statusCode(415)
.body("errors", is(notNullValue()))
.body("errors[0].message", is("Invalid Content-Type header"))
.body("errors[0].errorCode", is("INVALID_CONTENT_TYPE_HEADER"))
.body("errors[0].exceptionClass", is("JsonApiException"));
}

@Test
public void resourceNotFound() {
String json =
Expand Down