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

Directly generate ToSchema instance from .proto AST #63

Merged
merged 2 commits into from
Apr 20, 2018

Conversation

Gabriella439
Copy link
Contributor

The motivation for this change is to fix the mismatch between the Swagger API
declared in ToSchema instances and the actual API served by the FromJSONPB
and ToJSONPB instances.

The difference between the two was in casing conventions: the JSONPB API was
preserving the original case of the fields from the .proto API, whereas the
Swagger API was camel-casing the declared fields.

The reason they differ was that they were going through two separate code
pathways. In the case of JSONPB the FromJSONPB and ToJSONPB instances were
being generated directly from the .proto AST without any intermediate steps:

`.proto` AST →(code generation)→ `FromJSONPB`

`.proto` AST →(code generation)→ `ToJSONPB`

... and that code generation step preserved the original case.

However, the ToSchema instance was generated indirectly via GHC generics:

`.proto` AST →(code generation)→ Haskell data type →(Generics)→ `ToSchema`

... and the code generation step for the Haskell data type was not preserving
case.

After this change the code generation directly generates the ToSchema
instance:

`.proto` AST →(code generation)→ `ToSchema`

Some alternative approaches that I considered and rejected:

  • Change the Haskell data type generation to preserve the original case instead
    of camel-casing Haskell field names

    I rejected this approach because it is highly disruptive to downstream code
    and also generates ugly field names

  • Customize the ToSchema instances derived via Generics to convert camel
    case to snake case

    I rejected this approach because it is not correct if the original .proto
    field names were already camel-cased. This would then lead to the opposite
    problem: declaring a snake-cased field which was actually camel-cased

I tested this downstream on our large internal .proto files. The only
difference in the generated Swagger is that the casing for declared fields is
fixed and the generated Haskell code still compiles without any changes to the
client packages.

I also added some simple golden tests for simple visual inspection of what the
generated Swagger looks like.

Gabriel Gonzalez added 2 commits April 20, 2018 11:04
The motivation for this change is to fix the mismatch between the Swagger API
declared in `ToSchema` instances and the actual API served by the `FromJSONPB`
and `ToJSONPB` instances.

The difference between the two was in casing conventions: the JSONPB API was
preserving the original case of the fields from the `.proto` API, whereas the
Swagger API was camel-casing the declared fields.

The reason they differ was that they were going through two separate code
pathways.  In the case of JSONPB the `FromJSONPB` and `ToJSONPB` instances were
being generated directly from the `.proto` AST without any intermediate steps:

    `.proto` AST →(code generation)→ `FromJSONPB`

    `.proto` AST →(code generation)→ `ToJSONPB`

... and that code generation step preserved the original case.

However, the `ToSchema` instance was generated indirectly via GHC generics:

    `.proto` AST →(code generation)→ Haskell data type →(Generics)→ `ToSchema`

... and the code generation step for the Haskell data type was *not* preserving
case.

After this change the code generation directly generates the `ToSchema`
instance:

    `.proto` AST →(code generation)→ `ToSchema`

Some alternative approaches that I considered and rejected:

* Change the Haskell data type generation to preserve the original case instead
  of camel-casing Haskell field names

  I rejected this approach because it is highly disruptive to downstream code
  and also generates ugly field names

* Customize the `ToSchema` instances derived via `Generic`s to convert camel
  case to snake case

  I rejected this approach because it is not correct if the original `.proto`
  field names were already camel-cased.  This would then lead to the opposite
  problem: declaring a snake-cased field which was actually camel-cased

I tested this downstream on our large internal `.proto` files.  The only
difference in the generated Swagger is that the casing for declared fields is
fixed and the generated Haskell code still compiles without any changes to the
client packages.

I also added some simple golden tests for simple visual inspection of what the
generated Swagger looks like.
It should be `OVERLAPPING` instead of `OVERLAPPABLE`.  Apparently GHC doesn't
care because the old version still worked, but better to be safe than sorry.
Copy link
Collaborator

@intractable intractable left a comment

Choose a reason for hiding this comment

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

I didn't do deep review, but this LGTM and it's already been tested. Thank you so much for doing this!

@Gabriella439 Gabriella439 merged commit c103a8c into master Apr 20, 2018
@Gabriella439 Gabriella439 deleted the gabriel/direct_toSchema_2 branch April 20, 2018 19:10
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