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

make schema nullable instead of adding null-types into the apispec schema #3331

Merged
merged 1 commit into from
Nov 22, 2023

Conversation

cornerman
Copy link
Contributor

These null-types are only supported in openapi 3.1.0. For 3.0.3, we need a different encoding.
Therefore, we should leave the distinction to sttp-apispec which handles this already, instead of deciding here in tapir.

@adamw
Copy link
Member

adamw commented Nov 21, 2023

Thanks for the investigation! It seems there are some test failures, though - maybe changes are required in both apispec & tapir.

@cornerman
Copy link
Contributor Author

I see, though, I actually have difficulties building locally, even on master.

I am on nix, so I did cd doc; nix develop; cd ../; sbt compile. But I get errors like this:

[error] /home/cornerman/old_home/projects/tapir/core/src/main/scala-2/sttp/tapir/macros/SchemaAnnotationsMacros.scala:7:80: macro implementation has incompatible shape:
[error]  required: (c: scala.reflect.macros.blackbox.Context): c.Expr[sttp.tapir.SchemaAnnotations[T]]
[error]  or      : (c: scala.reflect.macros.blackbox.Context): c.Tree
[error]  found   : (c: scala.reflect.macros.blackbox.Context): c.Expr[sttp.tapir.internal.SchemaAnnotations[T]]
[error] type mismatch for return type: c.Expr[sttp.tapir.internal.SchemaAnnotations[?T]] does not conform to c.Expr[sttp.tapir.SchemaAnnotations[T]]
[error]   implicit def derived[T]: SchemaAnnotations[T] = macro SchemaAnnotationsMacro.derived[T]
[error]                                                                                ^

or

[error] -- [E008] Not Found Error: /home/cornerman/old_home/projects/tapir/server/armeria-server/src/main/scala/sttp/tapir/server/armeria/RouteMapping.scala:21:13
[error] 21 |ExchangeType.UNARY
[error]    |^^^^^^^^^^^^^^^^^^
[error]    |value UNARY is not a member of object sttp.tapir.server.armeria.ExchangeType

Do you have an idea, why this happens?

@adamw
Copy link
Member

adamw commented Nov 21, 2023

Hm this looks quite weird, are you sure you have an up-to-date codebase? I'm not familiar with nix, but doing an sbt compile (or better, scoped to one project, such as sbt "openapiDocs/compile", as there's a lot of subprojects) should work. Which Java version do you use?

@cornerman
Copy link
Contributor Author

I am on the current master (commit 5350715). Using java version 20.

Running just sbt "openapiDocs/compile" works, other sub-projects seem to have issues though.

@cornerman
Copy link
Contributor Author

cornerman commented Nov 21, 2023

In the github-action logs, I cannot really see which tests are failing. Wondering what could be the reason that this change fails for java version 11 but not for java version 21 (only JVM though).

@adamw
Copy link
Member

adamw commented Nov 21, 2023

Well, let's stick with that then :)The test that fails is VerifyYamlTest, it's in that project (openapiDocs)

@cornerman
Copy link
Contributor Author

cornerman commented Nov 21, 2023

Thank you. I fixed the test, but I am guessing, we will now want to adapt sttp-apispec, to give us some encoding for the nullability for ref types in openapi 3.0.3. But this would make the spec valid again.

Imho, this should be something like in this stack-overflow thread: https://stackoverflow.com/questions/40920441/how-to-specify-a-property-can-be-null-or-a-reference-with-swagger

@adamw
Copy link
Member

adamw commented Nov 21, 2023

Thanks - though the test does expect a nullable field there, so yes, we do need to fix apispec :)

@cornerman
Copy link
Contributor Author

cornerman commented Nov 21, 2023

Additionally, we should have a test for openapi 3.1.0 and openapi 3.0.3. So, we are sure that all works.

Currently, we are testing only 3.1.0 with regards to nullability. For json, this line (https://github.com/softwaremill/sttp-apispec/blob/master/jsonschema-circe/src/main/scala/sttp/apispec/internal/JsonSchemaCirceEncoders.scala#L28) will probably take care of it.

One question about the openapi 3.0.3 support in apispec: Which of the two proposed solution makes sense for us? The first one with allOf and nullable sounds good to me and I saw it in other place before, but people complain about missing support in some tools.

@cornerman
Copy link
Contributor Author

Ok, actually I am a bit confused now. The yaml-codec uses the json codec internally, so I am wondering, why this code in sttp-apispec (https://github.com/softwaremill/sttp-apispec/blob/master/jsonschema-circe/src/main/scala/sttp/apispec/internal/JsonSchemaCirceEncoders.scala?rgh-link-date=2023-11-21T18%3A04%3A48Z#L28) does not have any effect when setting nullable to true.

@cornerman
Copy link
Contributor Author

I have now added a PR in sttp-apispec: softwaremill/sttp-apispec#131

The tests now pass again and I have added additional tests for openapi 3.0.3.

@cornerman cornerman force-pushed the openapi-303-nullability branch from c88f4b0 to d607fa5 Compare November 21, 2023 19:18
…hema

These null-types are only supported in openapi 3.1.0. For 3.0.3, we need a different encoding.
Therefore, we should leave the distinction to sttp-apispec, instead of deciding here in tapir.
@cornerman cornerman force-pushed the openapi-303-nullability branch from 5cc7719 to 659d27d Compare November 21, 2023 21:39
@adamw adamw merged commit 0518457 into softwaremill:master Nov 22, 2023
12 checks passed
@adamw
Copy link
Member

adamw commented Nov 22, 2023

Thanks!

@cornerman cornerman deleted the openapi-303-nullability branch November 22, 2023 21:29
@cornerman
Copy link
Contributor Author

Would you mind making a new release soonish? I am in dire need for this patch :)

@adamw
Copy link
Member

adamw commented Nov 23, 2023

Ah yes sorry, on its way :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants