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

[BUG] [JAVA] regressions introduced by #17622 (lombok) #17793

Open
5 of 6 tasks
jpfinne opened this issue Feb 5, 2024 · 3 comments
Open
5 of 6 tasks

[BUG] [JAVA] regressions introduced by #17622 (lombok) #17793

jpfinne opened this issue Feb 5, 2024 · 3 comments

Comments

@jpfinne
Copy link
Contributor

jpfinne commented Feb 5, 2024

Bug Report Checklist

  • Have you provided a full/minimal spec to reproduce the issue?
  • Have you validated the input using an OpenAPI validator (example)?
  • Have you tested with the latest master to confirm the issue still exists?
  • Have you searched for related issues/PRs?
  • What's the actual output vs expected output?
  • [Optional] Sponsorship to speed up the bug fix or feature request (example)
Description

#17622 allows the use of lombok. It works for simple cases but I don't think it is production ready.

There are multiple regressions

  • contraints not generated
  • JsonProperty gone even if the property name is different than the field name
  • javadoc not generated
  • incorrect constructors and builders for inheritance
  • toString is different (minor)
  • x-setter-extra-annotation ignored ( x-extra-field-annotation works)
  • probably other missing features : Optional, @Valid...
openapi-generator version

7.3.0-SNAPHOT

OpenAPI declaration file content or url
openapi: 3.0.1
info:
  title: lombok issues
  description: lombok issues
  version: 1.0.0
paths:
  /testLombok:
    get:
      description: test
      responses:
        200:
          description: 'response'
          content:
            application/json:
              schema:
                $ref: '#/components/schemas/LombokData'
components:
  schemas:
    ParentData:
      type: object
      properties:
        type:
          type: string
      discriminator:
        propertyName: type
    LombokData:
      allOf:
        - $ref: '#/components/schemas/ParentData'
        - type: object
          properties:
            extra-data:
              description: 'This is an extra-data'
              type: string
              minLength: 10

Here the generated LombokData.java

@lombok.Data
@lombok.NoArgsConstructor
@lombok.AllArgsConstructor
@lombok.Builder
public class LombokData extends ParentData {
  private String extraData;
}

compared to the non lombok version:

  /**
   * This is an extra-data
   * @return extraData
  */
  @Size(min = 10) 
  @Schema(name = "extra-data", description = "This is an extra-data", requiredMode = Schema.RequiredMode.NOT_REQUIRED)
  @JsonProperty("extra-data")
  public String getExtraData() {
    return extraData;
  }
Generation Details

java -jar openapi-generator-cli.jar generate -g spring -i issue_lombok.yaml -p additionalModelTypeAnnotations='@lombok.Data;@lombok.NoArgsConstructor;@lombok.AllArgsConstructor;@lombok.Builder'

Steps to reproduce

Build with 7.3.0-SNAPSHOT with lombok additionalModelTypeAnnotations
java -jar openapi-generator-cli.jar generate -g spring -i issue_lombok.yaml -p additionalModelTypeAnnotations='@lombok.Data;@lombok.NoArgsConstructor;@lombok.AllArgsConstructor;@lombok.Builder'

Related issues/PRs

#17622

Suggest a fix

Fix the different pojo.mustache with

  • Constraints on the field

  • lombok superBuilder for inheritance

  • javadoc on the field

  • @JsonProperty on the field (using @Getter(onMethod=@__({@JsonProperty("extra-data")})))

General question: Do we really need a openapi generator that needs lombok to create java classes?
pojo.mustache is already complex.
Configuring lombok is also complex. How to generate for example @Builder(toBuilder=true)?

Lombok is nice when writing code. Here the openapi generator does the job.

The only valid reason I see is the lack of the builder pattern in the current pojo.mustache.
It can be introduced. The generated code can be equivalent or better than a lombok annotated class.

IMHO it will be easier to implements new features without the extra lombok handling (immutability, records, genericity, merging the different java generators...)

@dabdirb
Copy link
Contributor

dabdirb commented Feb 7, 2024

@jpfinne thanks for you comments.
Why we need lombok models, if we just run a fresh generated program, it just provide rest api, there is business logic here, if i implement the database entity by using lombok, i will found the rest api models are not consistent. adding the lombok support also provide a work around to current implementation, such as constructor issue #15796, #15827.
I think the bean validation constraint is mandatory, should be added.
jackson @JsonProperty should be a optional, only exist when the field name not same as property name.
SuperBuilder support is simple, just add additionalModelTypeAnnotations @lombok.experimental.SuperBuilder
java doc and x-setter-extra-annotation, if we make the model support both lombok and all legacy features, the implementation will too complex to maintain, especially mustache template file.

@jpfinne
Copy link
Contributor Author

jpfinne commented Feb 7, 2024

So we agree that the generated java code should behave the same as some hand written pojo annotated with lombok.
ie: all arguments constructor + builder pattern.

IMHO it is better to add these 2 features in mustache code.

@EladShechter
Copy link

The story of constraints that are not generated when using data is disturbing. I hope it will be fixed as soon as possible

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants