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

Update swagger-parser to fix definition parse bug #41

Merged
merged 3 commits into from
Apr 5, 2018

Conversation

trane
Copy link

@trane trane commented Apr 2, 2018

While parsing swagger with additionalProperties, not all definitions are
resolved in swagger-parser < 1.0.34.
(swagger-api/swagger-parser#602)
This brings us to the most recent version of swagger-parser on the 1.x
line.

Fixes: #40

@trane
Copy link
Author

trane commented Apr 2, 2018

Interesting new NPE introduced from this change. Investigating.

@trane
Copy link
Author

trane commented Apr 2, 2018

Looks like we aren't handling UntypedProperty within SwaggerUtil.scala#propMeta, which is being triggered by PropertyExtractorsTest

# snip
property:
  default: "what"

According to the Swagger spec (2.0) definitions (https://github.com/OAI/OpenAPI-Specification/blob/master/versions/2.0.md#definitionsObject) can have null type via the JSON schema spec: https://tools.ietf.org/html/draft-zyp-json-schema-04#section-3.5

What is the most reasonable thing to do here?

I can think of a few things:

  1. Fail when type isn't present.
  2. Set the type to some null or nullable-like value (not sure how we represent this in scala.meta
  3. Ignore it
  4. Recursively inspect the default value (which is itself required to be a JSON schema object) and use the resulting resolved type to be the type we use (though we still run into this nullable situation)

@trane
Copy link
Author

trane commented Apr 2, 2018

For now, I'm failing property definitions that are untyped.

@blast-hardcheese
Copy link
Member

@trane Thanks for the contribution and investigation! Everything looks good, save for the merge conflict -- would you please resolve this at your convenience?

@blast-hardcheese
Copy link
Member

@trane One last time, my apologies

@trane
Copy link
Author

trane commented Apr 2, 2018

@blast-hardcheese, I'm concerned about some of the behavior I'm seeing now with this code. Namely, I'm getting unhelpful errors now like:

Error: Unable to resolve type for null (null null null null)

Where before I would get

Error: No responses defined for getActivities

@blast-hardcheese
Copy link
Member

@trane So, addressing #41 (comment):

As for HTTP, a "null" value doesn't really make sense. ?Foo= is still foo="". JSON, however, could represent literal null values. For this reason, defaulting the type-less properties to None.type might make sense, but then what does it mean to have a None.type be required. This would be a great opportunity for static parameters, where an undefined parameter always gets encoded as a None and can't be overwritten.

@trane
Copy link
Author

trane commented Apr 3, 2018

This would be a great opportunity for static parameters, where an undefined parameter always gets encoded as a None and can't be overwritten.

@blast-hardcheese I'm not sure I follow what you mean by "static parameters". Are you saying that we generate a class constructor which has the None.type singleton type instead of an Option[???]?

case class MyParam(foo: String, bar: None.type)

@trane
Copy link
Author

trane commented Apr 3, 2018

I did a bit more digging into what swagger-codegen did with the addition of UntypedProperty

For all the Scala code generators, they either returned Any (Akka, Scala, etc) or Object (finch). Though, how could we create an encoder/decoder for such an arbitrary type in any meaningful way?
It appears in the case of additionalProperties and schema etc, that these all just have to be some valid json, nothing more.

Perhaps the right answer here is to resolve as a io.circe.Json?

@blast-hardcheese
Copy link
Member

Exposing it as the underlying json library type makes sense. This would also be a stopgap for #43, although not a great one.

While parsing swagger with additionalProperties, not all definitions are
resolved in swagger-parser < 1.0.34.
(swagger-api/swagger-parser#602)
This brings us to the most recent version of swagger-parser on the 1.x
line.

Fixes: guardrail-dev#40
According to the Swagger spec (2.0) definitions
(https://github.com/OAI/OpenAPI-Specification/blob/master/versions/2.0.md#definitionsObject)
can have null type via the JSON schema spec:
https://tools.ietf.org/html/draft-zyp-json-schema-04#section-3.5

We don't currently have a mechanism to handle this case, so I am
returning an error for now.
@trane trane force-pushed the fix-definition-parsing branch from 100655e to 9dd3097 Compare April 4, 2018 23:21
@trane
Copy link
Author

trane commented Apr 4, 2018

Alright, I have resolved the issue with the introduction of UntypedProperty from the new swagger-parser in the best way I can think of for now. Should be good to merge if you feel good about it, too, @blast-hardcheese

@@ -295,6 +295,8 @@ object SwaggerUtil {
Target.pure(Resolved(t"Double", None, Default(d).extract[Double].map(Lit.Double(_))))
case d: DecimalProperty =>
Target.pure(Resolved(t"BigDecimal", None, None))
case u: UntypedProperty =>
Target.pure(Resolved(t"io.circe.Json", None, None)) // TODO: o.getProperties
Copy link
Member

Choose a reason for hiding this comment

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

Does getProperties exist on UntypedProperty? UntypedProperty.java suggests it doesn't (which would also make sense, as there's no way for it to know)

Copy link
Member

Choose a reason for hiding this comment

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

Additionally, something to note here is that SwaggerUtil needs to eventually be collapsed down into the different ProtocolGenerators, otherwise io.circe.Json won't be valid for other JSON libraries (json4s, support for Java, etc). Not a concern right now, but this is something that'll need to happen for #36

Copy link
Author

Choose a reason for hiding this comment

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

yeah, I figured that the io.circe.Json was good for now given that the protocol generator stuff isn't parameterized yet. I hope it's as simple as a find/replace of io.circe.Json to ${correctProtocolGenerator} when that happens.

swagger-codegen added UntypedProperty after version 1.0.32 which we
didn't handle and this caused issues with code generation no longer
properly reporting errors, just having a generic:

Error: Unable to resolve type for null (null null null null)

I took a look at the code that was added and what other implementations
did: https://github.com/swagger-api/swagger-core#2507

For all the Scala code generators, they either returned Any (Akka, Scala, etc)
or Object (finch). Though, how could we create an encoder/decoder for such an
arbitrary type in any meaningful way?

It appears in the case of additionalProperties and any other json schema
proprty, all that is required is valid json. Since there is no actual
type that we can infer from an untyped it makes sense to return the most
specific type we can: Json. In this case, we are returning io.circe.Json
@trane trane force-pushed the fix-definition-parsing branch from 9dd3097 to d5629ee Compare April 5, 2018 00:28
@blast-hardcheese
Copy link
Member

This looks good, thanks!

@blast-hardcheese blast-hardcheese merged commit 943a40c into guardrail-dev:master Apr 5, 2018
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.

Not all definitions end up in Swagger.definitions
3 participants