-
Notifications
You must be signed in to change notification settings - Fork 134
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
Java language support redux #192
Conversation
c9507d8
to
3e1b5aa
Compare
modules/codegen/src/main/scala/com/twilio/guardrail/Common.scala
Outdated
Show resolved
Hide resolved
modules/codegen/src/main/scala/com/twilio/guardrail/ProtocolGenerator.scala
Show resolved
Hide resolved
...gen/src/main/scala/com/twilio/guardrail/generators/Java/AsyncHttpClientClientGenerator.scala
Outdated
Show resolved
Hide resolved
...gen/src/main/scala/com/twilio/guardrail/generators/Java/AsyncHttpClientClientGenerator.scala
Outdated
Show resolved
Hide resolved
.../codegen/src/main/scala/com/twilio/guardrail/generators/Java/DropwizardServerGenerator.scala
Outdated
Show resolved
Hide resolved
modules/codegen/src/main/scala/com/twilio/guardrail/generators/Java/JacksonGenerator.scala
Outdated
Show resolved
Hide resolved
modules/codegen/src/main/scala/com/twilio/guardrail/generators/Java/JacksonGenerator.scala
Outdated
Show resolved
Hide resolved
modules/codegen/src/main/scala/com/twilio/guardrail/generators/Java/JacksonGenerator.scala
Outdated
Show resolved
Hide resolved
}): _* | ||
))) | ||
|
||
// TODO: leave out with${name}() if readOnlyKey? |
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.
"read only" means different things from the server side and client side. I haven't come up with a good way to represent this yet. We should want either use case to work correctly, and getting the response from a server and giving it to a client should work without resetting the read-only values to null.
feaa662
to
a4f6a0d
Compare
803e4f6
to
6564b6f
Compare
fc8498a
to
a7360c4
Compare
modules/codegen/src/main/scala/com/twilio/guardrail/generators/Java/AkkaHttp.scala
Outdated
Show resolved
Hide resolved
) | ||
|
||
def doShow(tpe: Type): Expression = tpe match { | ||
case cls: ClassOrInterfaceType if cls.isOptional || cls.isNamed("List") => |
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.
x-scala-type: com.example.List
would also trigger this as well, right? Is the full classpath known at this point?
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.
It would, unfortunately. Should try to qualify it I guess?
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.
Yeah -- an additional question is that we don't support showables for implementers of an interface, preventing java.util.List
from actually being registered as showable, fwict
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.
There's nothing to prevent anyone from registering any concrete (or non-concrete) class as a Showable
, but yeah, List
is handled in a special way with behavior a user couldn't add. Unfortunately javaparser has no idea what a List is (or, better, what an Iterable is), so I'm not sure what to do here.
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 means any parameterized type will not be able to integrate, right? If Showable<A>
had both String show(A o)
and Boolean canShow(A o)
, I could
shower.register(classOf[List[_]], new Showable[List[_]] {
def canShow(l: List[_]): Boolean = l.headOption.fold(true)(o => shower.canShow(o.getClass))
def show(l: List[_]): String = s"List(${l.map(shower.show(_)).mkString(", ")})"
)
similar to how unapply
works
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.
But that doesn't actually do anything useful here, at least for the specific case of a list of something. If you look at the generated code for AsyncHttpClient, the code that gets emitted in the special case of List
is (roughly):
for (final ${containedType} item : list) {
builder.addFormParam("${paramName}", Shower.show(item));
}
Being able to take a list and generate a string that concats the values with commas in between isn't useful.
Parameterized types that aren't iterable should work, assuming the contained type also has a Showable
instance registered. But again, the string that results has to actually be useful as a value. So you could perhaps do somthing like:
shower.register(classOf[Option[_]], new Showable[Option[_]] {
def show(o: Option[_]): String = o.map(shower.show).orNull
}
That would actually work properly for the specific case of adding a form or query param for the AHC client generator, I think.
Back to list types: what we really need is a way to identify if a (custom) type is iterable or not, and then handle that iterable in whatever way makes sense for that purpose (add each element as a form or query param, or whatever). I'm not really sure how to do that, though, since JavaParser, when given an arbitrary string for a type, can't tell if the type implements Iterable
. Perhaps if we know the OpenAPI type is array
, then we just assume that if something is passed via x-scala-type
for it, it must be Iterable
? Not sure...
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.
At least if that assumption is wrong, it would be a discoverable type error. I'm thinking of attempts at refinement types, like NonEmptyList
registered with Jackson such that more requirements can be expressed in the protocol, leading way to the OpenAPI validation stuff
...gen/src/main/scala/com/twilio/guardrail/generators/Java/AsyncHttpClientClientGenerator.scala
Outdated
Show resolved
Hide resolved
modules/codegen/src/main/scala/com/twilio/guardrail/generators/JavaGenerator.scala
Outdated
Show resolved
Hide resolved
modules/codegen/src/main/scala/com/twilio/guardrail/generators/syntax/Java.scala
Outdated
Show resolved
Hide resolved
modules/codegen/src/main/scala/com/twilio/guardrail/generators/JavaGenerator.scala
Show resolved
Hide resolved
modules/sample-dropwizard/src/test/scala/core/Dropwizard/DropwizardRoundTripTest.scala
Outdated
Show resolved
Hide resolved
} | ||
}) | ||
|
||
val httpClient: Request => CompletionStage[Response] = { request => |
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 kind of test harness looks useful; is this what you were talking about with generating test classes as well?
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.
Nah, the test class generation was to ensure that any custom types had a Showable
implementation, but there was a chicken/egg problem there, and I haven't thought about it more to see if there's anything we can do.
2b066a0
to
67cc572
Compare
modules/codegen/src/main/scala/com/twilio/guardrail/SwaggerUtil.scala
Outdated
Show resolved
Hide resolved
c25c9f0
to
4d1b12a
Compare
Picks the 'best' content-type to use, and also handles the difference between urlencoded and form-data correctly. Should probably handle all possible combinations of consumes+produces and emit multiple resource methods, but this will do for now.
It's not broken with empty paths, but Jersey warns about it on startup.
40b499e
to
fb393c7
Compare
Based on & supersedes #176
Not ready for merge, but here's what we've got so far. I was planning to break this up into just Java language support and then client/framework support, but I've found that, because of my lack of familiarity with things, the only way I've been able to make sure everything works and also figure out what things need to be refactored and added has been to actually implement full protocol object and client support. So...
JavaLanguage
abstractionJavaInterp
EnumProtocolTermInterp
ModelProtocolTermInterp
ArrayProtocolTermInterp
ProtocolSupportTermInterp
PolyProtocolTermInterp
ClientTermInterp
forAsyncHttpClient
(nearly done; only missing query/form/body param support)ServerTermInterp
for DropwizardFrameworkInterp
for AHC/DWCodegenApplication
for DWAfter I'm finished I can take a stab out of separating this into 3 PRs if you think it's worth it: 1) structural changes/refactoring, 2) base Java language support, and 3) DW server/AHC client/framework support.
(Note that I plan to force-push to this branch quite a bit, so don't base anything on it just yet.)