chore: Adjust openapi generator for azure spec #4953
Conversation
deekshas8
changed the title
deekshas8
changed the title
| if (isAllOfSchema(schema)) { | ||
| return { | ||
| ...(schema.allOf?.length && | ||
| parseXOfSchema(schema, refs, 'allOf', options)), | ||
| ...((schema.properties || 'additionalProperties' in schema) && | ||
| parseObjectSchema(schema, refs, options)) | ||
| }; | ||
| } |
There was a problem hiding this comment.
[req] I still believe combining allOf and type: object is just incorrect, therefore if both are set and we know a few cases where services do that, I would allow that and treat it as allOf only. Parsing other properties and handling them a certain way is just guess work and as far as I know we haven't seen examples of that before, did we?
Therefore my suggestion is to just move this down below anyOf so objects are prioritized lower.
There was a problem hiding this comment.
[pp] I think it could be helpful to have a warning when generating this (incorrect) type. What do you think?
There was a problem hiding this comment.
Added info log if allOf and properties are at the same level. It is restructured into the "inheritance" type as discussed
| return { | ||
| [xOf]: (schema[xOf] || []).map(entry => parseSchema(entry, refs, options)) | ||
| [xOf]: (schema[xOf] || []).map(entry => | ||
| parseSchema({ ...entry, required }, refs, options) |
There was a problem hiding this comment.
[req] In this case the schema would overwrite anything that is required in the actual entry. We should at least merge this, but I think we shouldn't do this at all.
There was a problem hiding this comment.
Removed this for now. Required will be considered in a followup
There was a problem hiding this comment.
I merge this now. Have also added a test for it.
There was a problem hiding this comment.
I would prefer if we could split PRs like this one by concern: allof + properties and schema naming.
I have some more questions:
- I assume the schema names including the prefix go through the unique name generator? Maybe a test would be nice?
- I think the name prefix is not a hidden property. I think that is ok, but then please make sure to document it in the portal as well.
- Consider not only supporting
propertiesfor the all of schemas, but alsoadditionalProperties.
| } | ||
|
|
||
| if (schema.allOf?.length) { | ||
| if (schema.properties) { |
There was a problem hiding this comment.
[req] I think this needs to be handled for anyOf and oneOf as well.
There was a problem hiding this comment.
Sorry I had somehow missed this again. I moved the logic to the parseXOfSchema function now.
| schema.properties || | ||
| (schema.additionalProperties && | ||
| typeof schema.additionalProperties === 'object') |
There was a problem hiding this comment.
[q] Shouldn't we add Record<string, any> to the type if schema.additionalProperties = true?
There was a problem hiding this comment.
if the answer to the question above is yes, you could consider extracting a function to check whether the given schema is an object schema and reusing it for the object check above as well.
There was a problem hiding this comment.
So, if the object has properties and schema.additionalProperties = true then it will add it. If its:
type: object
allOf:
- ...
additionalProperties: true
then I think not. I probably can just check for schema.additionalProperties instead of
schema.additionalProperties && typeof schema.additionalProperties === 'object'
and that should already fix it. Honestly I am forgetting now why I checked for object type. Let me try.
There was a problem hiding this comment.
Tested this after removing && typeof schema.additionalProperties === 'object' check. And it works.
2 participants
Closes https://github.com/SAP/cloud-sdk-backlog/issues/1238
detectedis required, it is not yet considered in generation process.