-
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
Changes from 1 commit
003ae0b
da7578b
ecc50a8
b0955ca
9083018
cdad4e7
4d17b38
efa2c45
d2fd7f8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
// Copyright (c) Microsoft Corporation. | ||
// Licensed under the MIT license. | ||
|
||
export const SDK_VERSION: string = "1.0.0-preview.1"; | ||
nguerrera marked this conversation as resolved.
Show resolved
Hide resolved
|
||
export const DEFAULT_SCOPE = "https://eventhubs.azure.net/.default"; | ||
export const LIB_INFO = `azsdk-js-schema-registry/${SDK_VERSION}`; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,41 @@ | ||
/* | ||
* Copyright (c) Microsoft Corporation. | ||
* Licensed under the MIT License. | ||
* | ||
* Code generated by Microsoft (R) AutoRest Code Generator. | ||
* Changes may cause incorrect behavior and will be lost if the code is regenerated. | ||
*/ | ||
|
||
import * as operations from "./operations"; | ||
import * as Models from "./models"; | ||
import * as Mappers from "./models/mappers"; | ||
import { GeneratedSchemaRegistryClientContext } from "./generatedSchemaRegistryClientContext"; | ||
import { GeneratedSchemaRegistryClientOptionalParams } from "./models"; | ||
|
||
class GeneratedSchemaRegistryClient extends GeneratedSchemaRegistryClientContext { | ||
/** | ||
* Initializes a new instance of the GeneratedSchemaRegistryClient class. | ||
* @param endpoint The Schema Registry service endpoint, for example | ||
* my-namespace.servicebus.windows.net. | ||
* @param options The parameter options | ||
*/ | ||
constructor( | ||
endpoint: string, | ||
options?: GeneratedSchemaRegistryClientOptionalParams | ||
) { | ||
super(endpoint, options); | ||
this.schema = new operations.Schema(this); | ||
} | ||
|
||
schema: operations.Schema; | ||
} | ||
|
||
// Operation Specifications | ||
|
||
export { | ||
GeneratedSchemaRegistryClient, | ||
GeneratedSchemaRegistryClientContext, | ||
Models as GeneratedSchemaRegistryModels, | ||
Mappers as GeneratedSchemaRegistryMappers | ||
}; | ||
export * from "./operations"; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -29,7 +29,7 @@ export interface SchemaGetByIdHeaders { | |
/** | ||
* Serialization type for the schema being stored. | ||
*/ | ||
xSerialization?: string; | ||
xSchemaType?: string; | ||
/** | ||
* References specific schema in registry namespace. | ||
*/ | ||
|
@@ -45,17 +45,17 @@ export interface SchemaGetByIdHeaders { | |
} | ||
|
||
/** | ||
* Defines headers for Schema_getIdByContent operation. | ||
* Defines headers for Schema_queryIdByContent operation. | ||
*/ | ||
export interface SchemaGetIdByContentHeaders { | ||
export interface SchemaQueryIdByContentHeaders { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why are the generated models having the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
/** | ||
* URL location of schema, identified by schema group, schema name, and version. | ||
*/ | ||
location?: string; | ||
/** | ||
* Serialization type for the schema being stored. | ||
*/ | ||
xSerialization?: string; | ||
xSchemaType?: string; | ||
/** | ||
* References specific schema in registry namespace. | ||
*/ | ||
|
@@ -81,7 +81,7 @@ export interface SchemaRegisterHeaders { | |
/** | ||
* Serialization type for the schema being registered. | ||
*/ | ||
xSerialization?: string; | ||
xSchemaType?: string; | ||
/** | ||
* References specific schema in registry namespace. | ||
*/ | ||
|
@@ -96,6 +96,11 @@ export interface SchemaRegisterHeaders { | |
xSchemaVersion?: number; | ||
} | ||
|
||
/** | ||
* Defines values for SerializationType. | ||
*/ | ||
export type SerializationType = "avro"; | ||
|
||
/** | ||
* Contains response data for the getById operation. | ||
*/ | ||
|
@@ -126,9 +131,9 @@ export type SchemaGetByIdResponse = SchemaGetByIdHeaders & { | |
}; | ||
|
||
/** | ||
* Contains response data for the getIdByContent operation. | ||
* Contains response data for the queryIdByContent operation. | ||
*/ | ||
export type SchemaGetIdByContentResponse = SchemaGetIdByContentHeaders & | ||
export type SchemaQueryIdByContentResponse = SchemaQueryIdByContentHeaders & | ||
SchemaId & { | ||
/** | ||
* The underlying HTTP response. | ||
|
@@ -146,7 +151,7 @@ export type SchemaGetIdByContentResponse = SchemaGetIdByContentHeaders & | |
/** | ||
* The parsed HTTP response headers. | ||
*/ | ||
parsedHeaders: SchemaGetIdByContentHeaders; | ||
parsedHeaders: SchemaQueryIdByContentHeaders; | ||
}; | ||
}; | ||
|
||
|
@@ -178,8 +183,12 @@ export type SchemaRegisterResponse = SchemaRegisterHeaders & | |
/** | ||
* Optional parameters. | ||
*/ | ||
export interface SchemaRegistryClientOptionalParams | ||
export interface GeneratedSchemaRegistryClientOptionalParams | ||
extends coreHttp.ServiceClientOptions { | ||
/** | ||
* Api Version | ||
*/ | ||
apiVersion?: string; | ||
/** | ||
* Overrides client endpoint. | ||
*/ | ||
|
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?
cc @sadasant, @jonathandturner
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 theSchemaIdResponse
andSchemaResponse
definitions instead of having a separateResponse
type being exported from the package. It is not of much use to the user outside ofSchemaIdResponse
andSchemaResponse
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.