-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
First pass on Schema Registry convenience layer #10602
Conversation
This comment has been minimized.
This comment has been minimized.
#10659 tracks a test recorder issue that is blocking tests from working on node. (This comment edited to reflect current state, other comments hidden as resolved.) |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@ramya-rao-a @xirzec This is ready for review. |
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.
One minor comment, but overall I think this looks good.
I kept wanting to find a way to simplify/reduce the number of interfaces, but the nature of the API seems to be very asymmetric.
|
||
// @public | ||
export interface Response { | ||
_response: HttpOperationResponse; |
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.
Is it common for this particular API that a developer would need to directly check headers or HTTP status code? If not, it might be worth omitting Response
from types to reduce the number of interfaces here.
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.
It was my understanding that we always offer the _response as a pattern, is that not so? I didn't realize this was a call we'd make per API. :)
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.
Oh boy!
I was under the assumption like Nick that we always have _response
in the output which would be the raw response of the http request.
But it looks none of the 3 Keyvault packages or the Text Analytics one follow this pattern
App Config and the 3 storage packages do follow this pattern
I do believe there is a guideline around this across languages that there should be a way for users to get to the raw response.
@bterlson Do you recall why Key Vault does not follow the above pattern?
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.
Meanwhile, I would recommend inlining the _response
in the SchemaIdResponse
and SchemaResponse
definitions instead of having a separate Response
type being exported from the package. It is not of much use to the user outside of SchemaIdResponse
and SchemaResponse
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.
+1 to inlining.
The contention around _response
to me has more to do if we type it or not. Providing the response as an escape hatch makes sense to me, but it seems to offer so little value for some clients. If the developer is always going to write try/catch and treat the catch as a failure, and if the status code isn't useful to them in determining what to do.... then why would they ever need to reach into the response?
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.
Yeah, I don't think anyone is arguing for removing that property (you actually can't today because core-http
makes it non-configurable.)
I don't think it's worth typing unless we actually show a sample of why you'd use it, but my feelings aren't that strong here.
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.
Is it accurate to say that we can always type it later if we hide it now?
In other words, can we can add members to these interfaces, return a derived interface, tack on & Response, etc. and not have it be considered a breaking change? (My paranoia, at least I hope it's paranoia, here comes from my past life on .NET BCL 😊)
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.
Adding would be non-breaking, removing it would be breaking, you are correct in that.
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.
OK, let's hide it then, seems like the best tie breaker given no strong opinions to type it. Thanks, all.
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.
I started on this in draft in #10800. I'm waiting for the discussion about the Java surface area tomorrow to further align to that.
:) Coincidentally, I just got off a call with @MiYanni where he had it down to the same return type for all the API in C#, which I thought sounded off, but he pointed out to me that when you don't have the content coming back, you have it coming in so you can still return it. We're going to discuss his changes on Tuesday and my plan is to make a pass as a follow-up to try to get consistent with what the group likes for C#. At a minimum there will likely be some renames, but hopefully also some simplification. I can drop _response in that pass if that's something we decide we want to do. |
schema.content, | ||
options | ||
); | ||
return convertSchemaIdResponse(response); |
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.
Apologies if this is already discussed.. what is the reason for not using the response directly here and in the other two methods?
Is it because it has extra properties that we don't want to expose or have properties using different names than what we want?
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.
The generated names are pretty bad right now with leading x from the x-header. And xSchemaType is also about to be renamed to xSerializationType. I've used the good name already sans x prefix.
With some work on the swagger it would be possible to get better names using http://azure.github.io/autorest/extensions/#x-ms-client-name.
I mentioned this gently on the swagger PR: Azure/azure-rest-api-specs#10220 (comment). That PR has gotten a ton of feedback and iteration, and I was rather late with that point and it wasn't blocking to just adapt the response like this.
After talking to @MiYanni, the plan is to go over his names and shape on Tuesday with an eye towards standardizing between languages. We should feed back the name choices down to the swagger too.
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.
The generated names are pretty bad right now with leading x from the x-header. And xSchemaType is also about to be renamed to xSerializationType. I've used the good name already sans x prefix.
I believe we can use what are called "transforms" when running the code generator that will use the desired names in the generated code. Going over the shape with Yanni and Adam sounds like a good idea.
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.
OK, let's try to get good names into the swagger first, and failing that I can look at transforms. My instinct is that we shouldn't be using transforms to rename things when we have a chance to make the swagger better. Maintaining transforms also seems at least as cumbersome as the tiny converter functions.
There were some other issues about the generated types, it put ?
on things that are always supposed to be returned (started a Teams thread on that), and the serialization type isn't open ended as it should be (started another one on that too), but I guess those can just be cast away as long as the names line up. When I made the comment on the swagger PR about not being blocked on the swagger getting good names, I was under the impression that these slight type differences would force me to copy, as I still haven't fully internalized a world where such casts are possible. 😳
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.
I played with the transforms and so far I cannot find a way to get this to be named anything but body
azure-sdk-for-js/sdk/schemaregistry/schema-registry/src/generated/models/index.ts
Line 111 in 7a26e1d
body: string; |
How I renamed the others:
https://gist.github.com/nguerrera/0621e7d58e69aabe948e037f5b5bb0db
*/ | ||
export interface SchemaGetIdByContentHeaders { | ||
export interface SchemaQueryIdByContentHeaders { |
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.
Why are the generated models having the Schema
prefix? I don't see anything in the swagger that would have caused this
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.
Only guessing from what I can see, but the operation IDs in the swagger of the form Schema_<Operation>
. The generator then appears to group all of the methods into client.Schema.<Operation>
and generate names like Schema<Operation>Headers
. It is maybe not especially helpful for a REST surface as small as SchemaRegistry (currently just three operations to use operation groups like this, what other group will there be than Schema
? :) I definitely did not want this indirection from client to one and only one thing to three methods in the public API. I think if the Schema_
were dropped from the swagger, the API would come out flatter and without this prefix. I think using Schema_ was actually suggested on the swagger PR, though.
Api Management - make /tenant endpoints ARM compliant in 2020-06-01-preview version (Azure#11549) * Adds base for updating Microsoft.ApiManagement from version stable/2019-12-01 to version 2020-06-01-preview * Updates readme * Updates API version in new specs and examples * Add support in API Management for Availability Zones (Azure#10284) * apim in azs * fix prettier check * PATCH should return 200 OK (Azure#10328) * add support for PATCH returning 200 OK * CI fixes prettier fix CI fixes part 2 * Password no longer a mandatory property when uploading Certificates * add missing x-ms-odata extension for filter support * +gatewayhostnameconfiguration protocol changes (Azure#10292) * [2020-06-01-preview] Update Oauth Server secrets Contract (Azure#10602) * Oauth server secrets contract * fix azureMonitor enum * API Management Service Deleted Services Resource (Azure#10607) * API Management Service Deleted Services Resource * Path fix * Lint + custom-words fixes * Location URI parameter for deletedservices Resource * GET for deletedservices by service name * Remove resourceGroupName from resource path * fixes * schema for purge operation * perttier applied * 204 response code added Co-authored-by: REDMOND\glfeokti <glfeokti@microsoft.com> * OperationNameFormat property added to Diagnostic contract (Azure#10641) * OperationNameFormat property added to Diagnostic contract * add azuremonitor to update contract Co-authored-by: REDMOND\glfeokti <glfeokti@microsoft.com> * [Microsoft.ApiManagement][2020-06-01-preview] Change Network Status response contract (Azure#10331) * Change Network Status response contract * Update examples for network status contract * ApiManagement - tenant/settings endpoints * ApiManagement - tenant/settings endpoints fix * ApiManagement - tenant/settings endpoints fix prettier * ApiManagement - tenant/settings endpoints fix 3 * ApiManagement - tenant/settings endpoints fix 4 * ApiManagement - tenant/settings endpoints fix 5 Co-authored-by: Samir Solanki <samirsolanki@outlook.com> Co-authored-by: maksimkim <maksim.kim@gmail.com> Co-authored-by: promoisha <feoktistovgg@gmail.com> Co-authored-by: REDMOND\glfeokti <glfeokti@microsoft.com> Co-authored-by: RupengLiu <rliu1211@terpmail.umd.edu> Co-authored-by: vfedonkin <vifedo@microsoft.com>
Contributes to #10437.
Implements convenience layer + unit tests.
Follow-up still needed after this before closing #10437:
I will do that in a follow-up PR