First pass on Schema Registry convenience layer #10602

nguerrera · 2020-08-13T00:50:28Z

Contributes to #10437.

Implements convenience layer + unit tests.

Follow-up still needed after this before closing #10437:

Add samples
Set package to publish

I will do that in a follow-up PR

sdk/schemaregistry/schema-registry/src/constants.ts

sdk/schemaregistry/schema-registry/src/schemaRegistryClient.ts

nguerrera · 2020-08-17T23:30:10Z

#10659 tracks a test recorder issue that is blocking tests from working on node. recorder.skip(node, ...) with a link to that issue is used and we only have coverage on browser for now. Tests pass on node too when run against live service.

(This comment edited to reflect current state, other comments hidden as resolved.)

nguerrera · 2020-08-20T18:06:00Z

@ramya-rao-a @xirzec This is ready for review.

xirzec

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.

xirzec · 2020-08-21T00:22:09Z

sdk/schemaregistry/schema-registry/review/schema-registry.api.md

+
+// @public
+export interface Response {
+    _response: HttpOperationResponse;


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.

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. :)

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?

cc @sadasant, @jonathandturner

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

+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?

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.

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 😊)

Adding would be non-breaking, removing it would be breaking, you are correct in that.

OK, let's hide it then, seems like the best tie breaker given no strong opinions to type it. Thanks, all.

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.

nguerrera · 2020-08-21T00:59:43Z

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.

:)

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.

ramya-rao-a · 2020-08-21T02:38:56Z

sdk/schemaregistry/schema-registry/src/schemaRegistryClient.ts

+      schema.content,
+      options
+    );
+    return convertSchemaIdResponse(response);


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?

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.

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.

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. 😳

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;

Swagger:
https://raw.githubusercontent.com/Azure/azure-rest-api-specs/a90b9146e543d3eec11381bd52d3b6ff271b1b78/specification/schemaregistry/data-plane/Microsoft.EventHub/preview/2018-01-01-preview/schemaregistry.json

How I renamed the others:
https://gist.github.com/nguerrera/0621e7d58e69aabe948e037f5b5bb0db

ramya-rao-a · 2020-08-21T02:40:44Z

sdk/schemaregistry/schema-registry/src/generated/models/index.ts

 */
-export interface SchemaGetIdByContentHeaders {
+export interface SchemaQueryIdByContentHeaders {


Why are the generated models having the Schema prefix? I don't see anything in the swagger that would have caused this

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>

First pass on Schema Registry convenience layer

003ae0b

nguerrera requested review from xirzec and ramya-rao-a August 13, 2020 00:50

xirzec reviewed Aug 13, 2020

View reviewed changes

sdk/schemaregistry/schema-registry/src/constants.ts Show resolved Hide resolved

xirzec reviewed Aug 13, 2020

View reviewed changes

sdk/schemaregistry/schema-registry/src/schemaRegistryClient.ts Outdated Show resolved Hide resolved

xirzec reviewed Aug 13, 2020

View reviewed changes

sdk/schemaregistry/schema-registry/src/schemaRegistryClient.ts Outdated Show resolved Hide resolved

nguerrera added 3 commits August 13, 2020 14:50

Move interfaces and helpers to separate files

da7578b

Keep version updated automatically

ecc50a8

Add tests

b0955ca

This comment has been minimized.

Sign in to view

Fix tests

9083018

This comment has been minimized.

Sign in to view

HarshaNalluru mentioned this pull request Aug 18, 2020

[Schema Registry] Debug #10602 #10683

Closed

nguerrera added 2 commits August 18, 2020 16:01

Merge branch 'master' into schema-registry

cdad4e7

Update recordings

4d17b38

nguerrera added this to the [2020] September milestone Aug 18, 2020

This comment has been minimized.

Sign in to view

nguerrera marked this pull request as ready for review August 19, 2020 00:24

nguerrera requested a review from bterlson as a code owner August 19, 2020 00:24

nguerrera requested a review from xirzec August 19, 2020 00:25

nguerrera mentioned this pull request Aug 19, 2020

[Schema Registry] Add convenience layer for SchemaRegistryClient #10437

Closed

nguerrera added the Schema Registry label Aug 19, 2020

nguerrera added 2 commits August 19, 2020 07:42

Merge branch 'master' into schema-registry

efa2c45

Re-re-re-record

d2fd7f8

HarshaNalluru mentioned this pull request Aug 20, 2020

[Core HTTP] Bug Fix - TokenRefresher shouldn't be recreated for each request #10728

Merged

xirzec approved these changes Aug 21, 2020

View reviewed changes

nguerrera merged commit 7b4c1bf into Azure:master Aug 21, 2020

nguerrera deleted the schema-registry branch August 21, 2020 01:08

ramya-rao-a reviewed Aug 21, 2020

View reviewed changes

This was referenced Aug 27, 2020

Standardize the layout of the http _response property in output types #10887

Closed

[schema-registry] Implement avro-based serializer #10890

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

First pass on Schema Registry convenience layer #10602

First pass on Schema Registry convenience layer #10602

nguerrera commented Aug 13, 2020 •

edited

This comment has been minimized.

nguerrera commented Aug 17, 2020 •

edited

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.

nguerrera commented Aug 20, 2020

xirzec left a comment

xirzec Aug 21, 2020

nguerrera Aug 21, 2020 •

edited

ramya-rao-a Aug 21, 2020

ramya-rao-a Aug 21, 2020

xirzec Aug 21, 2020

xirzec Aug 21, 2020

nguerrera Aug 21, 2020

xirzec Aug 22, 2020

nguerrera Aug 22, 2020

nguerrera Aug 24, 2020

nguerrera commented Aug 21, 2020 •

edited

ramya-rao-a Aug 21, 2020 •

edited

nguerrera Aug 21, 2020 •

edited

ramya-rao-a Aug 21, 2020

nguerrera Aug 21, 2020 •

edited

nguerrera Aug 24, 2020

ramya-rao-a Aug 21, 2020

nguerrera Aug 21, 2020 •

edited

First pass on Schema Registry convenience layer #10602

First pass on Schema Registry convenience layer #10602

Conversation

nguerrera commented Aug 13, 2020 • edited

This comment has been minimized.

nguerrera commented Aug 17, 2020 • edited

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.

nguerrera commented Aug 20, 2020

xirzec left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nguerrera Aug 21, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nguerrera commented Aug 21, 2020 • edited

ramya-rao-a Aug 21, 2020 • edited

Choose a reason for hiding this comment

nguerrera Aug 21, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nguerrera Aug 21, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nguerrera Aug 21, 2020 • edited

Choose a reason for hiding this comment

nguerrera commented Aug 13, 2020 •

edited

nguerrera commented Aug 17, 2020 •

edited

nguerrera Aug 21, 2020 •

edited

nguerrera commented Aug 21, 2020 •

edited

ramya-rao-a Aug 21, 2020 •

edited

nguerrera Aug 21, 2020 •

edited

nguerrera Aug 21, 2020 •

edited

nguerrera Aug 21, 2020 •

edited