[text analytics] Fixed Swagger validation issues for v3.1-preview.4 #12660
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Hi, @abhahn Thanks for your PR. I am workflow bot for review process. Here are some small tips. Any feedback about review process or workflow bot, pls contact swagger and tools team. vsswagger@microsoft.com |
Swagger Generation Artifacts
|
|
Hi @abhahn, Your PR has some issues. Please fix the CI sequentially by following the order of
|
openapi-pipeline-app
bot
added
CI-FixRequiredOnFailure
and removed
CI-FixRequiredOnFailure
labels
…order to support passing the same thing in the body of a Task object
openapi-pipeline-app
bot
added
CI-FixRequiredOnFailure
and removed
CI-FixRequiredOnFailure
labels
openapi-pipeline-app
bot
added
CI-FixRequiredOnFailure
and removed
CI-FixRequiredOnFailure
labels
openapi-pipeline-app
bot
added
CI-FixRequiredOnFailure
and removed
CI-FixRequiredOnFailure
labels
nschonni
reviewed
Jan 27, 2021
openapi-pipeline-app
bot
added
CI-FixRequiredOnFailure
and removed
CI-FixRequiredOnFailure
labels
…l validation and linting
abhahn
changed the title
ctufts
reviewed
Jan 27, 2021
| }, | ||
| "name": "piiCategories", | ||
| "default": "Default", | ||
| "x-ms-client-default": "Default", |
There was a problem hiding this comment.
@abhahn this doesn't have to be ["Default"] due to type: array? Or is this basically specifying that out of the enums, use this as the default value in the array?
There was a problem hiding this comment.
Yeah, to be completely honest I don't really understand why this works but setting "default": ["Default"] doesn't. This is the only way that I could get both Swagger model validation and Swagger linting to pass. I think it may have the consequence of leaving the endpoint with no default value from the standpoint of APIM, which may be a problem. If I understand correctly, I think x-ms-client-default will result in the generated SDK methods having a default value.
Of the things I tried, here are the combinations that will fail one step or the other: "default": ["Default"] (fails linting), "default": "Default" (fails model validation), and "x-ms-client-default": ["Default"] (fails linting).
I think that "default": ["Default"] should be the correct way (even though it is failing) because it is explicitly shown as the way to set default values for enums in the official OpenApi 2.0 docs. Unfortunately this page doesn't have anchors, but if you Ctrl+F you can find the following text for the example: "You can also specify the default array that the server will use if this parameter is omitted"
There was a problem hiding this comment.
good catch, I didn't see the bit in the doc that allows you to omit the brackets
There was a problem hiding this comment.
@tjprescott , just wondering if you could help us to understand why using an array to represent the default value using the default property won't work here. I'd like to resolve this as soon as possible so we can merge the main PR for v3.1-preview.4.
There was a problem hiding this comment.
@abhahn and I discussed this offline.
Given the property/parameter is an array type, like Abby pointed the swagger should represent the Default in an array format. But today only the property/parameter with schemas - string, integer, long, float, double, boolean is supported.
We kicked off a discussion with the AutoRest core team to evaluate enabling array schema for x-ms-client-default extension.
It may take some time for AutoRest core to evaluate and decide on the this feature request.
Meantime, we should not use this extension for array cases, as it cannot be translated to meaningful representation in autorest or any languages.
There was a problem hiding this comment.
thanks! Laurent @lmazuel for all offline help on this.
thediris
suggested changes
Jan 28, 2021
...ication/cognitiveservices/data-plane/TextAnalytics/preview/v3.1-preview.4/TextAnalytics.json
Outdated
Show resolved
Hide resolved
openapi-pipeline-app
bot
added
CI-FixRequiredOnFailure
and removed
CI-FixRequiredOnFailure
labels
|
@anuchandy , could you please have a look at this PR when you have a chance? I actually have a question in the comments above regarding the use of the |
anuchandy
approved these changes
Feb 3, 2021
anuchandy
merged commit 2bd1245
into
Azure:dev-cognitiveservices-TextAnalytics-v3.1-preview.4
anuchandy
added a commit
that referenced
this pull request
Feb 12, 2021
* Adds base for updating TextAnalytics from version preview/v3.1-preview.3 to version v3.1-preview.4 * Updates readme * Updates API version in new specs and examples * add entity linking task (#12366) * Opinion Mining API changes for Preview.4 (#12344) * Adding changed example * json change for om part 1 * Change API structure as well * refine text again * Getting back to false by default * word chagne * . * final wording change * fix for readme.md Co-authored-by: josuh <josuh@microsoft.com> * Analyze: made task results optional (#12373) * Made results array optional since tasks can be in a failed state with no results * Fixing a syntax issue in the README * Attempting to address an Autorest error in the README * Attempting to address an Autorest error in the README * Added some info about v3.1-preview.4 to the python readme file; attempting to correct syntax issues in readme.md * Attempting to fix generation for the Python SDK * Attempting to fix generation for the Python SDK Co-authored-by: Abigail Hartman <Abigail.Hartman@microsoft.com> * add stringtype parameter to entitylinking task (#12467) * TA v3.1-preview.4 - updated examples and definitions for TA for Health (#12340) * updated examples and health definitions for new preview version * fixed linting issue in readme * removed old properties in ta.json * undo changes to sentiment portion of response, update health response * fixed spelling issues * fixed more typose * remove linting issue in readme * added enums to assertions * add in ms-enum extension * ran prettier json linter over files * made assertion field singular * PII Types for 3.1-Preview.4 (#12392) * based off of correct branch now * change the request from enum to a generic list of strings * cleanup some other comments * add enum type * change type --> category * add categories to the swagger * update the types to make them read better * add in the NER types * add in address type as well * fix some typos * one last typo * Health descritpion and naming updates (#12616) * updated examples and health definitions for new preview version * fixed linting issue in readme * removed old properties in ta.json * undo changes to sentiment portion of response, update health response * fixed spelling issues * fixed more typose * remove linting issue in readme * added enums to assertions * add in ms-enum extension * ran prettier json linter over files * made assertion field singular * addressed comments and updated description * TA v3.1-preview.4 - Minor syntax changes to address comments on PR 12338 (#12650) * updated examples and health definitions for new preview version * fixed linting issue in readme * removed old properties in ta.json * undo changes to sentiment portion of response, update health response * fixed spelling issues * fixed more typose * remove linting issue in readme * added enums to assertions * add in ms-enum extension * ran prettier json linter over files * made assertion field singular * addressed comments and updated description * addressed comments for minor syntax changes * replaced all instances of well-known * fixed linting issue - pii category was set to list instead of string * [text analytics] Fixed Swagger validation issues for v3.1-preview.4 (#12660) * Trying to solve swagger validation issues * Removed x-ms-enum property from PiiCategories * Created a definition for PiiCategories in addition to a parameter in order to support passing the same thing in the body of a Task object * Ran 'prettier' to, uh... make it prettier. * Updated custom-words.txt * Corrected typos in Swagger and removed unneeded entries in custom-words.txt * Changed 'default' to 'x-ms-client-default' in order to pass both model validation and linting * Fixed a misspelled PII category * Reran prettier check * Removed an experimental line in the Swagger * Adding collectionFormat to fix an autorest error * Removing x-ms-client-default Co-authored-by: Abigail Hartman <Abigail.Hartman@microsoft.com> * Added Extensible Enum for Health Relation Types - Dev cognitiveservices text analytics v3.1 preview.4 (#12737) * updated examples and health definitions for new preview version * fixed linting issue in readme * removed old properties in ta.json * undo changes to sentiment portion of response, update health response * fixed spelling issues * fixed more typose * remove linting issue in readme * added enums to assertions * add in ms-enum extension * ran prettier json linter over files * made assertion field singular * addressed comments and updated description * addressed comments for minor syntax changes * replaced all instances of well-known * fixed linting issue - pii category was set to list instead of string * revert change to pii default * remove default value as this is an optional parameter * added extensible list to relation types for healthcare * revert change to pii default * changed enum type and made extensible via the modelAsString param * ran npm prettier check and fixed spelling error (#12947) Co-authored-by: Chris Tufts <christopher.tufts25@gmail.com> Co-authored-by: Mikael <mikael.sitruk@outlook.com> Co-authored-by: Joohee Suh <58048046+josuhazure@users.noreply.github.com> Co-authored-by: josuh <josuh@microsoft.com> Co-authored-by: Abigail Hartman <Abigail.Hahn@microsoft.com> Co-authored-by: Abigail Hartman <Abigail.Hartman@microsoft.com> Co-authored-by: Thaminda Edirisooriya <61993993+thediris@users.noreply.github.com>
mkarmark
pushed a commit
to mkarmark/azure-rest-api-specs
that referenced
this pull request
Jul 21, 2021
* Adds base for updating TextAnalytics from version preview/v3.1-preview.3 to version v3.1-preview.4 * Updates readme * Updates API version in new specs and examples * add entity linking task (Azure#12366) * Opinion Mining API changes for Preview.4 (Azure#12344) * Adding changed example * json change for om part 1 * Change API structure as well * refine text again * Getting back to false by default * word chagne * . * final wording change * fix for readme.md Co-authored-by: josuh <josuh@microsoft.com> * Analyze: made task results optional (Azure#12373) * Made results array optional since tasks can be in a failed state with no results * Fixing a syntax issue in the README * Attempting to address an Autorest error in the README * Attempting to address an Autorest error in the README * Added some info about v3.1-preview.4 to the python readme file; attempting to correct syntax issues in readme.md * Attempting to fix generation for the Python SDK * Attempting to fix generation for the Python SDK Co-authored-by: Abigail Hartman <Abigail.Hartman@microsoft.com> * add stringtype parameter to entitylinking task (Azure#12467) * TA v3.1-preview.4 - updated examples and definitions for TA for Health (Azure#12340) * updated examples and health definitions for new preview version * fixed linting issue in readme * removed old properties in ta.json * undo changes to sentiment portion of response, update health response * fixed spelling issues * fixed more typose * remove linting issue in readme * added enums to assertions * add in ms-enum extension * ran prettier json linter over files * made assertion field singular * PII Types for 3.1-Preview.4 (Azure#12392) * based off of correct branch now * change the request from enum to a generic list of strings * cleanup some other comments * add enum type * change type --> category * add categories to the swagger * update the types to make them read better * add in the NER types * add in address type as well * fix some typos * one last typo * Health descritpion and naming updates (Azure#12616) * updated examples and health definitions for new preview version * fixed linting issue in readme * removed old properties in ta.json * undo changes to sentiment portion of response, update health response * fixed spelling issues * fixed more typose * remove linting issue in readme * added enums to assertions * add in ms-enum extension * ran prettier json linter over files * made assertion field singular * addressed comments and updated description * TA v3.1-preview.4 - Minor syntax changes to address comments on PR 12338 (Azure#12650) * updated examples and health definitions for new preview version * fixed linting issue in readme * removed old properties in ta.json * undo changes to sentiment portion of response, update health response * fixed spelling issues * fixed more typose * remove linting issue in readme * added enums to assertions * add in ms-enum extension * ran prettier json linter over files * made assertion field singular * addressed comments and updated description * addressed comments for minor syntax changes * replaced all instances of well-known * fixed linting issue - pii category was set to list instead of string * [text analytics] Fixed Swagger validation issues for v3.1-preview.4 (Azure#12660) * Trying to solve swagger validation issues * Removed x-ms-enum property from PiiCategories * Created a definition for PiiCategories in addition to a parameter in order to support passing the same thing in the body of a Task object * Ran 'prettier' to, uh... make it prettier. * Updated custom-words.txt * Corrected typos in Swagger and removed unneeded entries in custom-words.txt * Changed 'default' to 'x-ms-client-default' in order to pass both model validation and linting * Fixed a misspelled PII category * Reran prettier check * Removed an experimental line in the Swagger * Adding collectionFormat to fix an autorest error * Removing x-ms-client-default Co-authored-by: Abigail Hartman <Abigail.Hartman@microsoft.com> * Added Extensible Enum for Health Relation Types - Dev cognitiveservices text analytics v3.1 preview.4 (Azure#12737) * updated examples and health definitions for new preview version * fixed linting issue in readme * removed old properties in ta.json * undo changes to sentiment portion of response, update health response * fixed spelling issues * fixed more typose * remove linting issue in readme * added enums to assertions * add in ms-enum extension * ran prettier json linter over files * made assertion field singular * addressed comments and updated description * addressed comments for minor syntax changes * replaced all instances of well-known * fixed linting issue - pii category was set to list instead of string * revert change to pii default * remove default value as this is an optional parameter * added extensible list to relation types for healthcare * revert change to pii default * changed enum type and made extensible via the modelAsString param * ran npm prettier check and fixed spelling error (Azure#12947) Co-authored-by: Chris Tufts <christopher.tufts25@gmail.com> Co-authored-by: Mikael <mikael.sitruk@outlook.com> Co-authored-by: Joohee Suh <58048046+josuhazure@users.noreply.github.com> Co-authored-by: josuh <josuh@microsoft.com> Co-authored-by: Abigail Hartman <Abigail.Hahn@microsoft.com> Co-authored-by: Abigail Hartman <Abigail.Hartman@microsoft.com> Co-authored-by: Thaminda Edirisooriya <61993993+thediris@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
MSFT employees can try out our new experience at OpenAPI Hub - one location for using our validation tools and finding your workflow.
Changelog
Please ensure to add changelog with this PR by answering the following questions.
Contribution checklist:
If any further question about AME onboarding or validation tools, please view the FAQ.
ARM API Review Checklist
Ensure to check this box if one of the following scenarios meet updates in the PR, so that label “WaitForARMFeedback” will be added automatically to involve ARM API Review. Failure to comply may result in delays for manifest application. Note this does not apply to data plane APIs, all “removals” and “adding a new property” no more require ARM API review.
Please ensure you've reviewed following guidelines including ARM resource provider contract and REST guidelines. Estimated time (4 hours). This is required before you can request review from ARM API Review board.
If you are blocked on ARM review and want to get the PR merged with urgency, please get the ARM oncall for reviews (RP Manifest Approvers team under Azure Resource Manager service) from IcM and reach out to them.
Breaking Change Review Checklist
If there are following updates in the PR, ensure to request an approval from API Review Board as defined in the Breaking Change Policy.
Action: to initiate an evaluation of the breaking change, create a new intake using the template for breaking changes. Addition details on the process and office hours are on the Breaking change Wiki.
Please follow the link to find more details on PR review process.