Azure / azure-sdk-for-js

This repository is for active development of the Azure SDK for JavaScript (NodeJS & Browser). For consumers of the SDK we recommend visiting our public developer docs at https://docs.microsoft.com/javascript/azure/ or our versioned developer docs at https://azure.github.io/azure-sdk-for-js.
MIT License
2.09k stars 1.2k forks source link

ServiceBusAdministrationClient update queue method deprecation #18506

Closed alxizr closed 2 years ago

alxizr commented 3 years ago

Hello, as per the Azure documentation the option to update the queue properties is deprecated as of 11.01.2021 so need to update the ServiceBusAdministrationClient class and remove the method

https://docs.microsoft.com/en-gb/rest/api/servicebus/update-queue

It doesn't work anyways and it took me couple of minutes to figure out why exactly :(

thanks

ramya-rao-a commented 3 years ago

Hey @alxizr

What is being deprecated is an older set of APIs. You can see details in

The ServiceBusAdministrationClient class in the @azure/service-bus package uses a different set of APIs that should be working as expected.

It doesn't work anyways and it took me couple of minutes to figure out why exactly :(

Can you share a code snippet to show what you are trying to do so that we can help figuring out why it does not work as expected for you?

alxizr commented 3 years ago

I am using Node.js and this npm package. I was trying to create the queue programmatically and then update its properties. The official documentation says that it is not possible to set a single property on the queue object but rather need to pass all of its properties to be able to update it, alas, we get nowhere unfortunately.

This is the piece of code I have an issue with:

` const { ServiceBusClient, ServiceBusAdministrationClient } = require("@azure/service-bus"); const SERVICE_BUS_CONNECTION_STRING = "my_connection_string"; const QUEUE_NAME = "my_queue_name";

const sbac = new ServiceBusAdministrationClient(SERVICE_BUS_CONNECTION_STRING); const sbc = new ServiceBusClient(SERVICE_BUS_CONNECTION_STRING);

const queue = (await sbac.queueExists(QUEUE_NAME)) ? await sbac.getQueue(QUEUE_NAME) : await sbac.createQueue(QUEUE_NAME); const sb_sender = await sbc.createSender(queue.name); `

As you can see the queue object when it is assigned gets an existing queue or a new instance. No matter if i pass the options object at creation or at update, it will not work.

Here are some of the properties I want to set, but not the only ones:

This piece of code will not work as well. await sbac.updateQueue({ ...queue, enableBatchedOperations: true, enablePartitioning: true, autoDeleteOnIdle: true, requiresDuplicateDetection: true, maxDeliveryCount: 50, });

This is a quote from the API itself

Updates the queue based on the queue properties provided. All queue properties must be set even though only a subset of them are actually updatable. Therefore, the suggested flow is to use the output from getQueue(), update the desired properties in it, and then pass the modified object to updateQueue().

See https://docs.microsoft.com/rest/api/servicebus/update-queue for more details.

You can see that this link is the same from before, where it says that the method is deprecated.

Hope you can help. Thanks

ramya-rao-a commented 3 years ago

Thanks for the details @alxizr

In https://docs.microsoft.com/rest/api/servicebus/update-queue, if you look at the other APIs that are siblings to the update queue in the toc like create queue, update topic etc, they all have the same deprecation note, but they are not the APIs used by the ServiceBusAdministrationClient.

@HarshaNalluru, we have tests that ensure the update queue operation on the ServiceBusAdministrationClient works as expected right? Can you share a code snippet that shows how to update the queue and then do a get operation to confirm the changes are in place?

HarshaNalluru commented 3 years ago

@alxizr

Quoting a snippet from here(https://github.com/Azure/azure-sdk-for-js/blob/main/sdk/servicebus/service-bus/samples/v7/typescript/src/advanced/administrationClient.ts).

const getQueueResponse = await serviceBusAdministrationClient.getQueue(queueName);
getQueueResponse.maxDeliveryCount = 12;
const updateQueueResponse = await serviceBusAdministrationClient.updateQueue(getQueueResponse);
console.log("(Updated)max delivery count = ", updateQueueResponse.maxDeliveryCount);

Just for reference and to clear any confusion, if you want to update a queue property,

Question 1

await sbac.createQueue(queueName);
const queue = await sbac.getQueue(queueName);
await sbac.updateQueue({ ...queue, enableBatchedOperations: true, enablePartitioning: true, autoDeleteOnIdle: true, requiresDuplicateDetection: true, maxDeliveryCount: 50, });

Why did this snippet of yours did not work?

There are multiple problems here.

Question 2

create didn't work fine?

Follow the "Better solution" section from "Question 1" section and try again maybe?

Let me know if you have more questions. Hope this solves your issues.

HarshaNalluru commented 3 years ago

To @ramya-rao-a

Should this

See https://docs.microsoft.com/rest/api/servicebus/update-queue for more details.

be updated to something like below

See https://docs.microsoft.com/rest/api/servicebus/update-queue for more details on the properties such as which of them can be updated.

to avoid confusion?

Or

Are there(or can we get) docs from the service team for references?

ramya-rao-a commented 3 years ago

Oh, I totally missed the part where @alxizr mentions that the link to https://docs.microsoft.com/rest/api/servicebus/update-queue is in our docs for ServiceBusAdministrationClient.updateQueue() itself! Sorry about that :(

@HarshaNalluru, we will have to work with the service folks to document the updatable properties in a different place and then link to that. @spelluru should be able to help there. Please work with him offline

alxizr commented 3 years ago

Thanks for the elaborative response.

@HarshaNalluru I want to address problem 1 - you are right, I copied over the wrong line. the actual line is autoDeleteOnIdle: "PT5M"

regarding the other properties, as i said, they are only an example, but some properties cannot be changed after the queue is created such as requiresSession, requiresDuplicateDetection, requiresSession, requiresDuplicateDetection, enablePartitioning, and name, and name

The only property I am able to update is actually is maxDeliveryCount from the default 10 to any other number. Another issue is that the namespace's tier. I have both basic and premium tiers but it is all the same.

I will leave it as is for now and create the queue manually via the portal to have the properties that i need. If you could investigate into this and see where fixes are required it will be much appreciated

alxizr commented 3 years ago

@HarshaNalluru @ramya-rao-a

Another question i have and if you can answer, it will be awesome.

Is it possible to use the Azure Service Bus Queue or Topic also on the client side with React for example ? I am looking for the best practice recommendation. I mainly use it on the backend actually and I have some use cases where the browser clint side would benefit from subcribing to the queue/topic.

HarshaNalluru commented 3 years ago

@alxizr thanks for the questions.

regarding the other properties, as I said, they are only an example, but some properties cannot be changed after the queue is created

Service Bus service doesn't allow changing those properties after the queue is created but requires all the properties as input for the update call. This is because it is a "PUT" request and not a "PATCH" request, meaning.. "undefined/null" for those (non-updatable) properties would mean that we are modifying those properties with undefined or default values.

The only property I am able to update is actually is maxDeliveryCount from the default 10 to any other number

There should be more fields that can be changed

interface FiledsThatCanBeChanged {
  deadLetteringOnMessageExpiration?: boolean;
  defaultMessageTtl?: string;
  duplicateDetectionHistoryTimeWindow?: string;
  lockDuration?: string;
  maxDeliveryCount?: number;
  name: string;
}

I will leave it as is for now and create the queue manually via the portal to have the properties that i need. If you could investigate into this and see where fixes are required it will be much appreciated

You could use the createQueue API as well which would avoid your problems.

Is it possible to use the Azure Service Bus Queue or Topic also on the client side with React for example ? I am looking for the best practice recommendation. I mainly use it on the backend actually and I have some use cases where the browser clint side would benefit from subcribing to the queue/topic.

If your application is public, I'd recommend you to use the service bus client in a separate server than in the client-side of the react app. Mainly due to the fact that the secrets will be exposed if you use them on the client side in your public app. If you think the secrets such as connection string or AAD credentials will be safe depending on how you use them in the app, you can use the service bus client in your application client-side.

alxizr commented 3 years ago

@HarshaNalluru you are missing what I am saying. please see the code I attached. There is a ternary operator at the beginning that checks if the queue is present or not and acts accordingly.

const queue = (await sbac.queueExists(QUEUE_NAME)) ? await sbac.getQueue(QUEUE_NAME) : await sbac.createQueue(QUEUE_NAME);

My question from the beginwith was that it doesn't matter how I am approaching this issue I cannot set these properties that are immutable to updates, such as enablePartitioning

alxizr commented 3 years ago

If your application is public, I'd recommend you to use the service bus client in a separate server than in the client-side of the react app. Mainly due to the fact that the secrets will be exposed if you use them on the client side in your public app. If you think the secrets such as connection string or AAD credentials will be safe depending on how you use them in the app, you can use the service bus client in your application client-side.

ok thanks

HarshaNalluru commented 3 years ago

@alxizr

you are missing what I am saying. please see the code I attached. There is a ternary operator at the beginning that checks if the queue is present or not and acts accordingly. My question from the beginwith was that it doesn't matter how I am approaching this issue I cannot set these properties that are immutable to updates, such as enablePartitioning

You can set the enablePartitioning property while creating the queue. For that, follow below

await sbac.createQueue(
        queueName,
        {
            enableBatchedOperations: true,
            enablePartitioning: true,
            autoDeleteOnIdle: "P20M", // 20 months
            requiresDuplicateDetection: true,
            maxDeliveryCount: 50,
        }
    );

instead of

await sbac.createQueue(QUEUE_NAME);

when you use the createQueue method to enable partitioning at the time of queue creation.

ghost commented 2 years ago

Hi, we're sending this friendly reminder because we haven't heard back from you in a while. We need more information about this issue to help address it. Please be sure to give us your input within the next 7 days. If we don't hear back from you within 14 days of this comment the issue will be automatically closed. Thank you!

ghost commented 2 years ago

Hi, we're sending this friendly reminder because we haven't heard back from you in a while. We need more information about this issue to help address it. Please be sure to give us your input within the next 7 days. If we don't hear back from you within 14 days of this comment the issue will be automatically closed. Thank you!

ghost commented 2 years ago

Hi, we're sending this friendly reminder because we haven't heard back from you in a while. We need more information about this issue to help address it. Please be sure to give us your input within the next 7 days. If we don't hear back from you within 14 days of this comment the issue will be automatically closed. Thank you!

ramya-rao-a commented 2 years ago

@alxizr Hopefully the comments from @HarshaNalluru above have helped resolve your issue I have made the relevant changes in https://github.com/Azure/azure-sdk-for-js/pull/19081 to avoid the confusion in the docs.

Thanks for reporting!