add support to generate subschemas as static nested classes #1380

wigbam · 2022-02-27T16:18:44Z

See issue #125

In regards to naming (inner vs nested), I believe 'nested' is a more correct term here (static nested class, to be precise):
https://docs.oracle.com/javase/tutorial/java/javaOO/nested.html

I have added integration tests for both trivial and not-so-trivial use cases, but perhaps there are more edge cases.

wigbam · 2022-02-28T09:22:54Z

This PR supersedes #1008 and #1160

hupfdule · 2022-04-29T09:52:28Z

As you are writing this PR supersedes #1160, what is the reason you created this PR?
What does it do differently than #1160? What additional functionality does it provide to #1160?

wigbam · 2022-04-29T10:28:59Z

As you are writing this PR supersedes #1160, what is the reason you created this PR?

Not sure I fully understand the question. I have added this PR in order to add the support for the feature in question.

What does it do differently than #1160?

The implementation logic is different and subjectively more elegant
It does not rely on static fields or any other questionable methods (Generate only one top-level class per schema URI by using inner classes #1160 (comment))

What additional functionality does it provide to #1160?

full unit test coverage for all affected classes
full integration test coverage for all functionality with multiple trivial and not so trivial cases

jsonschema2pojo-core/src/main/java/org/jsonschema2pojo/rules/ObjectRule.java

wigbam · 2022-04-29T10:40:57Z

jsonschema2pojo-core/src/main/java/org/jsonschema2pojo/rules/ObjectRule.java

                    fqn = fqn.substring(0, index) + ruleFactory.getGenerationConfig().getClassNamePrefix() + fqn.substring(index) + ruleFactory.getGenerationConfig().getClassNameSuffix();
                }

-                if (usePolymorphicDeserialization) {


It seems as usePolymorphicDeserialization branch had no difference in outcome from the other one, hence it could be removed. Left it in so as not to pollute the PR with unrelated stuff.

pom.xml

hupfdule · 2022-04-29T20:05:16Z

Hello,

and thanks for your fast answer!

As you are writing this PR supersedes #1160, what is the reason you created this PR?

Not sure I fully understand the question. I have added this PR in order to add the support for the feature in question.

Well, there already was an active PR for supporting that feature. Therefore I am asking. There is not much reason to create a PR that does exactly the same thing again. So there must be some differences.

What does it do differently than #1160?

The implementation logic is different and subjectively more elegant

I can't decide whether that's the case. Elegance is quite subjective. So that is something that the repository owner must decide.

It does not rely on static fields or any other questionable methods (Generate only one top-level class per schema URI by using inner classes #1160 (comment))

Well, that's nice. I am not happy with the fact that a static map is used in the other PR.

I found the following other differences between this PR and #1160:

Usage of the term "nested class" instead of "inner class".

I am fine with that. "Nested class" seems a bit more correct. Could be applied to the other PR as well, if desired.
Providing a short option -nc additionally to the long option --use-nested-classes.

Seems fine, too. Can also be applied to the other PR if desired.
Bigger changes in org.jsonschema2pojo.rules.ObjectRule, org.jsonschema2pojo.rules.TypeRule and org.jsonschema2pojo.util.NameHelper. This is the main difference between both PRs.

I didn't dive deep enough to understand why all of the changes were done, but I can directly spot these differences.
1. This PR is more intrusive than Generate only one top-level class per schema URI by using inner classes #1160 (in regards to changed code lines). More intrusive usually means less likely to get accepted by the repository owner.
2. This PR seems to not respect different schema files. What I mean is that it does not separate the generation of nested classes between different Schema URIs.
3. It doesn't seem to fully support the use case it is supposed to support. See the two example JSON files from Generate only one top-level class per schema URI by using inner classes #1160:
```
{
  "type" : "object",
  "properties" : {
    "ambiguousType" : {
      "type" : "object",
      "properties" : {
        "name" : "string"
      }
    },
    "One" : {
      "type" : "object",
      "properties" : {
        "pattern" : "string"
      }
    }
  }
}
```
  and
```
{
  "type" : "object",
  "properties" : {
    "ambiguousType" : {
      "type" : "object",
      "properties" : {
        "length" : "string"
      }
    },
    "Two" : {
      "type" : "object",
      "properties" : {
        "forecast" : "string"
      }
    }
  }
}
```
  Both define the type ambiguousType. This should be generated as a nested type and therefore do not create a name clash. However with your implementation, the ambiguousType is getting generated as a top-level type and therefore is subject to a name clash.

I don't care much about which PR gets approved and integrated into jsonschema2pojo as long as both provide the same functionality. At the moment I see a small advantage of yours (not needing a static map) and a bigger disadvantage (generating top-level classes where nested classes should be generated and not respecting schema URI boundaries).

Unfortunately both PRs are ignored by the repository owner up until now (ignored in that there was no reaction to them; not even a negative one). So we do not know what needs to be changed to get approved (apart from the comments on #1008 which was abandoned by the author).

wigbam · 2022-04-29T23:12:45Z

@hupfdule wow, many thanks for such a thorough review and your comments! I will address all of them individually soon, but the main priority is of course the functionality deficiency you have pointed out. Not sure how it got slipped through, I thought I had all corner cases covered. Again, thank you for pointing it out.

wigbam · 2022-05-03T21:32:29Z

@hupfdule, I have tried to reproduce the issue with the generation you have described, but I was unable to. I have added more explicit integration tests now, specifically canGenerateTheWholeDirectory() which generates all schemas in a given directory. The directory contains the following two files, each defining nestedOne and nestedTwo:

It passes fine and does not result in schemas defined inline being generated as top-level classes. Not sure if I am missing something?

hupfdule · 2022-05-20T14:45:50Z

@wigbam Sorry for the late reply. I didn't notice have answered.
I tried with your current version and it still does not work for the example files above.
Did you try it with those files? Please use the test UseInnerClassesIT from my PR and get it to work with your code.

Sorry, I have tried again. And I may be false. I have to try again.

hupfdule · 2022-05-20T18:48:49Z

Yes, it seems I was wrong. I didn't test it thoroughly, but it seems to be working.

But you should definitely not introduce changes unrelated to the actual Code. For example https://github.com/joelittlejohn/jsonschema2pojo/pull/1380/files#diff-e8700089f1b3fd621a3f5dc397def9056c689291769b3aa00222bdde3c8ff846L240. Is it necessary to remove an unused branch to fulfil the purpose of your PR? If not, then don't change it.

Also your changes to the pom should be reverted. They have nothing to do with the actual functionality you want to introduce it should not be part of this PR (you can file a second PR for that purpose if you want).

Now you should raise awareness on this PR. @joelittlejohn Can you give us some hints on whether you would accept either this PR or #1160? Would you consider to merge one of them (and which one) and what changes are necessary to get approved?

wigbam · 2022-05-20T19:21:29Z

Yes, it seems I was wrong. I didn't test it thoroughly, but it seems to be working.

Great! Thanks for confirming!

But you should definitely not introduce changes unrelated to the actual Code. For example https://github.com/joelittlejohn/jsonschema2pojo/pull/1380/files#diff-e8700089f1b3fd621a3f5dc397def9056c689291769b3aa00222bdde3c8ff846L240. Is it necessary to remove an unused branch to fulfil the purpose of your PR? If not, then don't change it.

I don't agree and the reasoning is as follows:

The class in question had to be modified for the purpose of introducing the new functionality
The class in question did not have any unit tests to begin with
In order for the class to be properly unit tested it would have been necessary create unit tests for both branches which would have been pointless since they are equivalent
No integration tests were broken

Also your changes to the pom should be reverted. They have nothing to do with the actual functionality you want to introduce it should not be part of this PR (you can file a second PR for that purpose if you want).

I can agree here, those were not related to the PR. I will revert.

jsonschema2pojo-ant/src/main/java/org/jsonschema2pojo/ant/Jsonschema2PojoTask.java

wigbam · 2023-07-12T20:46:26Z

I am going to keep this up-to-date in case it might get another look one day.

hupfdule · 2023-08-02T14:57:15Z

I just noticed that your PR does not generate the nested class as static. Is there some reason for that? It makes using a builder more complicated as the nested class cannot be instantiated without the enclosing class. Therefore it cannot be generated in a single step.

wigbam · 2023-08-02T16:29:59Z

I just noticed that your PR does not generate the nested class as static. Is there some reason for that? It makes using a builder more complicated as the nested class cannot be instantiated without the enclosing class. Therefore it cannot be generated in a single step.

Oh boy 🤦‍♂️ somehow it slipped through the cracks. I have it working locally. Will update today/tomorrow. Many thanks for the good spot!

RanABI · 2023-08-17T14:24:01Z

Hi guys :) Anything new with this PR? This would be a super useful addition.

hupfdule · 2023-09-06T19:08:49Z

Oh boy 🤦‍♂️ somehow it slipped through the cracks. I have it working locally. Will update today/tomorrow. Many thanks for the good spot!

Hi, any update on it? I’d like to use your patch for a current project.

wigbam · 2023-09-06T21:41:51Z

Oh boy 🤦‍♂️ somehow it slipped through the cracks. I have it working locally. Will update today/tomorrow. Many thanks for the good spot!

Hi, any update on it? I’d like to use your patch for a current project.

Things been quite hectic over the past month, but all is fixed and updated now. Please give it a try.

hupfdule · 2023-10-10T15:22:17Z

Sorry that it took such a long time to answer, but I didn’t find the time to check earlier.

Basically it looks good. The generated files seem to correct.

Only one small remark. In https://github.com/wigbam/jsonschema2pojo/blob/6060219f977ead894f25b6d6b2c098ef7195074d/jsonschema2pojo-maven-plugin/src/main/java/org/jsonschema2pojo/maven/Jsonschema2PojoMojo.java#L143 the annotation

@Parameter(property = "jsonschema2pojo.useNestedClasses", defaultValue = "false")

should be added to let maven generate the help information for it. I don’t know why this in not generated automatically, but apparently the annotation is necessary.

wigbam · 2023-10-14T14:21:18Z

@Parameter(property = "jsonschema2pojo.useNestedClasses", defaultValue = "false")

Thanks for spotting. I have added the annotation as per above now.

wigbam force-pushed the nested_classes branch 2 times, most recently from 885c6ae to 1b83b04 Compare February 27, 2022 16:24

wigbam mentioned this pull request Feb 27, 2022

Generate inner public static classes for object properties. #125

Open

wigbam force-pushed the nested_classes branch from 1b83b04 to c99c8d7 Compare February 28, 2022 20:53

wigbam force-pushed the nested_classes branch 3 times, most recently from 0a10483 to 39636fc Compare March 14, 2022 23:17

wigbam commented Apr 29, 2022

View reviewed changes

jsonschema2pojo-core/src/main/java/org/jsonschema2pojo/rules/ObjectRule.java Outdated Show resolved Hide resolved

wigbam commented Apr 29, 2022

View reviewed changes

pom.xml Outdated Show resolved Hide resolved

wigbam force-pushed the nested_classes branch 3 times, most recently from 2b82ee0 to 6615997 Compare March 17, 2023 16:38

wigbam changed the title ~~add support to generate subschemas as nested classes~~ add support to generate subschemas as static nested classes Mar 18, 2023

wigbam commented Mar 28, 2023

View reviewed changes

jsonschema2pojo-ant/src/main/java/org/jsonschema2pojo/ant/Jsonschema2PojoTask.java Show resolved Hide resolved

wigbam force-pushed the nested_classes branch from 007b9de to 83edb0a Compare September 6, 2023 21:37

wigbam force-pushed the nested_classes branch from 83edb0a to 12b54ad Compare September 6, 2023 21:38

wigbam force-pushed the nested_classes branch from 12b54ad to 6060219 Compare September 10, 2023 15:29

Stepan Kolesnik and others added 6 commits July 23, 2024 19:39

add support to generate subschemas as nested classes

b14f073

add more tests, use hamcrest matchers in assertions

4fb13b6

add more tests, use hamcrest matchers in assertions

c4da4f8

fixes following the rebase

90ca530

nested classes should be static

e970534

add Maven Mojo parameter annotation

fa51fc7

wigbam force-pushed the nested_classes branch from 99cc317 to fa51fc7 Compare July 23, 2024 17:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add support to generate subschemas as static nested classes #1380

add support to generate subschemas as static nested classes #1380

wigbam commented Feb 27, 2022 •

edited

Loading

wigbam commented Feb 28, 2022

hupfdule commented Apr 29, 2022

wigbam commented Apr 29, 2022 •

edited

Loading

wigbam Apr 29, 2022 •

edited

Loading

hupfdule commented Apr 29, 2022

wigbam commented Apr 29, 2022

wigbam commented May 3, 2022 •

edited

Loading

hupfdule commented May 20, 2022 •

edited

Loading

hupfdule commented May 20, 2022

wigbam commented May 20, 2022

wigbam commented Jul 12, 2023

hupfdule commented Aug 2, 2023

wigbam commented Aug 2, 2023

RanABI commented Aug 17, 2023

hupfdule commented Sep 6, 2023

wigbam commented Sep 6, 2023

hupfdule commented Oct 10, 2023

wigbam commented Oct 14, 2023

add support to generate subschemas as static nested classes #1380

Are you sure you want to change the base?

add support to generate subschemas as static nested classes #1380

Conversation

wigbam commented Feb 27, 2022 • edited Loading

wigbam commented Feb 28, 2022

hupfdule commented Apr 29, 2022

wigbam commented Apr 29, 2022 • edited Loading

wigbam Apr 29, 2022 • edited Loading

Choose a reason for hiding this comment

hupfdule commented Apr 29, 2022

wigbam commented Apr 29, 2022

wigbam commented May 3, 2022 • edited Loading

hupfdule commented May 20, 2022 • edited Loading

hupfdule commented May 20, 2022

wigbam commented May 20, 2022

wigbam commented Jul 12, 2023

hupfdule commented Aug 2, 2023

wigbam commented Aug 2, 2023

RanABI commented Aug 17, 2023

hupfdule commented Sep 6, 2023

wigbam commented Sep 6, 2023

hupfdule commented Oct 10, 2023

wigbam commented Oct 14, 2023

wigbam commented Feb 27, 2022 •

edited

Loading

wigbam commented Apr 29, 2022 •

edited

Loading

wigbam Apr 29, 2022 •

edited

Loading

wigbam commented May 3, 2022 •

edited

Loading

hupfdule commented May 20, 2022 •

edited

Loading