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

Allow custom class names for generated clients and server resources/handlers #314

Open
kelnos opened this issue May 28, 2019 · 4 comments
Open
Labels
core Pertains to guardrail-core enhancement Functionality that has never existed in guardrail good first issue Does not require deep knowledge of the codebase and is easily testable help wanted Easy to moderately difficult issues that don't require deep knowledge or architectural thought

Comments

@kelnos
Copy link
Member

kelnos commented May 28, 2019

Currently guardrail uses the last component of the custom package name (as specified using x-{jvm,scala,java}-package or tags) as a prefix for the generated class name. It would be useful to allow people to name the class name something arbitrary.

@blast-hardcheese blast-hardcheese added core Pertains to guardrail-core enhancement Functionality that has never existed in guardrail good first issue Does not require deep knowledge of the codebase and is easily testable help wanted Easy to moderately difficult issues that don't require deep knowledge or architectural thought labels Jun 5, 2019
@zezutom
Copy link

zezutom commented Jul 4, 2020

Addressed in #719

@kwhitehouse
Copy link

Hi there,

Problem statement: We're using Guardrail to generate client code for a large API surface. We'd like to give the generated client a more explicit name than the auto-generated Client.scala.

Available options: It looks like currently there are two ways to do this:

  1. Add tagsBehaviour = Context.PackageFromTags to the ScalaClient stanza.
    Compile / guardrailTasks := List(
        ScalaClient(
          file("some.openapi.yaml"),
          pkg = "com.some.package",
          tagsBehaviour = Context.PackageFromTags
        )

This of course will only have an effect if the openapi spec uses tags: to annotate paths:. For any defined tags, it appears that both a new directory and a more explicitly named client will be generated. For instance:

  • Instead of generating some-package/target/more/paths/here/client/Client.scala
  • We might generate some-package/target/more/paths/here/client/SomeTag/SomeTagClient.scala and some-package/target/more/paths/here/client/AnotherTag/AnotherTagClient.scala

Cons of this approach are related to the fact that we are relying on whatever tags are specified within the openapi definition.

  • We're SOL if there aren't any tags in the openapi specification.
  • Tags are often not very descriptive, so we do get something different than just Client.scala, but it might not be much better.
  1. Add x-scala-class-prefix within the openapi yaml specification.
paths:
  /api/v1/mypath:
    get:
      x-scala-class-prefix: HelloWorld
      ...

This appears to generate multiple clients:

  • A Client.scala for all paths not annotated with x-scala-class-prefix
  • A HelloWorldClient.scala for all paths annotated with x-scala-class-prefix specified as HelloWorld.

Cons of this approach are:

  • We are required to edit the openapi specification. Ideally we want to be able to copy-paste the specification from a Swagger page and never edit this file.

Proposal:

We're hoping we could introduce a parameter overrideGeneratedClientClassName that can be specified via the ScalaClient stanza. It looks like this parameter value could be threaded through to ClientGenerator.scala#L52 and replace the hardcoded "Client" there. I think the nuances in adding this are mainly around how this new parameter might interact with the two currently available mechanisms I outlined above for updating generated client names. Options might be:

  • Throw an exception if a user tries to combine these different options.
  • Define some sort of precedence, so that if a user combines these mechanisms we default to using just one of them. It looks like this is currently the behavior when combining both x-scala-class-prefix and tagsBehaviour = Context.PackageFromTags: when I use both of these options, I generate a HelloWorldClient.scala and SomeTag/SomeTagClient.scala and AnotherTag/AnotherTagClient.scala.
  • Combine these different mechanisms. For instance, if a user specifies the new parameter overrideGeneratedClientClassName and x-scala-class-prefix and tagsBehaviour = Context.PackageFromTags, perhaps we'd generate the following classes:
    • SomeTag/SomeTagMyOverridenClientName.scala
    • AnotherTag/AnotherTagMyOverridenClientName.scala
    • MyOverridenClientName.scala

@kwhitehouse
Copy link

Hey @blast-hardcheese :) Thanks for reviewing my other open issue! I love the 👀 but also would be curious to hear more detailed thoughts. Do you see any issues with implementing this feature? I'd be happy to take a stab at it if it's something that you folks wouldn't be opposed to merging.

@blast-hardcheese
Copy link
Member

Hey @blast-hardcheese :) Thanks for reviewing my other open issue! I love the 👀 but also would be curious to hear more detailed thoughts. Do you see any issues with implementing this feature? I'd be happy to take a stab at it if it's something that you folks wouldn't be opposed to merging.

Meetings all morning for me, I didn't mean to leave this as just the 👀.

I'm sympathetic to the need here, I wouldn't mind adding this functionality, though probably just going with name instead of overrideGeneratedClientClassName. We would want this functionality for clients as well as servers.

Additionally, I'm wondering why the last component in pkg isn't picked up as the prefix for the class, that could solve the issue you're running into as well (although with the possibly hilarious ClientClient if you have pkg=com.example.foo.client).

I think at least for now, name makes quite a bit of sense, works for clients and servers, and doesn't break anyone who's not using this today.

I think this name only makes sense when not using tags as packages and not using x-scala-package/x-scala-class-prefix, as setting the value of the string literal "Client" that gets suffixed is kind of an abuse of the intent of the suffix to include the prefix as well as the suffix.

Does this match your expectations?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Pertains to guardrail-core enhancement Functionality that has never existed in guardrail good first issue Does not require deep knowledge of the codebase and is easily testable help wanted Easy to moderately difficult issues that don't require deep knowledge or architectural thought
Projects
None yet
Development

No branches or pull requests

4 participants