-
Notifications
You must be signed in to change notification settings - Fork 16
WIP: Implement support for application/octet-stream #83
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
Changes from all commits
de22b47
57345b5
232f717
025661f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,6 +15,7 @@ | |
| import io.vertx.core.Future; | ||
| import io.vertx.core.Vertx; | ||
| import io.vertx.core.http.HttpServerRequest; | ||
| import io.vertx.json.schema.JsonSchema; | ||
| import io.vertx.json.schema.JsonSchemaValidationException; | ||
| import io.vertx.json.schema.OutputUnit; | ||
| import io.vertx.openapi.contract.MediaType; | ||
|
|
@@ -153,6 +154,28 @@ RequestParameter validateBody(RequestBody requestBody, ValidatableRequest reques | |
| throw new ValidatorException("The format of the request body is not supported", UNSUPPORTED_VALUE_FORMAT); | ||
| } | ||
| Object transformedValue = transformer.transformRequest(mediaType, request); | ||
| // Corner case for "application/octet-stream" media type: | ||
| // Skip full schema validation if the following simplified validation succeeds, | ||
| // otherwise validate with JSON Validator and fail the validation | ||
| // of the transformed value eventually. | ||
| if (mediaType.getIdentifier().equals(MediaType.APPLICATION_OCTET_STREAM)) { | ||
| JsonSchema schema = mediaType.getSchema(); | ||
| String schemaTypeValue = schema.get("type", null); | ||
pk-work marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| if (schemaTypeValue != null && schemaTypeValue.equals("string")) { | ||
| String schemaFormatValue = schema.get("format", null); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. for
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The simplified validation procedure has to deal with both 3.0 and 3.1 of OpenAPI. Therefore the statements used seem to be redundant.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay, you are right, format can be null. But as mentioned in the comment below, I'm not convinced by the current implementation of the "schema validation bypass". My concerns:
But I understand the benefit of having a schema validation bypass, especially for binary data. Maybe we can call a method like "canSkipValidation" and extract this method and its logic into a separated class. You could pass the contract reference to this method to check if the contract is 3.0 or 3.1 which makes the logic in this method less complex. What do you think? |
||
| // Case for OpenAPI 3.0 and OpenAPI 3.1 (backwards compatibility) | ||
| if (schemaFormatValue != null && schemaFormatValue.equals("binary")) { | ||
| return new RequestParameterImpl(transformedValue); | ||
| } else { | ||
| // Case only for OpenAPI 3.1 | ||
| String schemaContentMediaType = schema.get("contentMediaType", null); | ||
| if (schemaContentMediaType != null | ||
| && schemaContentMediaType.equals(MediaType.APPLICATION_OCTET_STREAM)) { | ||
| return new RequestParameterImpl(transformedValue); | ||
|
Comment on lines
+167
to
+174
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't agree to this. As long as the type is
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry I don't understand that one? The The verification of the value of
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What I mean is, that you skip the validation. Couldn't it be possible to define a schema like this? |
||
| } | ||
| } | ||
| } | ||
| } | ||
| OutputUnit result = contract.getSchemaRepository().validator(mediaType.getSchema()).validate(transformedValue); | ||
|
|
||
| try { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| /* | ||
| * Copyright (c) 2024, Lucimber UG | ||
| * | ||
| * This program and the accompanying materials are made available under the | ||
| * terms of the Eclipse Public License 2.0 which is available at | ||
| * http://www.eclipse.org/legal/epl-2.0, or the Apache License, Version 2.0 | ||
| * which is available at https://www.apache.org/licenses/LICENSE-2.0. | ||
| * | ||
| * SPDX-License-Identifier: EPL-2.0 OR Apache-2.0 | ||
| * | ||
| */ | ||
|
|
||
| package io.vertx.openapi.validation.transformer; | ||
|
|
||
| import io.vertx.core.buffer.Buffer; | ||
| import io.vertx.openapi.contract.MediaType; | ||
| import io.vertx.openapi.validation.ValidatableRequest; | ||
| import io.vertx.openapi.validation.ValidatableResponse; | ||
| import io.vertx.openapi.validation.ValidatorException; | ||
|
|
||
| import static io.vertx.openapi.contract.MediaType.APPLICATION_OCTET_STREAM; | ||
| import static io.vertx.openapi.validation.ValidatorErrorType.MISSING_REQUIRED_PARAMETER; | ||
|
|
||
| public class ApplicationOctetStreamTransformer implements BodyTransformer { | ||
|
|
||
| @Override | ||
| public Object transformRequest(MediaType type, ValidatableRequest request) { | ||
| return transform(type, request.getBody().getBuffer(), request.getContentType(), "request"); | ||
| } | ||
|
|
||
| @Override | ||
| public Object transformResponse(MediaType type, ValidatableResponse response) { | ||
| return transform(type, response.getBody().getBuffer(), response.getContentType(), "response"); | ||
| } | ||
|
|
||
| private Object transform(MediaType type, Buffer body, String contentType, | ||
| String responseOrRequest) { | ||
| if (contentType == null || contentType.isEmpty() | ||
| || !contentType.equalsIgnoreCase(APPLICATION_OCTET_STREAM)) { | ||
| String msg = "The " + responseOrRequest | ||
| + " doesn't contain the required content-type header application/octet-stream."; | ||
| throw new ValidatorException(msg, MISSING_REQUIRED_PARAMETER); | ||
| } | ||
pk-work marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| return body; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,75 @@ | ||
| /* | ||
| * Copyright (c) 2024, Lucimber UG | ||
| * | ||
| * This program and the accompanying materials are made available under the | ||
| * terms of the Eclipse Public License 2.0 which is available at | ||
| * http://www.eclipse.org/legal/epl-2.0, or the Apache License, Version 2.0 | ||
| * which is available at https://www.apache.org/licenses/LICENSE-2.0. | ||
| * | ||
| * SPDX-License-Identifier: EPL-2.0 OR Apache-2.0 | ||
| * | ||
| */ | ||
|
|
||
| package io.vertx.openapi.validation.transformer; | ||
|
|
||
| import io.vertx.core.buffer.Buffer; | ||
| import io.vertx.openapi.validation.ValidatableRequest; | ||
| import io.vertx.openapi.validation.ValidatableResponse; | ||
| import io.vertx.openapi.validation.ValidatorException; | ||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| import java.util.Random; | ||
|
|
||
| import static com.google.common.truth.Truth.assertThat; | ||
| import static io.netty.handler.codec.http.HttpHeaderValues.APPLICATION_JSON; | ||
| import static io.netty.handler.codec.http.HttpHeaderValues.APPLICATION_OCTET_STREAM; | ||
| import static io.vertx.openapi.MockHelper.mockValidatableRequest; | ||
| import static io.vertx.openapi.validation.ValidatorErrorType.MISSING_REQUIRED_PARAMETER; | ||
| import static org.junit.jupiter.api.Assertions.assertThrows; | ||
|
|
||
| class ApplicationOctetStreamTransformerTest { | ||
| private final BodyTransformer transformer = new ApplicationOctetStreamTransformer(); | ||
| private final Random random = new Random(); | ||
|
|
||
| @Test | ||
| void testTransformRequest() { | ||
| byte[] bytes = new byte[102400]; // Mimic body of 100 Kibibyte | ||
| random.nextBytes(bytes); | ||
| Buffer dummyBody = Buffer.buffer(bytes); | ||
| ValidatableRequest request = mockValidatableRequest(dummyBody, APPLICATION_OCTET_STREAM.toString()); | ||
| assertThat(transformer.transformRequest(null, request)).isEqualTo(dummyBody); | ||
| } | ||
|
|
||
| @Test | ||
| void testTransformRequestThrows() { | ||
| ValidatorException exception = | ||
| assertThrows(ValidatorException.class, () -> transformer.transformRequest(null, | ||
| mockValidatableRequest(Buffer.buffer("\"foobar"), APPLICATION_JSON.toString()))); | ||
| String expectedMsg = "The request doesn't contain" + | ||
| " the required content-type header application/octet-stream."; | ||
| assertThat(exception.type()).isEqualTo(MISSING_REQUIRED_PARAMETER); | ||
| assertThat(exception).hasMessageThat().isEqualTo(expectedMsg); | ||
| } | ||
|
|
||
| @Test | ||
| void testTransformResponse() { | ||
| byte[] bytes = new byte[102400]; // Mimic body of 100 Kibibyte | ||
| random.nextBytes(bytes); | ||
| Buffer dummyBody = Buffer.buffer(bytes); | ||
| ValidatableResponse response = ValidatableResponse.create(200, dummyBody, | ||
| APPLICATION_OCTET_STREAM.toString()); | ||
| assertThat(transformer.transformResponse(null, response)).isEqualTo(dummyBody); | ||
| } | ||
|
|
||
| @Test | ||
| void testTransformResponseThrows() { | ||
| ValidatableResponse response = ValidatableResponse | ||
| .create(200, Buffer.buffer("\"foobar"), APPLICATION_JSON.toString()); | ||
| ValidatorException exception = | ||
| assertThrows(ValidatorException.class, () -> transformer.transformResponse(null, response)); | ||
| String expectedMsg = "The response doesn't contain" + | ||
| " the required content-type header application/octet-stream."; | ||
| assertThat(exception.type()).isEqualTo(MISSING_REQUIRED_PARAMETER); | ||
| assertThat(exception).hasMessageThat().isEqualTo(expectedMsg); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should become a constant maybe in
MediaType. I would also removecontentMediaTypeas in the example only format and type is mentioned. This is then also compatible with 3.0There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll replace it as a constant in
MediaType.But stripping the
contentMediaTypeisn't a good idea, because it replacesformatin OpenAPI 3.1. I've intentionally placed both keys in that JSON object to be compatible with OpenAPI 3.x.That way it's future proof, because later adaptions of this library for OpenAPI 3.1 or later might break.
Am I overthinking that part?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mh maybe you are right, but I think we can simply ignore
contentMediaType, because the MediaType is already specified withif (identifier.equalsIgnoreCase(MediaType.APPLICATION_OCTET_STREAM)) {Would you agree?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If someone uses the alias
application/octet-stream: {}, it's safe to assume that the contract file is of version 3.1, because that wasn't allowed before in 3.0 and lower. Therefore I'm removingformatfrom theschemaJson.The final schema object that gets injected into the media type object would look like the following:
Yeah, that makes sense to ignore it here.
Summary: For contract validation we place a default schema object like seen above, if someone uses the alias definition introduced in OpenAPI 3.1. For request validation we simplify the validation if the media type object ID is
application/octet-stream.Sounds good?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the benefit of having
contentMediaTypein the default schema object? I still don't understand it.Why can't we simply use
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default schema object must only be applied if someone has an
openapi-contract.ymlfile with OpenAPI 3.1. Before that, no shortcuts like{}could be used in the schema file.In OpenAPI 3.0 you must write:
In OpenAPI 3.1 you (can) now write:
And that gets expanded to:
It really is confusing! As of my understanding, in OpenAPI 3.1,
formathas been replaced bycontentMediaTypewhen dealing withSchema Objects in relation to binary file uploads. See https://spec.openapis.org/oas/latest.html#considerations-for-file-uploadsThe task of this library must be the expansion before the validation of the contract. Right now I'm only providing that expansion for
application/octet-stream, because everything else isn't part of this PR.We could still use your approach of the old way of declaring it, but maybe that will create confusion in later stages of development of this library? Supporting multiple versions of OpenAPI is challenging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Later in the code, if necessary, you can distinguish between 3.0 and 3.1 only be checking the keys of the schema object. If
formatis missing butcontentMediaTypeis present, it must be 3.1. Otherwise it will be 3.0 schema. This might be come in handy...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I don't get the specs .... maybe we don't need a schema object at all. It seems like it is intentionally left out when using
{}in the contract.Then it is treated like
OK, so leave out the schema object? What do you think?