-
-
Notifications
You must be signed in to change notification settings - Fork 6.4k
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
Properties with custom types inheritance fix #18052
Properties with custom types inheritance fix #18052
Conversation
thanks for the PR cc @martin-mfg |
@martin-mfg can you please review when you've time? thank you. |
Hi, just some additional results from my experiments, in case this PR is referenced again in the future:
|
Any updates? |
// in openapi-generator, we also store type in `type` for 3.1 schema | ||
// to make it backward compatible with the rest of the code base. | ||
if (schema.getType() != null) { | ||
clone.setType(schema.getType()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like this logic is completely removed, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SchemaUtils.cloneSchema must not lose a type info after clone (instead of AnnotationUtils), so yep, I've removed it :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
FYI. we've refactored the code a bit and no longer using type
to store the actual type in 3.1 spec: #18577
return schema; | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
personally i prefer not having another class SchemaUtils just for cloneSchema
?
can we merge this (or add this back) into model utils instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👌
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Hi! Please notify me before merge, I'll resolve merge conflicts. |
Can you please do so and PM me via Slack when done? |
@wing328 sorry, I don't use slack. I've resolved merge conflicts. Had to add jackson-annotations dependency explicitly, because openapi-generator right now has different minor versions of jackson in dependencies (lower for jackson-datatype-threetenbp). |
#' @param suffix test suffix | ||
#' @param text Some text containing white spaces | ||
#' @param date A date | ||
#' @param ... Other optional arguments. | ||
#' @export | ||
initialize = function(`id` = NULL, `outcomes` = [SUCCESS, FAILURE], `suffix` = NULL, `text` = NULL, `date` = NULL, ...) { | ||
initialize = function(`id` = NULL, `outcomes` = ["SUCCESS","FAILURE"], `suffix` = NULL, `text` = NULL, `date` = NULL, ...) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one more minor request if you don't mind. can you please add a test for the following schema:
outcomes:
type: array
items:
type: string
enum:
- SUCCESS
- FAILURE
- SKIPPED
default:
- SUCCESS
- FAILURE
to ensure the default values are clone correctly for string enum?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure! I've added ModelUtilsTest.testCloneArrayOfEnumsSchema
fyi @OpenAPITools/generator-core-team |
PR merged. Thanks again for the PR. |
* custom types support in inheritance fix * files changed after scripts run * remove unused method * move cloneSchema to ModelUtils * imports * changes after scripts run * test cloning array of enums schema
@dreambrother can you please take a look at the above exception when you've time? To repeat it, please run |
Approach to deep clone Schema's introduced in #16992 breaks support for properties with custom types (added using schemaMappings or type/importMappings) when such properties are inherited using allOf. AnnotationsUtils#clone returns null for them. I've replaced AnnotationsUtils#clone with ObjectMapper's typed serialization/deserialization.
@OpenAPITools/generator-core-team
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
(upcoming 7.1.0 minor release - breaking changes with fallbacks),8.0.x
(breaking changes without fallbacks)