[core-client] Updated ServiceClient implementation to work with core-https by xirzec · Pull Request #9854 · Azure/azure-sdk-for-js

xirzec · 2020-07-02T01:05:24Z

This PR accomplishes most of the work of lifting the old ServiceClient implementation (and dependent code) out of core-http and updates it to work with core-https.

The only thing not yet supported is XML, because I need to create another (very small) package to provide the helpers and pull in the heavy XML dependency.

Otherwise, this should be in a good enough state for us to start updating AutoRest.TypeScript to work with it.

I cleaned up the serialization/deserialization code a bit, but I'm sure there are much more opportunities to do make it even cleaner. Please feel free to suggest whatever you think would be an improvement towards readability / simplicity.

I think I've looked at it too long at this point to be able to think of non-breaking cleanups. 😅

Also remember that the eventual goal is to deprecate most of this code so that we can have codegen directly compose custom serializers.

Fixes #8615

chradek

Overall I like the direction this is going in! One question I had was why isn't serialization in it's own policy? I liked that deserialization was moved into one.

chradek · 2020-07-08T22:23:37Z

sdk/core/core-client/src/base64.browser.ts

+// Licensed under the MIT license.
+
+// eslint-disable-next-line @azure/azure-sdk/ts-no-namespaces
+declare global {


@richardpark-msft could have used this recently 😉

nit: I wonder whether these could be useful for other libraries too. I've seen similar functions storage (for testing though). On the other hand they are small so probably fine to have copies.

chradek · 2020-07-08T22:39:37Z

sdk/core/core-client/src/dom.d.ts

+// Copyright (c) Microsoft Corporation.
+// Licensed under the MIT license.
+
+/// <reference lib="dom" />


Remind me because I'm fuzzy...this let's us reference DOM types as core-client developers right? But it doesn't actually pull in DOM for external consumers of this library? Does this make it so consumers don't have to specify DOM themselves as well when using this library?

I copied this from core-http because I was getting annoying compile errors, but I think it won't require external users to import that lib since they're not compiling our source and we don't re-export types that are part of dom.

It just is supposed to give us typing for things that are from dom when we feature detect them (e.g. in serializer.ts when it checks if Blob exists.)

/cc @willmtemple who knows a thing or two because he's seen a thing or two

chradek · 2020-07-08T22:40:35Z

sdk/core/core-client/src/interfaces.ts

+  PipelineRequest
+} from "@azure/core-https";
+
+export interface OperationRequest extends PipelineRequest {


Nit: Missing docs on OperationRequest.

This was a great comment because it made me realize that extending the request this way didn't work great with clone() on the request. I fixed that bug by moving these props into a grab-bag option that clone() can persist!

chradek · 2020-07-08T22:41:30Z

sdk/core/core-client/src/interfaces.ts

+ */
+export type QueryCollectionFormat = "CSV" | "SSV" | "TSV" | "Pipes" | "Multi";
+
+export type ParameterPath = string | string[] | { [propertyName: string]: ParameterPath };


Nit: missing docs

sdk/core/core-client/src/interfaces.ts

sdk/core/core-client/src/utils.ts

chradek · 2020-07-08T22:58:37Z

sdk/core/core-client/test/urlHelpers.ts

+// Copyright (c) Microsoft Corporation.
+// Licensed under the MIT license.
+
+// import { assert } from "chai";


This feels like a tease...

oops, this gets tested indirectly through the service client tests, though it might be useful to add more tests specifically later.

sdk/core/core-client/src/serializer.ts

joheredi

It looks great! I really like how much more readable this is 😄

joheredi · 2020-07-08T22:37:21Z

sdk/core/core-client/review/core-client.api.md

+
+// @public
+export const MapperTypeNames: {
+    readonly Base64Url: "Base64Url";


Thanks for sorting these 😄

I'll take credit but really I just replaced a crazy utility function that made a fake enum with a simple const object. 😅

joheredi · 2020-07-08T23:45:31Z

sdk/core/core-client/src/deserializationPolicy.ts

+      if (operationSpec.isXML && responseSpec.bodyMapper.type.name === MapperTypeNames.Sequence) {
+        valueToDeserialize =
+          typeof valueToDeserialize === "object"
+            ? valueToDeserialize[responseSpec.bodyMapper.xmlElementName!]


do we want to validate that xmlElementName exists instead of ! it? We may be able to provide a better error message if we check for it

I tweaked this logic slightly and had it fallback to the empty array when the element name doesn't exist.

joheredi · 2020-07-09T00:07:51Z

sdk/core/core-client/src/serviceClient.ts

+   * @param credential The credentials used for authentication with the service.
+   * @param options The service client options that govern the behavior of the client.
+   */
+  constructor(options: ServiceClientOptions = {}) {


I love the new constructor!

I tried very hard to get rid of the old cruft. Do you know if we can ditch ServiceClientCredentials yet?

AppConfiguration library still uses it. Not sure why their AppConfigurationCredential is not used via a request policy though.

Yeah, I think we can move this to using custom policies with AzureKeyCredential

joheredi · 2020-07-09T00:36:09Z

sdk/core/core-client/src/deserializationPolicy.ts

+  const expectedStatusCodes = Object.keys(operationSpec.responses);
+  const hasNoExpectedStatusCodes =
+    expectedStatusCodes.length === 0 ||
+    (expectedStatusCodes.length === 1 && expectedStatusCodes[0] === "default");


Just a note, and probably doesn't make sense to add support until we switch to the new way of serializing/deserializing.

Autorest supports x-ms-error-response which allows users to mark responses as errors in Swagger and still be able to provide alternate deserialization rules to "default", currently we'd just see those as just another success response.

yeah this logic is terrible and unchanged from the old stuff. I'm fine adding support here, but maybe you can file an issue to do it in a follow-up PR that is more scoped to the behavior change?

xirzec · 2020-07-10T00:19:41Z

Overall I like the direction this is going in! One question I had was why isn't serialization in it's own policy? I liked that deserialization was moved into one.

deserialization was its own policy before in core-http. The problem with serialization is that serviceClients call sendOperationRequest which does all the serialization and makes the pipeline request at which point there's nothing for a policy to do.

chradek · 2020-07-13T22:54:43Z

deserialization was its own policy before in core-http. The problem with serialization is that serviceClients call sendOperationRequest which does all the serialization and makes the pipeline request at which point there's nothing for a policy to do.

Is this something we can change in a future PR then, if not in this one? (Is there already an issue to track doing this?) It seems weird that we'd allow deserialization to be replaceable, but not serialization. We also have examples of APIs today that serialize very differently based on the content-type of the body (e.g. binary versus text) and it'd be convenient if in the code generator we could decide which serialization to apply without having to make changes to this library to support this use case.

xirzec · 2020-07-13T23:00:56Z

Is this something we can change in a future PR then, if not in this one? (Is there already an issue to track doing this?) It seems weird that we'd allow deserialization to be replaceable, but not serialization. We also have examples of APIs today that serialize very differently based on the content-type of the body (e.g. binary versus text) and it'd be convenient if in the code generator we could decide which serialization to apply without having to make changes to this library to support this use case.

My understanding is that future codgen will create custom mappers/deserializers using functional composition, so this layer will really just be "process the string form into an object model and call the appropriate function".

I think we could take the approach the other way too, so instead of building the request from an OperationSpec, we would have a way to pack the raw inputs (operation args) into headers/url query params/body contents.

If we are able to separate out the logic correctly, the actual code we need to keep in the policies/service client will be pretty minimal.

/cc @joheredi for his thoughts.

xirzec added 11 commits June 30, 2020 18:00

Code compiling

e46463b

Working on serializer tests

6a3a57b

Serialization tests passing

69a535c

Working on service client tests

fd7c416

Everything but deserialization tests passing for serviceclient

0136801

Port over deserialization policy

64e5209

Tests passing

40f70d6

tests passing

bd552c4

deuglify QueryCollectionFormat

d59605e

Add missing exports, resolve duplicate names

353113f

Add credentials back to ServiceClient

6a0cedc

xirzec added Client This issue points to a problem in the data-plane of the library. Azure.Core labels Jul 2, 2020

xirzec requested review from bterlson, chradek, daviwil, jeremymeng and joheredi July 2, 2020 01:05

xirzec requested a review from ramya-rao-a as a code owner July 2, 2020 01:05

xirzec self-assigned this Jul 2, 2020

xirzec added 3 commits July 1, 2020 18:06

Don't need this

9990976

Merge remote-tracking branch 'upstream/master' into newServiceClient

89fd354

Update deps

ce71123

chradek reviewed Jul 8, 2020

View reviewed changes

joheredi approved these changes Jul 9, 2020

View reviewed changes

xirzec added 2 commits July 9, 2020 17:09

add additionalInfo to PipelineRequest

3ed9896

PR feedback

f3b76e5

review feedback

6bfa8db

xirzec added 3 commits July 17, 2020 15:45

Merge remote-tracking branch 'upstream/master' into newServiceClient

d5bb3ac

Drop ServiceClientCredentials

6255c28

Merge remote-tracking branch 'upstream/master' into newServiceClient

70fb549

xirzec merged commit a5fda32 into Azure:master Jul 27, 2020

xirzec deleted the newServiceClient branch July 27, 2020 20:28

xirzec mentioned this pull request Jul 27, 2020

[core-http] sendOperationRequest needs an overload that takes OperationOptions #6457

Closed

Conversation

xirzec commented Jul 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chradek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

joheredi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xirzec commented Jul 10, 2020

Uh oh!

chradek commented Jul 13, 2020

Uh oh!

xirzec commented Jul 13, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

xirzec commented Jul 2, 2020 •

edited

Loading