[Service Bus] Update to use latest core-amqp #6861

ramya0820 · 2020-01-09T01:05:13Z

For more context refer to #5127

common/config/rush/common-versions.json

ramya-rao-a

The test code needs to be updated to use code instead of name on the MessagingError when checking the error types.

See the changes made to the Event Hubs library in #6673 for reference.

ramya-rao-a · 2020-01-14T01:59:34Z

sdk/servicebus/service-bus/src/core/batchingReceiver.ts

@@ -92,7 +92,7 @@ export class BatchingReceiver extends MessageReceiver {
        const sessionError = context.session && context.session.error;
        let error = new MessagingError("An error occurred while receiving messages.");
        if (sessionError) {
-          error = translate(sessionError);
+          error = translate(sessionError) as MessagingError;


Casting this way is useful if down the line we want to access properties on the error object that we know are available only on the MessagingError and not Error.

But the case here is not so. We are adding this cast as a means to get away from the error TS gives that Error | MessagingError is not assignable to MessagingError.

The right approach here would be to type the error object during declaration as Error | MessagingError.

Same applies to a few other cases in this PR.

cc @chradek for any additional thoughts here

All changes here are because we want to access code and retryable properties which appear on MessagingError, so the reasoning seems fair.
The typing union would't work because the properties are not common to both (iirc while working on this locally).

I agree that it's more correct for error should be declared as Error | MessagingError since that's what translate will actually return.

Related to that somewhat, you could also instantiate the default MessagingError later on if the error doesn't exist to avoid creating an error that will likely not be used (I'm assuming the sessionError will be available more often than not in the onSessionError handler).

@ramya0820 Sorry, when I wrote my response your comment wasn't showing yet.

It doesn't look like either code or retryable are used in the onSessionError handler, so there's no need to cast to MessagingError inside the handler.

Tested on local - in general declaring type as Error | MessagingError wouldn't suffice, and we'll still need the as MessagingError casting to work with TypeScript and current code.

Only changes to be updated seems to be the 2 declarations in this file - all other places we access code and/or retryable properties and require casting.

About setting default error instance later - agree, updated

Yes, I had noticed that about these 2 instances - I meant in general all of the changes in this PR were for accessing the properties, so agree with updates to these 2 instances to be more focused on getting around TypeScript only~

Although, would be have been nicer if we didn't have to require the casting to access the properties if possible. Was there a reason why Error had to be a possible return value?
(Looks like the docs may need to be updated on the utility as well - I mean on the translate utility in @azure/core-amqp)

Was there a reason why Error had to be a possible return value?

Yes, the PR #6673 has all the relevant discussions around the changes to the MessagingError class as well as for the translate() function.

Looks like the docs may need to be updated on the utility as well - I mean on the translate utility in @azure/core-amqp

They were updated as part of #6673 already to the below:

Translates the AQMP error received at the protocol layer or a SystemError into a MessagingError. All other errors are returned unaltered.

Got it.
About the docs, I was looking at the line below that

* @returns {MessagingError} MessagingError object.

Ah, that's a good catch. Please feel free to send a PR to update the return section of the docs in the master branch.

sdk/servicebus/service-bus/src/core/batchingReceiver.ts

sdk/servicebus/service-bus/test/serviceBusClient.spec.ts

ramya-rao-a · 2020-01-14T03:59:09Z

/azp run js - servicebus - tests

azure-pipelines · 2020-01-14T03:59:19Z

Azure Pipelines successfully started running 1 pipeline(s).

ramya-rao-a · 2020-01-14T03:59:48Z

The feature/service-bus-track-2 branch has been updated with the latest from master and therefore should have fixes for all the different live test failures we have seen.

Please wait until the live/integration test run triggered from this PR to complete before merging this PR.

ramya-rao-a

There are a total of 9 live test failures. Please take a look and address them.

7 test failures are regarding unexpected error name. These were found in Event Hubs as well. The errors in these test cases are expected to be different in node vs browser. The test cases need to be updated appropriately. You can refer to the corresponding test cases in Event Hubs for reference.
2 are regarding the createQueue() operation.

…e/azure-sdk-for-js into issue-5127-p5

ramya0820 · 2020-01-14T21:44:38Z

/azp run js - servicebus - tests

azure-pipelines · 2020-01-14T21:44:49Z

Azure Pipelines successfully started running 1 pipeline(s).

ramya0820 · 2020-01-14T23:27:49Z

/azp run js - servicebus - tests

azure-pipelines · 2020-01-14T23:27:59Z

Azure Pipelines successfully started running 1 pipeline(s).

* Pin core-amqp version to preview.1 * Update lock file * Fix merge conflicts * [Service Bus] Update to use core-amqp (#5324) * Use core-amqp in atom management client, comment tests that use token provider * [Service Bus] Update to use latest core-amqp (#6861) * [Service Bus] Update constructors to add overloads and use Azure Identity (#5436) * sync-versions: rhea-promise and @azure/core-amqp for service-bus * resolve merge conflicts appropriately * API Report File for "@Azure/service-bus" * Address feedback * SERVICEBUS_CONNECTION_STRING_BROWSER remove from karma.conf * update pnpm-lock file * update API Report File for "@Azure/service-bus" * remove rhea and rhea-promise dependencies * add "rhea-promise" back Co-authored-by: Ramya Rao <ramya.rao.a@outlook.com> Co-authored-by: ramya0820 <45977823+ramya0820@users.noreply.github.com>

Fix build

519166f

ramya0820 requested review from chradek, KarishmaGhiya, mikeharder and ramya-rao-a as code owners January 9, 2020 01:05

ramya0820 self-assigned this Jan 9, 2020

KarishmaGhiya reviewed Jan 9, 2020

View reviewed changes

common/config/rush/common-versions.json Show resolved Hide resolved

ramya-rao-a suggested changes Jan 9, 2020

View reviewed changes

This was referenced Jan 10, 2020

[Service Bus] Use AwaitableSender from rhea-promise to send messages #6903

Closed

[Service Bus] Update constructors to add overloads and use Azure Identity #5436

Merged

Ramya Raja added 2 commits January 13, 2020 15:43

Update to use error code

d284f8c

Fix cast

b96b4a6

ramya-rao-a reviewed Jan 14, 2020

View reviewed changes

Ramya Raja added 2 commits January 13, 2020 18:22

Update declaration

8732e8c

Update default error instantiation

645f42c

ramya-rao-a reviewed Jan 14, 2020

View reviewed changes

sdk/servicebus/service-bus/src/core/batchingReceiver.ts Outdated Show resolved Hide resolved

Update error type

9d9cab9

ramya-rao-a reviewed Jan 14, 2020

View reviewed changes

sdk/servicebus/service-bus/test/serviceBusClient.spec.ts Outdated Show resolved Hide resolved

ramya-rao-a approved these changes Jan 14, 2020

View reviewed changes

ramya-rao-a suggested changes Jan 14, 2020

View reviewed changes

Ramya Raja added 2 commits January 14, 2020 13:09

Make SessionLockLostError nonretryable

7e94dfa

Fix tests

b10dfa7

ramya0820 force-pushed the issue-5127-p5 branch from 5f7b75e to b10dfa7 Compare January 14, 2020 21:10

Ramya Raja added 3 commits January 14, 2020 13:13

Merge branch 'feature/service-bus-track-2' of https://github.com/Azur…

3bb1e9c

…e/azure-sdk-for-js into issue-5127-p5

Fix typo

30b8f86

Update type in test

8c563ad

Azure deleted a comment from azure-pipelines bot Jan 14, 2020

Add isMessagingError utility

adbeac4

ramya-rao-a approved these changes Jan 15, 2020

View reviewed changes

ramya-rao-a merged commit b04922a into Azure:feature/service-bus-track-2 Jan 15, 2020

ramya-rao-a mentioned this pull request Jan 15, 2020

[Service Bus] Update to use latest preview version of core-amqp #5127

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Service Bus] Update to use latest core-amqp #6861

[Service Bus] Update to use latest core-amqp #6861

ramya0820 commented Jan 9, 2020

ramya-rao-a left a comment

ramya-rao-a Jan 14, 2020

ramya0820 Jan 14, 2020

chradek Jan 14, 2020

chradek Jan 14, 2020 •

edited

Loading

ramya0820 Jan 14, 2020

ramya0820 Jan 14, 2020

ramya0820 Jan 14, 2020 •

edited

Loading

ramya-rao-a Jan 14, 2020 •

edited

Loading

ramya0820 Jan 14, 2020

ramya-rao-a Jan 15, 2020

ramya-rao-a commented Jan 14, 2020

azure-pipelines bot commented Jan 14, 2020

ramya-rao-a commented Jan 14, 2020 •

edited

Loading

ramya-rao-a left a comment •

edited

Loading

ramya0820 commented Jan 14, 2020

azure-pipelines bot commented Jan 14, 2020

ramya0820 commented Jan 14, 2020

azure-pipelines bot commented Jan 14, 2020

[Service Bus] Update to use latest core-amqp #6861

[Service Bus] Update to use latest core-amqp #6861

Conversation

ramya0820 commented Jan 9, 2020

ramya-rao-a left a comment

Choose a reason for hiding this comment

ramya-rao-a Jan 14, 2020

Choose a reason for hiding this comment

ramya0820 Jan 14, 2020

Choose a reason for hiding this comment

chradek Jan 14, 2020

Choose a reason for hiding this comment

chradek Jan 14, 2020 • edited Loading

Choose a reason for hiding this comment

ramya0820 Jan 14, 2020

Choose a reason for hiding this comment

ramya0820 Jan 14, 2020

Choose a reason for hiding this comment

ramya0820 Jan 14, 2020 • edited Loading

Choose a reason for hiding this comment

ramya-rao-a Jan 14, 2020 • edited Loading

Choose a reason for hiding this comment

ramya0820 Jan 14, 2020

Choose a reason for hiding this comment

ramya-rao-a Jan 15, 2020

Choose a reason for hiding this comment

ramya-rao-a commented Jan 14, 2020

azure-pipelines bot commented Jan 14, 2020

ramya-rao-a commented Jan 14, 2020 • edited Loading

ramya-rao-a left a comment • edited Loading

Choose a reason for hiding this comment

ramya0820 commented Jan 14, 2020

azure-pipelines bot commented Jan 14, 2020

ramya0820 commented Jan 14, 2020

azure-pipelines bot commented Jan 14, 2020

chradek Jan 14, 2020 •

edited

Loading

ramya0820 Jan 14, 2020 •

edited

Loading

ramya-rao-a Jan 14, 2020 •

edited

Loading

ramya-rao-a commented Jan 14, 2020 •

edited

Loading

ramya-rao-a left a comment •

edited

Loading