[SPRING] Generate inner enum for query and path params by Zomzog · Pull Request #13351 · OpenAPITools/openapi-generator

Zomzog · 2022-09-04T22:59:32Z

I've added the inner class. The static * import for the delegate was the only solution I found for importing them.

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 
./bin/generate-samples.sh
./bin/utils/export_docs_generators.sh
```
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*.
For Windows users, please run the script in Git BASH.
File the PR against the correct branch: master (6.1.0) (minor release - breaking changes with fallbacks), 7.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.

Spring committee : @cachescrubber (2022/02) @welshm (2022/02) @MelleD (2022/02) @atextor (2022/02) @manedev79 (2022/02) @javisst (2022/02) @borsch (2022/02) @banlevente (2022/02)

welshm · 2022-09-07T13:22:30Z

modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/SpringCodegen.java


+    @Override
+    protected void updateModelForObject(CodegenModel m, Schema schema) {
+        super.updateModelForObject(m, schema); // PONY


Intentionally left this here?

I've removed it.

welshm · 2022-09-07T13:24:05Z

modules/openapi-generator/src/main/resources/JavaSpring/apiDelegate.mustache

 import javax.annotation.Generated;
 {{/useJakartaEe}}
-
+import static {{package}}.{{classname}}.*;


Would it be possible to avoid the * import and import the specific types?

For consistency, all the other imports above do not rely on *.

I've fixed it, I don't know if I must extract this part of the template somewhere, because it's done multiple times.

welshm · 2022-09-07T13:27:31Z

.../client/petstore/spring-cloud-spring-pageable/src/main/java/org/openapitools/api/PetApi.java

    )
    ResponseEntity<List<Pet>> findPetsByTags(
-        @NotNull @ApiParam(value = "Tags to filter by", required = true) @Valid @RequestParam(value = "tags", required = true) List<String> tags,
+        @NotNull @ApiParam(value = "Tags to filter by", required = true) @Valid @RequestParam(value = "tags", required = true) List&lt;String&gt; tags,


Looks like something happened here which didn't replace with < and > properly

Looks like a generation issue. I've just regenerate them and they are ok.

welshm · 2022-09-12T20:13:19Z

I think you need your other change in first to get these tests to pass - but the change looks good otherwise

MelleD · 2022-09-13T09:00:32Z

modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/SpringCodegen.java

    }

+    private Map<String, String> importFromMapping(String value) {
+        HashMap<String, String> e = new HashMap<>();


Do you really need a HashMap here?
If not it's better to use with a meaningful name:
Map<String, String> imports = new HashMap<>();

MelleD · 2022-09-13T09:05:13Z

@welshm and @Zomzog Short question. Does it really make sense to generate the enum in the API class/interface? Wouldn't it be better to have a separate file for the DTOs?

welshm · 2022-09-13T12:43:29Z

@welshm and @Zomzog Short question. Does it really make sense to generate the enum in the API class/interface? Wouldn't it be better to have a separate file for the DTOs?

That's a good callout - a separate file where the DTOs are located is probably a better result. It will make generating the imports a little harder

borsch · 2022-09-14T07:21:36Z

modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/SpringCodegen.java

            for (final CodegenOperation operation : ops) {
+                if (operation.allParams.stream().anyMatch(it -> it.isEnum)) {
+                    objs.getImports().add(importFromMapping("JsonCreator"));
+                    objs.getImports().add(importFromMapping("JsonValue"));


I doubt that @JsonCreator could handle enums as request parameter for cases when value != enum contant name(i.e. AVAILABLE("available") )

You can check this comment #3337 (comment)

What I'm thinking is that you have to leverage this PR changes #13349

I mean that all these enums should be added to that configuration file

borsch · 2022-09-16T11:13:42Z

@welshm and @Zomzog Short question. Does it really make sense to generate the enum in the API class/interface? Wouldn't it be better to have a separate file for the DTOs?

That's a good callout - a separate file where the DTOs are located is probably a better result. It will make generating the imports a little harder

I would leave it as it is. These enums are operations/class specific. Assume situation when you have two different operations from two different controllers, they both receive parameter action with enum type, but those have different sets of allowed values

IMHO: there should be a rule - as long as client declare enum as global type(components -> schemas -> MyEnum) it should be in separate class, in other cases(enum for model's field or enum for operation's parameter only) it should be inner enum to class, where it should be used

MelleD · 2022-09-16T11:30:41Z

@welshm and @Zomzog Short question. Does it really make sense to generate the enum in the API class/interface? Wouldn't it be better to have a separate file for the DTOs?

That's a good callout - a separate file where the DTOs are located is probably a better result. It will make generating the imports a little harder

I would leave it as it is. These enums are operations/class specific. Assume situation when you have two different operations from two different controllers, they both receive parameter action with enum type, but those have different sets of allowed values

IMHO: there should be a rule - as long as client declare enum as global type(components -> schemas -> MyEnum) it should be in separate class, in other cases(enum for model's field or enum for operation's parameter only) it should be inner enum to class, where it should be used

The issue is more on the client side like feign. Then you have to use an "interface enum" looks a little bit confusing.

borsch · 2022-09-17T05:44:07Z

@welshm and @Zomzog Short question. Does it really make sense to generate the enum in the API class/interface? Wouldn't it be better to have a separate file for the DTOs?

That's a good callout - a separate file where the DTOs are located is probably a better result. It will make generating the imports a little harder

I would leave it as it is. These enums are operations/class specific. Assume situation when you have two different operations from two different controllers, they both receive parameter action with enum type, but those have different sets of allowed values
IMHO: there should be a rule - as long as client declare enum as global type(components -> schemas -> MyEnum) it should be in separate class, in other cases(enum for model's field or enum for operation's parameter only) it should be inner enum to class, where it should be used

The issue is more on the client side like feign. Then you have to use an "interface enum" looks a little bit confusing.

nope, that's a common issue. you can't declare multiple enums with same name within same package

Zomzog · 2022-09-25T23:48:05Z

It was harder than expected because some enums are weird.
I've done some changes to the case of an ArrayEnum. It looks like it has mostly an impact on documentation for other generators.
I don't get why node3 failed. I will try to look at it later.

borsch · 2022-10-12T09:01:41Z

samples/client/petstore/java/apache-httpclient/docs/FakeApi.md


        FakeApi apiInstance = new FakeApi(defaultClient);
-        List<String> enumHeaderStringArray = Arrays.asList("$"); // List<String> | Header parameter enum test (string array)
+        List<String> enumHeaderStringArray = Arrays.asList("EnumHeaderStringArrayEnum.DOLLAR"); // List<String> | Header parameter enum test (string array)


This doesn't seems to be correct

Zomzog added 2 commits September 5, 2022 00:57

[SPRING] Generate inner enum for query and path params

84635ab

typo

ca6d1eb

welshm reviewed Sep 7, 2022

View reviewed changes

Zomzog and others added 4 commits September 12, 2022 20:44

regenerate samples

2007265

review

7c49f88

Review

d73e5fa

Merge branch 'master' into zomzog/13350_path_query_enum

6ce112f

MelleD reviewed Sep 13, 2022

View reviewed changes

borsch requested changes Sep 14, 2022

View reviewed changes

Zomzog added 3 commits September 25, 2022 21:52

Solve array enums

ea29642

Merge branch 'master' into zomzog/13350_path_query_enum

89d8fe4

fix option

dc53bba

Merge branch 'master' into zomzog/13350_path_query_enum

f150b6d

Zomzog force-pushed the zomzog/13350_path_query_enum branch from 651def2 to 6a23517 Compare October 9, 2022 21:49

fix merge

8941dfb

Zomzog force-pushed the zomzog/13350_path_query_enum branch from 6a23517 to 8941dfb Compare October 9, 2022 22:06

Zomzog closed this Oct 11, 2022

Zomzog reopened this Oct 11, 2022

borsch reviewed Oct 12, 2022

View reviewed changes

Uh oh!

Conversation

Zomzog commented Sep 4, 2022

PR checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

welshm commented Sep 12, 2022

Uh oh!

MelleD Sep 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MelleD commented Sep 13, 2022

Uh oh!

welshm commented Sep 13, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

borsch commented Sep 16, 2022

Uh oh!

MelleD commented Sep 16, 2022

Uh oh!

borsch commented Sep 17, 2022

Uh oh!

Zomzog commented Sep 25, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

MelleD Sep 13, 2022 •

edited

Loading