eclipse-vertx / vertx-openapi

OpenAPI repository
Other
10 stars 6 forks source link

WIP: Implement support for application/octet-stream #83

Open sebveit opened 1 month ago

sebveit commented 1 month ago

Motivation:

OpenAPI 3.1 supports media type "application/octet-stream" in the request body. See the migration guide: https://www.openapis.org/blog/2021/02/16/migrating-from-openapi-3-0-to-3-1-0

Conformance:

I've signed the Eclipse Contributor Agreement as explained in https://github.com/eclipse/vert.x/blob/master/CONTRIBUTING.md

Current state of PR:

Pascal and I had trouble figuring out what the OpenAPI spec actually defines in regards of octet-stream/binary related endpoints. This confusion seems to be fixed with OpenAPI spec of version 3.1.1. For now, this PR is on hold until Pascal tells otherwise.

sebveit commented 1 month ago

Related to issue #82

pk-work commented 1 month ago

Hi @sebveit,

thanks for your contribution, help is always welcome! Your PR is a good start, but to support a new content type it requires a little bit more effort than just extending the list with allowed values.

First we need to add a BodyTransformer [1] for application/octet-steam. Here are examples of already existing transformers [2][3]. Then we need to add the mapping in BaseValidator [4]. Then we can extend the list of supported values.

To keep quality high, it is also required to provide unit tests for the new functionality.

I hope this helps you to extend your PR. let me know if you need more insights.

[1] https://github.com/eclipse-vertx/vertx-openapi/blob/main/src/main/java/io/vertx/openapi/validation/transformer/BodyTransformer.java [2] https://github.com/eclipse-vertx/vertx-openapi/blob/main/src/main/java/io/vertx/openapi/validation/transformer/ApplicationJsonTransformer.java [3] https://github.com/eclipse-vertx/vertx-openapi/blob/main/src/main/java/io/vertx/openapi/validation/transformer/MultipartFormTransformer.java [4] https://github.com/eclipse-vertx/vertx-openapi/blob/fb7baab58444dfe503161436b99f2f6d237a5f92/src/main/java/io/vertx/openapi/validation/impl/BaseValidator.java#L40

sebveit commented 1 month ago

Hi @pk-work, nice to "meet" you! :-) You're right, I thought it would be a quick win before I stumbled on the validation package! Thanks for pointing out where to go. I will try to come up with a solution. Thx for guiding! If questions arise, I'll ask you!

sebveit commented 1 month ago

I've written the corresponding transformer and tests. Maybe you take a look if it's ok?

One thing I don't know if it needs to change, is the way Media Type Object of OpenAPI are handled by this project. In the migration guide of OpenAPI 3.1, there's an example of a media type without further content: application/octet-stream: {}

Before 3.1, it seems that you had to write something like the following:

application/octet-stream:
  schema:
    type: string
    format: binary

Migration guide: https://www.openapis.org/blog/2021/02/16/migrating-from-openapi-3-0-to-3-1-0

Is the current implementation OK with such an empty Media Type Object?

sebveit commented 4 weeks ago

I've fixed the following exception by providing a default schema for application/octet-stream, if the shortcut declaration is used - which is mentioned in the migration guide of OpenAPI 3.1. Encountered exception:

io.vertx.openapi.contract.OpenAPIContractException: The passed OpenAPI contract contains a feature that is not supported: Media Type without a schema
2024-08-19 11:31:30     at io.vertx.openapi.contract.OpenAPIContractException.createUnsupportedFeature(OpenAPIContractException.java:43)
2024-08-19 11:31:30     at io.vertx.openapi.contract.impl.MediaTypeImpl.<init>(MediaTypeImpl.java:33)
2024-08-19 11:31:30     at io.vertx.openapi.contract.impl.RequestBodyImpl.lambda$new$1(RequestBodyImpl.java:58)

Related commit 232f717.

sebveit commented 4 weeks ago

Now, I'm confronted with another exception:

java.lang.IllegalArgumentException: Instances of class io.vertx.core.buffer.impl.BufferImpl type are not supported
2024-08-19 11:58:11     at io.vertx.json.schema.impl.Utils$JSON.typeOf(Utils.java:214)
2024-08-19 11:58:11     at io.vertx.json.schema.impl.SchemaValidatorImpl.validate(SchemaValidatorImpl.java:101)
2024-08-19 11:58:11     at io.vertx.json.schema.impl.SchemaValidatorImpl.validate(SchemaValidatorImpl.java:54)
2024-08-19 11:58:11     at io.vertx.openapi.validation.impl.RequestValidatorImpl.validateBody(RequestValidatorImpl.java:156)
2024-08-19 11:58:11     at io.vertx.openapi.validation.impl.RequestValidatorImpl.lambda$validate$2(RequestValidatorImpl.java:106)

I'm getting deeper into the rabbit hole! What I previously considered a quick fix is now expanding to three projects. I'm beginning to understand why this hasn't been implemented yet.

Could I just catch that exception if it is a Buffer(Impl) type and pretend nothing happened and the validation is a success?

pk-work commented 3 weeks ago

Hi, currently I'm on PTO. I will have a look at the end of the week or next week.

sebveit commented 3 weeks ago

Thanks for the reply! Take you time! I've got a workaround for the actual project that depends on those changes by creating a local snapshot version with the modifications committed here.

The whole point of this project/lib seems to be validation of a given OpenAPI schema and the related request/response pairs. While that's true for the JSON or anything else expressed as strings, explicit binary values cannot really be validated by OpenAPI itself.

This PR needs a bit more polishing and additional tests before it can be merged, I guess. Nevertheless, I think that my commits are a good approach to the problem.

pk-work commented 3 weeks ago

The whole point of this project/lib seems to be validation of a given OpenAPI schema and the related request/response pairs. While that's true for the JSON or anything else expressed as strings, explicit binary values cannot really be validated by OpenAPI itself.

That's true when it comes to the validation of a binary string, you are very limited :)

This PR needs a bit more polishing and additional tests before it can be merged, I guess. Nevertheless, I think that my commits are a good approach to the problem.

Yes your commits are a very good start. Do you want to do the polishing and add missing tests, or should I take over? If I continue it might take a little bit longer, but as I understood you have already a work around.

sebveit commented 3 weeks ago

Yes your commits are a very good start. Do you want to do the polishing and add missing tests, or should I take over? If I continue it might take a little bit longer, but as I understood you have already a work around.

I'd like to finish this PR, since I'm using vertx alot and it's time to give back. The problem that I'm facing with OpenAPI and octet-stream is a good way to do so.

Thanks for the current review, I'm going to submit the changes/tests!