Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[Hub Generated] Review request for Microsoft.Azure.Search.Service to add version 2017-11-11-preview #5410

Merged
merged 7 commits into from
Mar 20, 2019
Merged

[Hub Generated] Review request for Microsoft.Azure.Search.Service to add version 2017-11-11-preview #5410

merged 7 commits into from
Mar 20, 2019

Conversation

brjohnstmsft
Copy link
Member

@brjohnstmsft brjohnstmsft commented Mar 17, 2019 •
edited
Loading

If you are a MSFT employee you can view your work branch via this link.

Contribution checklist:

@brjohnstmsft
[Azure Search] Make connectionString create-only to fix validation wa…
eedbe7b 
…rning

This change is just to satisfy Swagger spec validation; It has no effect on
code generation (at least for C#).
@brjohnstmsft
[Azure Search] Fix various Swagger validation errors
5ef54f1 
This is our first major push towards making searchservice.json pass all
Swagger validation. The validator found several issues that this change
fixes:
  - "Extensible enums" were being modeled incorrectly. On the wire, they
    are just strings, but we were modeling them as objects with a "name"
    property. This reflected the generated code, but not the wire format.
    The solution is to model them as proper enums in the Swagger. This causes
    new validation errors, since these enums should be open-ended, but that's
    an issue with the validator not understanding "extensible enums"; it's
    not a problem with our spec per se.
  - Properties of Skill were marked as "required", when they were being
    omitted or set to null in examples. This is because these properties
    actually appear to be optional from the point of view of the REST API, so
    now we model them as such.
  - "documentdb" was deprecated as a DataSourceType, but not replaced with
    "cosmosdb" in the examples. This is now fixed.
  - The example for "Get Indexer Execution Status" included properties in the
    response that aren't modeled in the Swagger. These properties actually do
    appear on the wire, but they shouldn't and will be removed in a future API
    version. In the meantime, I have removed them from the example.
@brjohnstmsft
[Azure Search] Applying consistent JSON formatting to Swagger spec
28977ce
@AutorestCI
Copy link

AutorestCI commented Mar 17, 2019 •
edited
Loading

Automation for azure-sdk-for-js

Nothing to generate for azure-sdk-for-js

@AutorestCI
Copy link

AutorestCI commented Mar 17, 2019 •
edited
Loading

Automation for azure-sdk-for-go

Nothing to generate for azure-sdk-for-go

@AutorestCI
Copy link

AutorestCI commented Mar 17, 2019 •
edited
Loading

Automation for azure-sdk-for-python

Nothing to generate for azure-sdk-for-python

@AutorestCI
Copy link

AutorestCI commented Mar 17, 2019 •
edited
Loading

Automation for azure-sdk-for-ruby

Nothing to generate for azure-sdk-for-ruby

@azuresdkci
Copy link
Contributor

Can one of the admins verify this patch?

@AutorestCI
Copy link

AutorestCI commented Mar 17, 2019 •
edited
Loading

Automation for azure-sdk-for-node

Nothing to generate for azure-sdk-for-node

@azuresdkci azuresdkci requested a review from jianghaolu March 17, 2019 20:45
@AutorestCI
Copy link

AutorestCI commented Mar 17, 2019 •
edited
Loading

Automation for azure-sdk-for-java

Nothing to generate for azure-sdk-for-java

@brjohnstmsft
Copy link
Member Author

@jianghaolu Contrary to the PR title, this is not a new API version. These changes are meant to address validation errors that we have never attempted to fix before. See individual commits for details. I have already requested exceptions for the remaining validation errors as I believe them to be limitations or bugs in the validator itself.

@brjohnstmsft
Copy link
Member Author

FYI @Yahnoosh

Also FYI @arv100kri @mgottein since this contains Cognitive Search-related changes. See commits for details. A .NET SDK PR will follow once this is merged.

@brjohnstmsft
Copy link
Member Author

@jianghaolu Also, please ignore the auto-generated .NET SDK PR. Our SDK includes custom code and other scripts that run during code generation. The auto-generated PRs will probably never succeed for our .NET SDK. Ask @shahabhijeet if you need details.

@arv100kri
Copy link
Member

Regarding this

The example for "Get Indexer Execution Status" included properties in the
response that aren't modeled in the Swagger. These properties actually do
appear on the wire, but they shouldn't and will be removed in a future API
version. In the meantime, I have removed them from the example.

@mgottein please comment on whether the comment is accurate or not

arv100kri
arv100kri approved these changes Mar 17, 2019
View reviewed changes
Copy link
Member

@arv100kri arv100kri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally looks good, I had one request (NamedEntity -> Entity in examples) and the question I posed to @mgottein

@brjohnstmsft
Copy link
Member Author

Regarding this

The example for "Get Indexer Execution Status" included properties in the
response that aren't modeled in the Swagger. These properties actually do
appear on the wire, but they shouldn't and will be removed in a future API
version. In the meantime, I have removed them from the example.

@mgottein please comment on whether the comment is accurate or not

@arv100kri I am asserting that it is accurate. :) The Get Indexer Status API re-uses a model class from the Indexing API internally (IndexResult). This is an accident of implementation. That type includes "success" and "statusCode" flags which are meaningless for "ItemError" (errors never succeed, and status code is meaningless for at least some indexer error conditions). I assume that is why these properties are not modeled in the Swagger (it was probably Eugene who did this work, and I would have reviewed and approved it back then).

@brjohnstmsft
[Azure Search] Make inputs/outputs properties of Skill required
3cc4af0 
It turns out these properties are required by the API. Also replaced the
deprecated NER skill in examples.
@brjohnstmsft
[Azure Search] Standardize formatting of JSON examples
42f9bf7
@brjohnstmsft
[Azure Search] Rolling back NamedEntityRecognition example update
f10fddb
@brjohnstmsft
[Azure Search] Make Swagger spec accurately reflect the semantics of …
2119e75 
…Field

Before this change, the descriptions in the Swagger for the Field type did
not accurately describe the semantics of the Azure Search REST API,
particularly around what the default values of the searchable, filterable,
sortable, and facetable properties were. Instead, they reflected the defaults
of our custom implementation of Field in the .NET SDK.

This change corrects the descriptions to accurately reflect the REST API. It
also adds more detail in anticipation of generating REST API documentation
from the Swagger (which we don't currently do).

A few other changes to Field are related to code generation:
 - Removed the x-ms-external flag so that AutoRest can generate this type.
 - Removed x-nullable: false and x-ms-client-name since they again reflect
   only the custom .NET code and not the desired REST API semantics.
 - Added regex transform rules to readme.md so that we can keep some of our
   custom code for Field in .NET purely for backward compatibility purposes.
   Our intention going forward for other languages is to rely solely on code
   generation for the Field type.
@brjohnstmsft
Copy link
Member Author

@jianghaolu Can you review and merge this please?

@jianghaolu jianghaolu merged commit 5cf04ee into Azure:master Mar 20, 2019
@brjohnstmsft brjohnstmsft deleted the fix-search branch March 20, 2019 20:51
@brjohnstmsft brjohnstmsft mentioned this pull request Mar 20, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arv100kri arv100kri arv100kri approved these changes

@jianghaolu jianghaolu jianghaolu approved these changes

Assignees

@jianghaolu jianghaolu

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@brjohnstmsft @AutorestCI @azuresdkci @arv100kri @jianghaolu