[Service Bus] [core-http] Implement ATOM based management API by ramya0820 · Pull Request #4880 · Azure/azure-sdk-for-js

ramya0820 · 2019-08-23T18:26:35Z

This PR includes end to end implementation of the ATOM management API and set of tests for CRUD operations on Queues.
Can be considered as a draft, as working on straightening out few more things - mostly code organization, simplifications.

cc @ramya-rao-a @AlexGhiondea

ramya0820 · 2019-08-23T18:31:56Z

@amarzavery @ramya-rao-a @nemakam @bterlson @daviwil
Let me know if you have any comments (mostly on high level things)

ramya0820 · 2019-08-23T18:35:48Z

Build successful but blocked on some other unrelated changes

SUCCESS (12)
================================
@azure/abort-controller (4.23 seconds)
@azure/core-auth (10.62 seconds)
@azure/core-http (18.85 seconds)
@azure/core-arm (10.59 seconds)
@azure/core-asynciterator-polyfill (4.43 seconds)
@azure/core-paging (2.20 seconds)
@azure/core-tracing (11.58 seconds)
@azure/event-processor-host (24.33 seconds)
@azure/service-bus (31.54 seconds)
@azure/storage-blob (22.70 seconds)
@azure/storage-file (19.05 seconds)
@azure/storage-queue (15.96 seconds)
================================

BLOCKED (5)
================================
@azure/core-amqp
@azure/keyvault-certificates
@azure/keyvault-keys
@azure/keyvault-secrets
@azure/event-hubs
================================

ramya-rao-a

@ramya0820 I have left a few high level comments, please take a look at them.

Once #4863 is merged, please rebase/merge from master, after which @bterlson will help in reviewing the changes specific to the http request/response code paths.

Also, please update the PR description with

pointers to the old code base from which this feature has been picked from. Especially the libraries which you have taken as a reference. Preferably, in a format which gives a 1:1 mapping between libraries and the files in this PR
any docs around the ATOM based management apis

ramya-rao-a · 2019-08-25T18:23:53Z

sdk/core/core-http/lib/credentials/serviceBusSASServiceClientCredentials.ts

+
+const HeaderConstants = Constants.HeaderConstants;
+
+export class ServiceBusSASServiceClientCredentials implements ServiceClientCredentials {


Since this credential is specific to a particular Azure service, it should live in the library for that service and not core-http.
Please move this to the Service Bus library.

ramya-rao-a · 2019-08-25T18:27:29Z

sdk/core/core-http/lib/policies/serviceBusAtomSerializationPolicy.ts

+ *  - serialize HTTP requests with input in JSON to ATOM based XML requests, and
+ *  - deserialize the ATOM based XML responses as they pass through the HTTP pipeline.
+ */
+export class ServiceBusAtomSerializationPolicy extends BaseRequestPolicy {


Does this policy make any assumptions on how Azure Service Bus expects requests and returns responses? If yes, then this should move to the Service Bus library. If this policy is service agnostic, then it can stay in core-http

ramya-rao-a · 2019-08-25T18:40:03Z

sdk/servicebus/service-bus/review/service-bus.api.md

    }

+// @public
+export class ServiceBusAtomManagementClient extends ServiceClient {


Customers will not be aware of the existence of a separate client in order to carry out the crud operations. They are aware of the existing ServiceBusClient and would expect all the crud operations to exist on it.

Below is what I would recommend:

Do not expose ServiceBusAtomManagementClient to customer, instead have it as property on ServiceBusClient.
When user creates a ServiceBusClient, you instantiate ServiceBusAtomManagementClient internally

Replicate all the crud operations on ServiceBusClient. These will call the corresponding counterparts in the ServiceBusAtomManagementClient

I looked into this, and since we establish AMQP connection at time of client instantiation, woukd this be a good design decision? cc @bterlson @chradek

ramya-rao-a · 2019-08-25T18:41:26Z

sdk/servicebus/service-bus/src/serviceBusAtomManagementClient.ts

+   * @param credentials Credentials needed for the client to connect to Azure.
+   * @param options The parameter options
+   */
+  constructor(connectionString: any, options?: ServiceClientOptions) {


Does the ATOM based management apis only work with Shared Access Keys?
Do they not work with AAD?

mikeharder · 2019-08-26T17:53:51Z

sdk/core/core-http/package.json

    "uuid": "^3.3.2",
-    "xml2js": "^0.4.19"
+    "xml2js": "^0.4.19",
+    "underscore": "1.4.x",


Suggested change

"underscore": "1.4.x",

"underscore": "^1.4.x",

Unless there is an exception, all dependencies should use caret ^ to float to latest within major.

@bterlson, do you think this is a runtime dependency worth taking? I know we wanted to minimize the number of runtime deps.

I do not. Please remove underscore dependency.

mikeharder · 2019-08-26T17:54:00Z

sdk/core/core-http/package.json

-    "xml2js": "^0.4.19"
+    "xml2js": "^0.4.19",
+    "underscore": "1.4.x",
+    "xmlbuilder": "0.4.3"


Suggested change

"xmlbuilder": "0.4.3"

"xmlbuilder": "^0.4.3"

Unless there is an exception, all dependencies should use caret ^ to float to latest within major.

@bterlson, do you think this is a runtime dependency worth taking? I know we wanted to minimize the number of runtime deps.

xmlbuilder I'm less sure about. Looking at the usage it seems fairly simple and could maybe be replaced with string templates instead. @ramya0820 let me know any thoughts on this?

I thought having a utility in place to work with serializing to / building XML docs seemed useful / preferable generally speaking.
Hence brought over the js2xml utility from azure-sb that uses both xmlbuilder and underscore. Would prefer if we could come up with an alternative library, like using https://www.npmjs.com/package/fast-xml-parser maybe?

I'll take another pass at the code to see if I can remove the dependencies and simplify / uniformize the XML handling bits in core-http (specifically the serialization bits).

I just worry that building these handlers from scratch, apart from incurring some more efforts and time, may

miss out any specific usage / corner cases that were being handled by the underlying libraries (xmlbuilder, underscore, etc.)(and probably why these were chosen in implementation in azure-sb?)

not be that easily extendable, scalable should any other XML related use-cases come in

For now, I'll take another look into minimizing / removing these 2 dependencies.

ShivangiReja · 2019-08-28T19:11:49Z

sdk/core/core-http/lib/atomResourceSerializerBase.ts

+import { stringIsEmpty, stringStartsWith } from "./util/utils";
+import { Constants } from "./util/constants";
+import { AtomHandler } from "./util/atomHandler";
+import { each, isUndefined, isArray, isObject, isDate } from "./util/utils";


This import can be combined with the line 17.

ShivangiReja · 2019-08-28T19:18:09Z

sdk/core/core-http/lib/atomXmlOperationSpec.ts

@@ -0,0 +1,14 @@
+import { ResourceSerializer } from "./resourceSerializer";


Copyright text is missing in this file.

ShivangiReja · 2019-08-28T19:33:09Z

sdk/core/core-http/lib/credentials/serviceBusSASServiceClientCredentials.ts

+    if (
+      sharedAccessKeyName === null ||
+      sharedAccessKeyName === undefined ||
+      typeof sharedAccessKeyName.valueOf() !== "string"


I think we don't need to add separate checks for null and undefined. We can combine these 2 checks.
Something like:

if(!(typeof sharedAccessKeyName ==="string" && sharedAccessKeyName )){ throw new Error("sharedAccessKeyName is missing."); }

ShivangiReja · 2019-08-28T19:35:33Z

sdk/core/core-http/lib/credentials/serviceBusSASServiceClientCredentials.ts

+    if (
+      sharedAccessKey === null ||
+      sharedAccessKey === undefined ||
+      typeof sharedAccessKey.valueOf() !== "string"


I think we don't need to add separate checks for null and undefined. We can combine these 2 checks.
Something like:

if(!(typeof sharedAccessKey==="string" && sharedAccessKey)){ throw new Error("sharedAccessKey is missing."); }

ramya0820 · 2019-08-28T21:18:13Z

Thanks for the comments so far! @mikeharder @ramya-rao-a @ShivangiReja @bterlson
As we had discussed offline earlier, this PR has been split into following two

[core-http] Update XML serialization to support Atom XML operations #4916 For changes specific to core-http
[Service Bus] Atom API support #4922 For changes specific to service bus

This PR will be closed in favor of other 2 PRs where the requested changes will be made at.

Implement ATOM based management API

6dc729d

ramya0820 requested review from KarishmaGhiya, ShivangiReja, chradek, daviwil, michaeljqzq, mikeharder and ramya-rao-a as code owners August 23, 2019 18:26

ramya0820 self-assigned this Aug 23, 2019

ramya0820 requested a review from amarzavery August 23, 2019 18:32

ramya-rao-a requested review from bterlson and removed request for KarishmaGhiya, ShivangiReja, amarzavery, daviwil, michaeljqzq and mikeharder August 25, 2019 18:09

ramya-rao-a suggested changes Aug 25, 2019

View reviewed changes

mikeharder requested changes Aug 26, 2019

View reviewed changes

Remove underscore library usage

4e43c2c

ShivangiReja reviewed Aug 28, 2019

View reviewed changes

ramya0820 closed this Aug 28, 2019


		const HeaderConstants = Constants.HeaderConstants;

		export class ServiceBusSASServiceClientCredentials implements ServiceClientCredentials {

		@@ -0,0 +1,14 @@
		import { ResourceSerializer } from "./resourceSerializer";

Conversation

ramya0820 commented Aug 23, 2019

Uh oh!

ramya0820 commented Aug 23, 2019

Uh oh!

ramya0820 commented Aug 23, 2019

Uh oh!

ramya-rao-a 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ramya0820 Aug 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

ramya0820 commented Aug 28, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ramya0820 Aug 26, 2019 •

edited

Loading