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

[java][client] fix: Add static modifier to inner class in Java when useSingleRequestParameter=true #20590

Merged
merged 4 commits into from
Feb 17, 2025

Conversation

cchacin
Copy link
Contributor

@cchacin cchacin commented Feb 4, 2025

When generating Java clients with the option useSingleRequestParameter set to true, there is a inner class generated that is not set as static.

Add a new value to be accepted for the useSingleRequestParameter:

  • true (existent)
  • false (existent)
  • static (new)

To generate an inner static class.

When useSingleRequestParameter=true:

public class DeletePetRequest {
    public class DeletePetRequest {
    ...
    }
...
}

When useSingleRequestParameter=static:

public class DeletePetRequest {
    public static class DeletePetRequest {
    ...
    }
...
}

@martin-mfg @Zomzog @lwlee2608

PR checklist

  • Read the contribution guidelines.
  • Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
  • Run the following to build the project and update samples:
    ./mvnw clean package || exit
    ./bin/generate-samples.sh ./bin/configs/*.yaml || exit
    ./bin/utils/export_docs_generators.sh || exit
    
    (For Windows users, please run the script in Git BASH)
    Commit all changed files.
    This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
    These must match the expectations made by your contribution.
    You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*.
    IMPORTANT: Do NOT purge/delete any folders/files (e.g. tests) when regenerating the samples as manually written tests may be removed.
  • File the PR against the correct branch: master (upcoming 7.x.0 minor release - breaking changes with fallbacks), 8.0.x (breaking changes without fallbacks)
  • If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

@cchacin cchacin changed the title fix: Add static modifier to inner class in Java when useSingleRequestParameter=true [java][client] fix: Add static modifier to inner class in Java when useSingleRequestParameter=true Feb 4, 2025
@cchacin cchacin force-pushed the fix-static-class branch 2 times, most recently from 77952f4 to f0016d1 Compare February 5, 2025 08:18
@wing328
Copy link
Member

wing328 commented Feb 5, 2025

cc @bbdouglas (2017/07) @sreeshas (2017/08) @jfiala (2017/08) @lukoyanov (2017/09) @cbornet (2017/09) @jeff9finger (2018/01) @karismann (2019/03) @Zomzog (2019/04) @lwlee2608 (2019/10) @martin-mfg (2023/08)

@Mattias-Sehlstedt
Copy link
Contributor

Mattias-Sehlstedt commented Feb 14, 2025

Hi sorry for intruding in the PR (I'll make an issue (and a PR for my suggestion) if you think this is misplaced), but I was just myself about to contribute something similar to this).

I had been looking into extending the functionality of the useSingleRequestParameter feature as well, where I for my personal project have made it so that the request model offers the same field-constructor approach that the general POJO model offers.

public static class DeletePetRequest {
    private String parameter1;
    private String parameter2;
    ...
 
    public DeletePetRequest() {}

    public DeletePetRequest(String parameter1, String parameter2, ...) {
        this.parameter1 = parameter1;
        ...
    }

    public DeletePetRequest parameter1(String parameter1) {
        this.parameter1 = parameter1;
    }

    public DeletePetRequest parameter2(String parameter2) {
        this.parameter2 = parameter2;
    }
    ...
}

This so that it is easy to only interact with the parameters that are relevant.

E.g., we generate an api-model for an api that has 5 parameters, for that I would with the current model need to state
new DeletePetRequest(param1, null, null, null, null).

Whereas with the POJO approach above we would only do
new DeletePetRequest().parameter1(param1).

This also makes it so that if the api adds new parameters then we are not forced to act on them (e.g., they add a sixth optional parameter, and I have to change the code from
new DeletePetRequest(param1, null, null, null, null) to new DeletePetRequest(param1, null, null, null, null, *null*).

Instead we do nothing with the POJO approach, and only if we want to sent that too then we change it to
new DeletePetRequest().parameter1(param1).*parameter6(param6)*.

Is this a usecase that makes sense? Or is there some other common way to realize a "null-empty", "parameter-resilient" approach? And if it is a suitable feature, how would one go about introducing it to restclient, where it seems that they have locked the implementation to using a record?

@martin-mfg
Copy link
Contributor

Hi @Mattias-Sehlstedt,

Is this a usecase that makes sense?

In my opinion, yes.
(Some people might argue that users should be forced to set all the parameters that are marked as "required" in the input OpenAPI specification, e.g. by only offering constructors which contain all the required parameters. But I would argue that this should either be enforced in a different way, or not enforced at all.)

Or is there some other common way to realize a "null-empty", "parameter-resilient" approach? And if it is a suitable feature, how would one go about introducing it to webclient, where it seems that they have locked the implementation to using a record?

Lombok's @Builder annotation solves this problem. And it even supports records. :) For non-records, a maybe even better solution would be to use @NoArgsConstructor together with @Setter.

(I'll make an issue (and a PR for my suggestion) if you think this is misplaced)

Please create a separate issue and PR. Thanks!

@wing328 wing328 merged commit a6280d9 into OpenAPITools:master Feb 17, 2025
15 checks passed
@cchacin cchacin deleted the fix-static-class branch February 20, 2025 00:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants