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

feat(#389): allow templating for generated Java / Kotlin class names #535

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

BlasiusSecundus
Copy link

Adds an optional nameTemplate input parameter to the plugin, allowing customization of the generated Java / Kotlin class names.

The following template variables are supported:

  • name - the original name of the class
  • schemaType - the GraphQL schema type (Type, Input, Interface, Enum)

The default value for this new property is null. In this case the output will be identical to the current one.

Examples:

Given an original class name Person and schema type Type:

  • null -> Person
  • "{name}GraphQL{schemaType}" -> PersonGraphQLType
  • "{name}GraphQL" -> PersonGraphQL
  • "{name}{schemaType}" -> PersonType

@congotej
Copy link

Hi @BlasiusSecundus! Thank you for opening this PR, I am actively looking into this. In the meantime, could you please resolve the conflicts?

@BlasiusSecundus BlasiusSecundus force-pushed the feature/389-cusomt-generated-class-name-templates branch from 9bb7f71 to 2cb13d9 Compare May 26, 2023 06:13
@BlasiusSecundus
Copy link
Author

I have resolved conflicts. However, when running the tests locally I saw KotlinCodeGenTest failing with errors like this one:

error: unresolved reference: fasterxml
import com.fasterxml.jackson.`annotation`.JsonProperty
           ^
error: cannot access built-in declaration 'kotlin.String'. Ensure that you have a dependency on the Kotlin standard library
import kotlin.String

I have not noticed this before, and this also happens on master branch, so I think this should not be related to my PR.

@BlasiusSecundus BlasiusSecundus force-pushed the feature/389-cusomt-generated-class-name-templates branch from 938bf05 to 7aa0855 Compare June 20, 2023 04:38
@mfjBiz
Copy link

mfjBiz commented Sep 27, 2023

@BlasiusSecundus
Hello, thank you for your work on this PR. We find it to be incredibly valuable for our use case.

Regarding the KotlinCodeGenTest failure you mentioned, I've checked out the latest master branch and it seems that the issue may have been resolved there.

@BlasiusSecundus BlasiusSecundus force-pushed the feature/389-cusomt-generated-class-name-templates branch 2 times, most recently from 49c094c to 4e88294 Compare September 27, 2023 17:45
@BlasiusSecundus
Copy link
Author

I rebased and resolved the conflicts. The aforementioned error still occurs on my machine (asl on master), but the CI build job ran successfully (on my forked repo at least). So maybe this is something related to my local setup only, I'll check.

@mfjBiz
Copy link

mfjBiz commented Sep 28, 2023

@BlasiusSecundus
Thank you for taking the time to rebase and resolve the conflicts. It's good to hear that the CI build job ran successfully on your forked repo.

@congotej
I noticed your previous comment about actively looking into this PR. Could I kindly ask for an update on the current status of this PR?
We're really looking forward to benefiting from this feature. Thank you!

@BlasiusSecundus BlasiusSecundus force-pushed the feature/389-cusomt-generated-class-name-templates branch 2 times, most recently from 8cf71ae to 8d2dc23 Compare January 21, 2024 16:00
@BlasiusSecundus
Copy link
Author

Btw. This PR should also address issue #323

@BlasiusSecundus BlasiusSecundus force-pushed the feature/389-cusomt-generated-class-name-templates branch 2 times, most recently from 0f12a5f to 4f6277c Compare August 22, 2024 20:09
… names

Adds an optional `nameTemplate` input parameter to the plugin, allowing
customization of the generated Java / Kotlin class names.

The following template variables are supported:
- name - the original name of the class
- schemaType - the GraphQL schema type (Type, Input, Interface, Enum)

The default value for this new property is null. In this case the output
will be identical to the current one.

Examples:

Given an original class name `Person` and schema type `Type`:
 - null -> Person
 - "{name}GraphQL{schemaType}" -> PersonGraphQLType
 - "{name}GraphQL" -> PersonGraphQL
 - "{name}{schemaType}" -> PersonType
@SankarshanDudhate
Copy link

SankarshanDudhate commented Aug 23, 2024

@srinivasankavitha @congotej don't mean to rush you but can you please review this? If there's any changes expected, please let us know. We've been eagerly waiting for this one.

@srinivasankavitha
Copy link
Contributor

Hi - apologies for the delayed responses on this PR. Could you please elaborate on how this feature would be useful for the projects you are working with? Is it more to address code readability when working with generated types? The plugin itself is designed to do a few things well, and imo, over time we've added way too many customizations already that are making this hard to maintain and ensure correctness. I'm hesitant to add more feature flags unless they really make it worth the maintenance cost.

@BlasiusSecundus
Copy link
Author

Could you please elaborate on how this feature would be useful for the projects you are working with?

For example, you have a Person class in the domain layer, and a Person class in the (generated) API layer. Without such a feature, a naming conflict would result either in hard-to-read code (e.g. using fully qualified names), or would require manual workarounds (such as using imports alias or typealias in Kotlin) - neither results in an optimal developer experience.

Is it more to address code readability when working with generated types?

Essentially yes, It makes the code much easier to read. (Is that Person from the domain layer or the API layer? - a prefix/suffix communicates that immediately)

I'm hesitant to add more feature flags unless they really make it worth the maintenance cost.

This feature is meant to be an equivalent of OpenAPI Generator's modelNamePrefix / modelNameSuffix settings. I think this feature would be used widely enough to justify the maintenance costs - which should not be that huge considering this is a quite simple feature.

@SankarshanDudhate
Copy link

The plugin itself is designed to do a few things well, and imo, over time we've added way too many customizations already that are making this hard to maintain and ensure correctness. I'm hesitant to add more feature flags unless they really make it worth the maintenance cost.

I agree with your point about there being too many customizations. However, lack of this feature causes too much confusion and conflicts, as @BlasiusSecundus pointed out above. We resorted to adding manual prefixes to our graphql types to overcome the conflicts but that has led to much more confusion and chaos on the consumer side as this made our types from one particular repo out-of-sync with the naming convention that the rest of our types follow. This is especially painful given that some of these types are exposed to the rest of the world (via a graphql api for devs). Hope this feature will be available soon.

@SankarshanDudhate
Copy link

SankarshanDudhate commented Sep 10, 2024

@srinivasankavitha @congotej what do you think?

@srinivasankavitha
Copy link
Contributor

@paulbakker and I discussed this and we will merge this after some testing on our end. Thanks for the PR contribution!

@tarundube
Copy link

This Feature will be of great help to avoid conflicts and readability, waiting for this one!!!!!

@wick3ds0ul
Copy link

I agree with @SankarshanDudhate ! We are following Person class and PersonType for graphql to avoid conflicts. This would standardize the codegen. However, would love to see the possibility of having Person domain class and Person type in graphql with no conflicts.

@paulbakker
Copy link
Collaborator

I tested this with a random test schema, and get some compilation issues because the naming isn't applied everywhere, it looks like.

Example schema:

type Query {
    lolomo: [ShowCategory]
    search(searchFilter: SearchFilter!): [Show]

}

input SearchFilter {
    title: String!
}

type ShowCategory {
    id: Int
    name: String
    shows: [Show]
}

interface Show @key(fields: "showId"){
    showId: Int
    title: String
    artworkUrl: String
}

type Movie implements Show{
    showId: Int
    title: String
    artworkUrl: String
    director: String
}

type Series implements Show {
    showId: Int
    title: String
    artworkUrl: String
    episodes: Int
}

E.g. this generates


@JsonTypeInfo(
    use = JsonTypeInfo.Id.NAME,
    include = JsonTypeInfo.As.PROPERTY,
    property = "__typename"
)
@JsonSubTypes({
    @JsonSubTypes.Type(value = Movie.class, name = "Movie"),
    @JsonSubTypes.Type(value = Series.class, name = "Series")
})
public interface DgsInterfaceShowGenerated {

Naming I configured: nameTemplate = 'Dgs{schemaType}{name}Generated'

@BlasiusSecundus can you look into this?

@paulbakker paulbakker self-requested a review September 16, 2024 20:47
Copy link
Collaborator

@paulbakker paulbakker left a comment

Choose a reason for hiding this comment

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

Test and fix use of interfaces.

@BlasiusSecundus
Copy link
Author

@paulbakker Thank you for the review and testing! Ill fix the issue of course and will add a test case for this scenario.

At first glance the problem seems to be that for the Movie type it generated DgsTypeMovieGenerated (which is correct), but in the Jackson annotations they were referenced using their untemplated names (which obviously resulted in compilation errors).

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.

8 participants