-
Notifications
You must be signed in to change notification settings - Fork 7
Don't override businessInformation #74
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
base: main
Are you sure you want to change the base?
Don't override businessInformation #74
Conversation
|
Note I also added a .prettierrc configuring, as far as I can see, the current way all code is formatted |
flaviostutz
left a comment
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 good! I let a few comments :)
| if ( | ||
| (!apiDefr.businessInformation && openapiDocument.info.contact?.email) || | ||
| openapiDocument.info.contact?.name | ||
| !apiDefr.businessInformation && |
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.
Even if businessInformation is set, we should use the info from info.contact.email/name, don't we?
Maybe we could change the logic to always try to set the info based on "info.contact" if it exists.
What do you think?
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.
That's indeed the general design question we need to find a good answer to! I think there's three options.
Let's take the version field as an example and say the OpenAPI document contains a version with value v1, and the user passes a version with value v2, e.g. new Wso2Api(..., ..., { apiDefinition: { version: 'v2' }, openapiDocument })
- The values from the OpenAPI document are only used to set defaults in apiDefinition. The user can override this by setting the value in apiDefinition. For the example above, the API gets created as version v2.
- The values from OpenAPI document take precedence over values provided by the user. For the example above, the API gets created as version v1.
- A mix of 1 and 2, depending on the field. This is the current behavior on main where description, version, and tags use the OpenAPI as a default, and where the four businessInformation fields always come from the OpenAPI document.
I think option 1 is the best as it gives clear defaults, and allows for the user to override these fields where needed.
Let's take a user that wants to learn about publishing to WSO2 and who takes the Petstore OpenAPI document as an example OpenAPI document. Do we want them to make changes to the OpenAPI document and set its contact information correctly, or do we want them to simply pass the correct contact information when writing the new Wso2API code?
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.
Great summary on this matter! I vote for option 2. As if we go for option 1, then we should change the name of the property to "default info xxx" to make it more clear and we don't want a breaking change now :)
The idea of this property was to be a "sugar" attribute to set these info correctly in OpenAI doc, but I see the mixed behavior causes confusion. So let's go to option 1.
What do you think?
Summary
This PR fixes/implements the following bugs/features
businessInformation, see wso2api: businessInformation is taken from OpenAPI document, and can't be overridden #72Certain fields in the
wso2ApiDefinitionget defaults taken the OpenAPI document, which can be overridden by the user providing them inwso2ApiDefinition, for example thedescription,version, andtags.The fields in
businessInformationfields,businessOwner,businessOwnerEmail,technicalOwner, andtechnicalOwnerEmailhowever were always taken from the OpenAPI document whenever the OpenAPI document contains ainfo.contact.email, and were not overridable.This PR takes defaults these fields from the OpenAPI document only when the user doesn't provide a
businessInformationfield.Closing issues
Fixes #72