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.

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

Search: Handle selected properties for List operations #11393

Closed
heaths opened this issue Apr 16, 2020 · 6 comments · Fixed by #11929
Closed

Search: Handle selected properties for List operations #11393

heaths opened this issue Apr 16, 2020 · 6 comments · Fixed by #11929
Assignees
Labels
Client This issue points to a problem in the data-plane of the library. Search
Milestone

Comments

@heaths
Copy link
Member

heaths commented Apr 16, 2020

Many of our List operations for SearchServiceClient accept an optional list of property names, but for .NET this isn't as intuitive since model property names like Name won't actually work (server error): name is required. Additionally, while etag is also not recognized, @odata.etag (theunderlying property name) causes a different server error: that @odata.etag is not allowed in $select or $expand query parameters.

@heaths heaths added Search Client This issue points to a problem in the data-plane of the library. labels Apr 16, 2020
@heaths heaths added this to the [2020] May milestone Apr 16, 2020
@heaths heaths self-assigned this Apr 16, 2020
@brjohnstmsft
Copy link
Member

An alternative to exposing this parameter on the client would be to have Get[Resource]Names APIs instead. That's the original purpose behind adding $select to those REST APIs in the first place. Those APIs actually run much faster with $select=name because we use a different code path in the service that doesn't retrieve all the metadata, only enumerates the names.

@heaths
Copy link
Member Author

heaths commented Apr 21, 2020

/cc @tg-msft @sima-zhu @xirzec @xiangyan99 @bryevdv

I like that idea. I noticed several properties couldn't even be specified in $select like etags. Anyone have any concerns with just providing Get{Resource}Names methods instead, especially if we split up the clients anyway?

@heaths
Copy link
Member Author

heaths commented Apr 21, 2020

I was thinking about this last night and got to thinking about a compromise between the difficulty in properly maintaining allowed property names, discoverability and understanding of using that, and performance. Instead of Get{Resource}Names and a separate Get{Resource}s, what about Get{Resource}s(bool onlyName = true, ...)? Either should work well for other languages.

I think we should still keep Get{Resource}s because for Key Vault we still get a lot of requests for getting all secrets, for example. There, however, list and get operations were intentionally separated because there are separate list and get permissions. Instead, people have to list, and for each interesting resources get the entire thing, which is everything they just got + the sensitive information like a secret or key.

Though, as long as we end up separating out "sub-clients", having the Get{Resource}Names wouldn't blow up the method count per client anyway. We should still have Get{Resource}s for the reason I mentioned above using Key Vault as an example, though.

@brjohnstmsft
Copy link
Member

@heaths A parameter like onlyName would work, although the response would be less convenient to consume than having a dedicated Get...Names API. Choosing between them might come down to which is most discoverable.

@heaths
Copy link
Member Author

heaths commented Apr 21, 2020

Good point. With Get{Resource}Names we could just return a collection of strings. We'll stick with that suggestion as the recommendation then.

@tg-msft
Copy link
Member

tg-msft commented Apr 22, 2020

Is there any way to check how customers are actually using this from telemetry? I think it's fine to offer Get*Names for convenience, but we might still want to support $select if there are scenarios. I'm fine making it less usable though (i.e., not getting fancy with name translation if we're supporting Get*Names for the common case).

@heaths heaths modified the milestones: [2020] May, [2020] June Apr 30, 2020
heaths added a commit to heaths/azure-sdk-for-net that referenced this issue May 7, 2020
Fixes Azure#11393. For now, there's very little practical value for passing $select given restrictions in which fields can even be specified (limited) and that they may be cased differently than models (Search is case-sensitive). But the server does have an optimized code path for returning just names, so we expose those methods instead.
heaths added a commit that referenced this issue May 8, 2020
* Add Get{Resource}Names methods and remove $select support

Fixes #11393. For now, there's very little practical value for passing $select given restrictions in which fields can even be specified (limited) and that they may be cased differently than models (Search is case-sensitive). But the server does have an optimized code path for returning just names, so we expose those methods instead.
openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-net that referenced this issue Nov 12, 2020
Adding API to support generating access token for ApplicationInsights Profiler (Azure#11393)

* Adds base for updating Microsoft.Insights from version preview/2020-10-05-preview to version 2020-10-26-preview

* Updates readme

* Updates API version in new specs and examples

* Update readme to pointing to profilerToken_API.json

* Update operations list

* Add profiler token getter

* Wire up the defintions

* Fix error: additonal property of liveToken

* Append post action

* Update api-version for example

* Fix some small issues

* Update readme for the resolving autorest check issue

* From profilertoken to profilerToken

* Ran prettier

* Appending back missing readme for 2020-10 tag

* Fix some mistakes

* Use common error response

* Tag secret with x-ms-secret

* Remove list operations

* Clean up packages

* Making 2 post operations for token

* Remove unused operations_list.json example

* Align with official master

* Resolve conflicts

* Add x-ms-secret for the token

* Update error response schema ref

* Update operation ids

* Remove unused error response
@github-actions github-actions bot locked and limited conversation to collaborators Mar 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Client This issue points to a problem in the data-plane of the library. Search
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants