[JAVA-SPRING,KOTLIN-SPRING] - Enhance pageable validation by adding minSize and minPage constraints; Search also allOf references for constraints and defaults#23694
Conversation
…. Search also allOf references for constraints and defaults
There was a problem hiding this comment.
1 issue found across 17 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/SpringPageableScanUtils.java">
<violation number="1" location="modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/SpringPageableScanUtils.java:165">
P2: Array schemas without `items` can now NPE here because `ModelUtils.isArraySchema()` does not guarantee `schema.getItems()` is non-null.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
1 issue found across 6 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="modules/openapi-generator/src/test/java/org/openapitools/codegen/kotlin/spring/KotlinSpringServerCodegenTest.java">
<violation number="1" location="modules/openapi-generator/src/test/java/org/openapitools/codegen/kotlin/spring/KotlinSpringServerCodegenTest.java:5961">
P2: Removing @Test orphaned this regression test, so JUnit will no longer execute it.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
| } | ||
|
|
||
| @Test(description = "oneOf with discriminator generates thin sealed interface with Jackson annotations") | ||
| // ------------------------------------------------------------------------- |
There was a problem hiding this comment.
P2: Removing @test orphaned this regression test, so JUnit will no longer execute it.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At modules/openapi-generator/src/test/java/org/openapitools/codegen/kotlin/spring/KotlinSpringServerCodegenTest.java, line 5961:
<comment>Removing @Test orphaned this regression test, so JUnit will no longer execute it.</comment>
<file context>
@@ -5958,7 +5958,61 @@ private Map<String, Object> springCloudKotlinPagedModelProps() {
}
- @Test(description = "oneOf with discriminator generates thin sealed interface with Jackson annotations")
+ // -------------------------------------------------------------------------
+ // substituteGenericPagedModel — modelNameSuffix / modelNamePrefix
+ // -------------------------------------------------------------------------
</file context>
Tip: Review your code locally with the cubic CLI to iterate faster.
|
Should this also cover Do note that it might be that the project represent them the same internally, as they do for some OAS 3.1 concepts. |
Hmm, good point. I would expect that such things should be ideally already handled by the normalizer? Or is it problematic because the values are not integers? But thanks for raising this. I see my blind attempt to centralize the logic in the ModelUtils.resolveMaximum and ModelUtils.resolveMinimum might be a bit premature. As I was considering it mostly in the context of the page/size use. That was definitely short-sighted. |
|
I am unable to provide comments in code, since I get an external error (so might be that you are flooded with the same comment over and over later). What I am wondering is if I would also suggest that some shared concepts be broken out into methods to improve readability and reduce duplication. For example that Is broken out into a "getRefSchema()" (might be that it already exists?). In the same vein also maybe the same for the if-cases. E.g., This would change the code from "null, null, ..., null" to Heavily opinionated of course and goes against how everything else is done. But personally I find most of the code to be extremely hard to read when it really should not be give the trivial things that are done. |
One could also reason that it is out-of-scope and that someone that utilizes exclusiveMaximum will have to extend your initial logic to support their use-case. I believe the current structure and separation of responsibility should make it at least semi-straightforward. And as always, if one aim to support everything with the first version, then one might never get a first version. 😄 |
|
No, I think this is a really good point. I will try to fix it. This might bite someone, when they decide to rely on the method later. It is called *Utils after all (-: |
Functionality implemented in #23575 is a bit incomplete (it is fully functional, but can miss some edge cases). In cases when the
minimum/maximum/defaultvalues live on a schema referenced viaallOf, the validations/defaults fail to generate.This PR fixes the behavior to properly discover the validationminimum/maximumconstraints as well as thedefault.default, the inlinedefaultwins (overrides any value defined deeper in the schema)maximum, the smallestmaximumwins (so ifmaximumin allOf referenced schema is 75 and inlinemaximumis 50, then the resultingmaximumis 50)minimum, the biggestminimumwins (so ifminimumin allOf referenced schema is 100 and inlineminimumis 10, then the resultingminimumis 100)PR checklist
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.
master(upcoming7.x.0minor release - breaking changes with fallbacks),8.0.x(breaking changes without fallbacks)"fixes #123"present in the PR description)Summary by cubic
Adds minSize and minPage support to pageable validation and resolves page/size constraints and defaults through $ref and allOf. Also fixes paged-model substitution and suppression when model name transforms (suffix/prefix) are used.
New Features
@ValidPageablesupportsminSizeandminPagein Java and Kotlin; generators emit these when present.Bug Fixes
$ref/allOfwith intersection semantics (smallest max, largest min; inline default wins) and generates@PageableDefault/@ValidPageablefrom shared component schemas.items; improved array detection in sort enum scanning.PagedModel<T>and suppress paged/meta schemas correctly whenmodelNameSuffix/modelNamePrefixor name mappings are set (registry keyed by transformed names; removals use raw names).Written for commit 609fc87. Summary will update on new commits.