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

First pass on Schema Registry convenience layer #10602

Merged
merged 9 commits into from
Aug 21, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions common/config/rush/pnpm-lock.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions sdk/schemaregistry/schema-registry/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,8 @@
"prettier": "@azure/eslint-plugin-azure-sdk/prettier.json",
"dependencies": {
"@azure/core-http": "^1.1.6",
"@azure/logger": "^1.0.0",
"@opentelemetry/api": "^0.10.2",
"tslib": "^2.0.0"
},
"devDependencies": {
Expand Down
67 changes: 67 additions & 0 deletions sdk/schemaregistry/schema-registry/review/schema-registry.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,73 @@

```ts

import { HttpOperationResponse } from '@azure/core-http';
import { OperationOptions } from '@azure/core-http';
import { PipelineOptions } from '@azure/core-http';
import { TokenCredential } from '@azure/core-http';

// @public
export interface GetSchemaByIdOptions extends OperationOptions {
}

// @public
export interface GetSchemaIdOptions extends OperationOptions {
}

// @public
export interface RegisterSchemaOptions extends OperationOptions {
}

// @public
export interface Response {
_response: HttpOperationResponse;
Copy link
Member

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.

Copy link
Contributor Author

@nguerrera nguerrera Aug 21, 2020

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

Copy link
Contributor

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

Copy link
Contributor

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

Copy link
Member

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?

Copy link
Member

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.

Copy link
Contributor Author

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

Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

}

// @public
export interface Schema extends SchemaDefinition, SchemaId {
}

// @public
export interface SchemaDefinition {
content: string;
}

// @public
export interface SchemaDescription extends SchemaDefinition {
group: string;
name: string;
serializationType: string;
}

// @public
export interface SchemaId {
id: string;
location: string;
locationById: string;
version: number;
}

// @public
export interface SchemaIdResponse extends SchemaId, Response {
}

// @public
export class SchemaRegistryClient {
constructor(endpoint: string, credential: TokenCredential, options?: SchemaRegistryClientOptions);
readonly endpoint: string;
getSchemaById(id: string, options?: GetSchemaByIdOptions): Promise<SchemaResponse>;
getSchemaId(schema: SchemaDescription, options?: GetSchemaIdOptions): Promise<SchemaIdResponse>;
registerSchema(schema: SchemaDescription, options?: RegisterSchemaOptions): Promise<SchemaIdResponse>;
}

// @public
export interface SchemaRegistryClientOptions extends PipelineOptions {
}

// @public
export interface SchemaResponse extends Schema, Response {
}


// (No @packageDocumentation comment for this package)

Expand Down
6 changes: 6 additions & 0 deletions sdk/schemaregistry/schema-registry/src/constants.ts
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
Expand Up @@ -7,29 +7,25 @@
*/

import * as coreHttp from "@azure/core-http";
import { SchemaRegistryClientOptionalParams } from "./models";
import { GeneratedSchemaRegistryClientOptionalParams } from "./models";

const packageName = "@azure/schema-registry";
const packageVersion = "1.0.0-preview.1";

export class SchemaRegistryClientContext extends coreHttp.ServiceClient {
export class GeneratedSchemaRegistryClientContext extends coreHttp.ServiceClient {
endpoint: string;
apiVersion: string;

/**
* Initializes a new instance of the SchemaRegistryClientContext class.
* @param credentials Subscription credentials which uniquely identify client subscription.
* Initializes a new instance of the GeneratedSchemaRegistryClientContext class.
* @param endpoint The Schema Registry service endpoint, for example
* https://mynamespace.servicebus.windows.net.
* my-namespace.servicebus.windows.net.
* @param options The parameter options
*/
constructor(
credentials: coreHttp.TokenCredential | coreHttp.ServiceClientCredentials,
endpoint: string,
options?: SchemaRegistryClientOptionalParams
options?: GeneratedSchemaRegistryClientOptionalParams
) {
if (credentials === undefined) {
throw new Error("'credentials' cannot be null");
}
if (endpoint === undefined) {
throw new Error("'endpoint' cannot be null");
}
Expand All @@ -44,13 +40,16 @@ export class SchemaRegistryClientContext extends coreHttp.ServiceClient {
options.userAgent = `${packageName}/${packageVersion} ${defaultUserAgent}`;
}

super(credentials, options);
super(undefined, options);

this.requestContentType = "application/json; charset=utf-8";

this.baseUri = options.endpoint || "{endpoint}/$schemagroups";
this.baseUri = options.endpoint || "https://{endpoint}";

// Parameter assignments
this.endpoint = endpoint;

// Assigning values to Constant parameters
this.apiVersion = options.apiVersion || "2018-01-01-preview";
}
}
27 changes: 18 additions & 9 deletions sdk/schemaregistry/schema-registry/src/generated/models/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ export interface SchemaGetByIdHeaders {
/**
* Serialization type for the schema being stored.
*/
xSerialization?: string;
xSchemaType?: string;
/**
* References specific schema in registry namespace.
*/
Expand All @@ -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 {
Copy link
Contributor

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

Copy link
Contributor Author

@nguerrera nguerrera Aug 21, 2020

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.

/**
* 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.
*/
Expand All @@ -81,7 +81,7 @@ export interface SchemaRegisterHeaders {
/**
* Serialization type for the schema being registered.
*/
xSerialization?: string;
xSchemaType?: string;
/**
* References specific schema in registry namespace.
*/
Expand All @@ -96,6 +96,11 @@ export interface SchemaRegisterHeaders {
xSchemaVersion?: number;
}

/**
* Defines values for SerializationType.
*/
export type SerializationType = "avro";

/**
* Contains response data for the getById operation.
*/
Expand Down Expand Up @@ -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.
Expand All @@ -146,7 +151,7 @@ export type SchemaGetIdByContentResponse = SchemaGetIdByContentHeaders &
/**
* The parsed HTTP response headers.
*/
parsedHeaders: SchemaGetIdByContentHeaders;
parsedHeaders: SchemaQueryIdByContentHeaders;
};
};

Expand Down Expand Up @@ -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.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,8 @@ export const SchemaGetByIdHeaders: coreHttp.CompositeMapper = {
name: "String"
}
},
xSerialization: {
serializedName: "x-serialization",
xSchemaType: {
serializedName: "x-schema-type",
type: {
name: "String"
}
Expand All @@ -62,19 +62,19 @@ export const SchemaGetByIdHeaders: coreHttp.CompositeMapper = {
}
};

export const SchemaGetIdByContentHeaders: coreHttp.CompositeMapper = {
export const SchemaQueryIdByContentHeaders: coreHttp.CompositeMapper = {
type: {
name: "Composite",
className: "SchemaGetIdByContentHeaders",
className: "SchemaQueryIdByContentHeaders",
modelProperties: {
location: {
serializedName: "location",
type: {
name: "String"
}
},
xSerialization: {
serializedName: "x-serialization",
xSchemaType: {
serializedName: "x-schema-type",
type: {
name: "String"
}
Expand Down Expand Up @@ -112,8 +112,8 @@ export const SchemaRegisterHeaders: coreHttp.CompositeMapper = {
name: "String"
}
},
xSerialization: {
serializedName: "x-serialization",
xSchemaType: {
serializedName: "x-schema-type",
type: {
name: "String"
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,11 @@
* Changes may cause incorrect behavior and will be lost if the code is regenerated.
*/

import { OperationURLParameter, OperationParameter } from "@azure/core-http";
import {
OperationURLParameter,
OperationQueryParameter,
OperationParameter
} from "@azure/core-http";

export const endpoint: OperationURLParameter = {
parameterPath: "endpoint",
Expand All @@ -31,6 +35,20 @@ export const schemaId: OperationURLParameter = {
}
};

export const apiVersion: OperationQueryParameter = {
parameterPath: "apiVersion",
mapper: {
// TODO: Manually patched for now due to https://github.com/Azure/azure-rest-api-specs/pull/10220#discussion_r469588293
// defaultValue: "2018-01-01-preview",
defaultValue: "2017-04",
isConstant: true,
serializedName: "api-version",
type: {
name: "String"
}
}
};

export const contentType: OperationParameter = {
parameterPath: ["options", "contentType"],
mapper: {
Expand Down Expand Up @@ -76,10 +94,10 @@ export const schemaName: OperationURLParameter = {
}
};

export const serializationType: OperationParameter = {
parameterPath: "serializationType",
export const xSchemaType: OperationParameter = {
parameterPath: "xSchemaType",
mapper: {
serializedName: "serialization-type",
serializedName: "X-Schema-Type",
required: true,
type: {
name: "String"
Expand Down