-
Notifications
You must be signed in to change notification settings - Fork 132
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
issue #222 #234
issue #222 #234
Conversation
So, in this case, I'd add an explicit test, similar to package issues.issue222.server.http4s.definitions
import io.circe._
import io.circe.syntax._
import io.circe.generic.semiauto._
-case class Request(state: Option[BigInt] = None, id: Option[String] = None) extends RequestFields
+case class Request(id: Option[String] = None)
object Request {
implicit val encodeRequest = {
val readOnlyKeys = Set[String]()
- Encoder.forProduct2("state", "id") { (o: Request) => (o.state, o.id) }.mapJsonObject(_.filterKeys(key => !(readOnlyKeys contains key)))
+ Encoder.forProduct1("id") { (o: Request) => o.id }.mapJsonObject(_.filterKeys(key => !(readOnlyKeys contains key)))
}
- implicit val decodeRequest = Decoder.forProduct2("state", "id")(Request.apply _)
+ implicit val decodeRequest = Decoder.forProduct1("id")(Request.apply _)
} which I assume is not your goal. I presume your intent would be to preserve all the keys, just to get rid of the object inheritance, which your PR does partially, just needing a bit more massaging to get to where it needs to be. Some considerations:
Not all of these need to be solved right now, I'm very much in favor of accomplishing primarily what is necessary to unblock consumers, but without consideration, it's very easy to implement something that will not be forward source-compatible. Again, sorry for having blocked you; I'm interested in helping unblock this feature request, so feel free to ask any follow-up questions here. |
WOOOO! I can't believe it I made the tests pass 🎉 |
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.
💯 Thanks for your effort so far! Your strategy is good, this just needs some polishing up before merge.
modules/codegen/src/main/scala/com/twilio/guardrail/ProtocolGenerator.scala
Outdated
Show resolved
Hide resolved
modules/codegen/src/main/scala/com/twilio/guardrail/ProtocolGenerator.scala
Outdated
Show resolved
Hide resolved
modules/codegen/src/main/scala/com/twilio/guardrail/generators/CirceProtocolGenerator.scala
Outdated
Show resolved
Hide resolved
modules/codegen/src/main/scala/com/twilio/guardrail/generators/CirceProtocolGenerator.scala
Outdated
Show resolved
Hide resolved
modules/codegen/src/main/scala/com/twilio/guardrail/generators/Java/JacksonGenerator.scala
Outdated
Show resolved
Hide resolved
…/CirceProtocolGenerator.scala Co-Authored-By: tomasherman <tomas.herman@gmail.com>
modules/codegen/src/main/scala/com/twilio/guardrail/ProtocolGenerator.scala
Outdated
Show resolved
Hide resolved
I think i addressed all questions / comments for this PR ... are there any changes needed before this can be merged? |
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 seem to be some gaps in implementation, one that would necessitate an immediate hotfix, another that would silently fail based on the ordering of parameters in a list
Right ... i will try to fix the issues you found tomorrow ... thanks for
the heads up
…On Wed, May 1, 2019 at 12:32 AM Devon Stewart ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In modules/codegen/src/test/scala/core/issues/Issue222.scala
<#234 (comment)>:
> +
+
+ compare(request.tpe, expectedRequestTpe)
+ compare(request.cls, expectedRequestCls)
+ compare(reqEncoder, expectedRequestEncoder)
+ compare(reqDecoder, expectedRequestDecoder)
+
+ val expectedFieldsTpe = t"""RequestFields"""
+ val expectedFieldsCls = q"""case class RequestFields(state: Option[BigInt] = None)"""
+
+ val List(fieldsEncoder, fieldsDecoder) = requestFields.staticDefns.definitions
+
+ val expectedFieldsEncoder = q"""
+ implicit val encodeRequestFields = {
+ val readOnlyKeys = Set[String]()
+ Encoder.forProduct1("state")((o: RequestFields) => o.state).mapJsonObject(_.filterKeys(key => !(readOnlyKeys contains key)))
Ah, my apologies -- I encountered the error I mentioned below, and I was
hasty in looking for an example of the undesired behaviour in what you
already had here.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#234 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAAYMHUQ3YCUXDUYTFN55ULPTDCIXANCNFSM4HFQVRAA>
.
|
ok so this one was a doozy ... anyways, i added two more test cases, hopefully fixing your concerns ... I fixed it but im not really sure why the original logic was the way it was (basically taking just last object from composed schema ... perheps bug and not intentional? I also added some error handling - guardrail will now fail with error message in case some composed object references definition that does not exist (i wasted like 30 minutes debugging this case when fixing one of those tests i added so i added this validation/error reporting) Let me know what you think! |
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 is wildly cool. Very well done. Thank you!
modules/codegen/src/main/scala/com/twilio/guardrail/generators/CirceProtocolGenerator.scala
Outdated
Show resolved
Hide resolved
…/CirceProtocolGenerator.scala Co-Authored-By: tomasherman <tomas.herman@gmail.com>
@kelnos Brain, i know this is not your issue but the changes you merged yesterday broke this PR... You seem like the guy that's mostly working on jackson stuff, how do you feel issue #222 should be compiled for Java? Basically it's this hierarchy: https://github.com/twilio/guardrail/blob/20ad1f605006e24f2a5a42c075e672b6b81408cf/modules/sample/src/main/resources/issues/issue222.yaml In scala, it's implemented such that if there is no discriminator, only the fields are copied without any hierarchy being generated...do you think it's ok to do the same in java? |
@tomasherman I know OO is somewhat controversial, but my opinion here is to implement these as class hierarchies and not flatten them out, for Java at least. A lack of discriminator just means that the spec author doesn't need polymorphic deserialization, but that doesn't say anything about how the user wants to use the generated classes. Perhaps they've created a hierarchy intentionally and intend to exploit use of the base class so they can operate on all the classes generically? I'd rather not take that possibility away. For people who don't care, the interface to create and interact with the objects is the same either way, so there's no loss to usability by maintaining the hierarchy. I think this is less an issue in Scala for some hand-wavy ill-defined reasons around programming philosophy, but I'm pretty happy with the behavior I've arrive at with Java/Jackson. |
Fair enough, however the generated code for the scenario i submitted above won't compile The problem is that both Request and RequestFields have static method builder() ... and in Request the types don't match (you can see the output of travis job) ... do you have an opinion how to solve this? |
@tomasherman sorry, just a sec -- speaking offline with @blast-hardcheese & realizing I was missing context around why your change is being made. Agree that we need to flatten things out in the case where the swagger spec specifies multiple inheritance. Unfortunately it's not super simple. Reverting my Jackson PR probably won't help, as the old code was garbage at dealing with polymorphism, so I'd expect your new test cases to fail with that code as well. I'm going to attempt to get the Jackson code generating flat objects for the mutiple-inheritance case over the next couple hours. If I can get it done, we'll merge that PR plus yours, and then cut a release. Otherwise we'll cut a release with what's in master now, and I'll continue working so we can get these changes out in the next release. Really sorry that I hadn't kept tabs on your work well enough to realize we'd be affecting each other and should have been coordinating. |
Ok, above PR referenced should make Jackson behave properly. I'm going to try to merge our branches together to see if we actually end up with something that works for everything. |
So my test spec doesn't actually build for scala with your branch merged in. Not sure if it's worth fixing, but for reference: swagger: 2.0
info:
version: 1.0.0
paths: {}
definitions:
A:
type: object
properties:
a:
type: string
B:
allOf:
- $ref: "#/definitions/A"
- type: object
properties:
b:
type: string
C:
type: object
properties:
c:
type: string
D:
type: object
properties:
d:
type: string
E:
allOf:
- $ref: "#/definitions/C"
- $ref: "#/definitions/D"
- type: object
properties:
e:
type: string
F:
type: object
discriminator: f
required:
- f
properties:
f:
type: string
G:
type: object
properties:
g:
type: string
H:
allOf:
- $ref: "#/definitions/F"
- $ref: "#/definitions/G"
- type: object
properties:
h:
type: string And the errors I'm getting:
|
Thanks for the effort! I think it's definitely worth fixing ... i will take look at this over the weekend and try to implement this properly, but if im not able to, can we agree to merge what i've done here? It's strictly better than what is curently implemented in guardrail and it will allow us (avast) to use it in our usecases and we can contribute further improvements along in the future |
Merging this now, further bugfixes for #234 (comment) can be in future releases. Better to have the new functionality and iterate on what we have for now. Again, thank you for your diligent work on this PR! |
Sweet! Thank you guys! Or next contribution will probably be attempt at
validations
…On Fri, 3 May 2019, 21:29 Devon Stewart, ***@***.***> wrote:
Merged #234 <#234> into master.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#234 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAAYMHRUGNRWXKHZ3WZOMRLPTSG7ZANCNFSM4HFQVRAA>
.
|
attempt at #222