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

How can I extend all models with x-nullable extensions if the type is Option? #69

Open
jenol opened this issue Mar 14, 2018 · 15 comments
Open

Comments

@jenol
Copy link

jenol commented Mar 14, 2018

I am using this tool on a large Akka HTTP project and I am very pleased with it. Keep up the good work!

I need a bit of help with one issue though. I would like to enrich all model types with an x-nullable vendor extension for property types of Option. This seems to be the most common workaround to support nullable types in Swagger 2.

I cannot decorate the models directly with annotations (because they are generated by protoc) and I am looking for a way to enrich the model types with nullable hints.

I see that there is a io.swagger.jaxrs.config.ReaderListener I can use to change the swagger document but I am not sure where I could register it and how to add the extension values on Option type. If you give me some direction, I could create a PR for this if code changes are needed.

Thank you for your help in advance.

@jenol
Copy link
Author

jenol commented Mar 14, 2018

I figured it out. I need to add the listener to the apiTypes.

@pjfanning
Copy link
Collaborator

@jenol if you could do a small sample demonstrating your solution on your github or bitbucket account, I can link to it from the README page.
If the solution needs a change in swagger-akka-http, could you provide a Pull Request?

@jenol
Copy link
Author

jenol commented Mar 15, 2018

@pjfanning this is how far I got. I still need a way to recognize the original property type. I can add the x-nullable based on that.

io.swagger.models.properties.Property doesn't seem to have a reference to the original type or original type name which would make things easier. I need to check if I can use required or format or a combination of properties to figure out if the type meant to be nullable. For this I will need to look deeper into the reader implementation.

https://gist.github.com/jenol/975ea072e60a893efedb9d51643ac812

@pjfanning
Copy link
Collaborator

Options should have property.isRequired() == false

@jenol
Copy link
Author

jenol commented Mar 15, 2018

It looks like getRequired is true for Option property types. I will keep digging

@pjfanning
Copy link
Collaborator

https://github.com/swagger-akka-http/swagger-akka-http/blob/master/src/main/scala/com/github/swagger/akka/SwaggerScalaModelConverter.scala has the support scala.Option - maybe you could add an option test case that demonstrates an option that isn't appearing as required=false (see the src/test dir for existing unit tests like this)

@jenol
Copy link
Author

jenol commented Mar 16, 2018

Good suggestion. I will do that. Thank you

@jenol
Copy link
Author

jenol commented Mar 16, 2018

I did some testing and getRequired seems to work correctly but the type is not correct. I updated the gist.

class Jeno(val test: Option[Int],
		val test2: Option[Boolean],
		val test3: Option[BigDecimal],
		val test4: Option[BigInt],
		val test5: Option[String])

Should generate

"Jeno" : {
  "type" : "object",
  "properties" : {
	"test" : {
	  "type" : "integer",
	  "format" : "int32",
	  "x-nullable" : true
	},
	"test2" : {
	  "type" : "boolean",
	  "x-nullable" : true
	},
	
	....
	
	"test5" : {
	  "type" : "string",
	  "x-nullable" : true
	}
  }
}

but generates

"Jeno" : {
	"type" : "object",
		"properties" : {
		"test" : {
		  "type" : "object",
		  "x-nullable" : true
		},
		"test2" : {
		  "type" : "object",
		  "x-nullable" : true
		},
		"test3" : {
		  "$ref" : "#/definitions/BigDecimal",
		  "x-nullable" : true
		},
		"test4" : {
		  "$ref" : "#/definitions/BigInt",
		  "x-nullable" : true
		},
		"test5" : {
		  "type" : "string",
		  "x-nullable" : true
		}
	}
}
"BigDecimal" : {...},
"BigInt" : {...}

@jenol
Copy link
Author

jenol commented Mar 16, 2018

@pjfanning
Copy link
Collaborator

pjfanning commented Mar 16, 2018

@jenol your code does not compile and it looks like a lot of unit tests will be affected by the change
Could you try to add new unit tests for your change and test with sbt clean test? If you get a working change, could you create a Pull Request?

@jenol
Copy link
Author

jenol commented Mar 19, 2018

Sorry about that. I am having issues with setting up the project on Windows and IntelliJ. I should have sent a gist. I just wanted to show the part of the code which may not handle all option types correctly. I will keep at it when I have a bit of time. :)

@pjfanning
Copy link
Collaborator

The code for inferring swagger models from Java/Scala classes is based on Java reflection and jackson-databind. It appears that for Option[T] where T is a primitive type like Int/Long/Double that this type info is erased.
One workaround would for you to subclass some of the classes that are causing you problems and to use swagger annotations on the subclasses to correct any issues in the models.

@ddfic
Copy link

ddfic commented Mar 29, 2018

hi @pjfanning not sure if I understand...
if Option[Int] is causing problems for us you suggest subclassing Option? sound pretty bad ;)

@pjfanning
Copy link
Collaborator

@ddfic I meant subclassing the classes generated by protoc

@ElfoLiNk
Copy link

ElfoLiNk commented Apr 5, 2018

Hello, i saw this issue after we upgraded to 0.13.0 Option[Int] stopped working correctly it is generated as object in the swagger json. The latest version were Option[Int] was converted correctly is 0.12.0. I created #72

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

No branches or pull requests

4 participants