Skip to content

Support pagination for enumerating deleted items #26974

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

Open
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

amrita-hegde
Copy link

@amrita-hegde amrita-hegde commented Jan 7, 2025

Description

Cosmos customers currently utilize the Azure PowerShell for restoring items. However, a limitation exists in the tool: it lacks the capability to retrieve the items by enumerating them page-wise. This change provides the capability to pass a continuation token to the Get-AzDataLakeStoreDeletedItem cmdlet so that, users can retrieve the next page using the continuation token. Maximum allowed value for this numResults(pagesize) is 4000. The number of returned entries could be more or less than numResults.

###Changes

Azure PowerShell

To enable subsequent pages to be fetched from PowerShell,

Define the optional parameter Continuation Token in Azure PowerShell for GetAzureRmDataLakeStoreDeletedItem command.

Use the continuation token to fetch subsequent pages when calling the API.

.NET SDK

Azure/azure-data-lake-store-net#69
Change is released in https://www.nuget.org/packages/Microsoft.Azure.DataLake.Store/2.0.2

This overloaded method will be called from Azure PowerShell by consuming the latest SDK version 2.0.2.

Testing

Screenshot 2025-04-21 184949

Response from GetAzureRmDataLakeStoreDeletedItem cmd

image

Test execution

Screenshot 2025-03-13 165029

When expired/incorrect continuation token is passed

Screenshot 2025-03-13 114340

Local build

image

Mandatory Checklist

  • SHOULD update ChangeLog.md file(s) appropriately
    • For SDK-based development mode, update src/{{SERVICE}}/{{SERVICE}}/ChangeLog.md.
      • A snippet outlining the change(s) made in the PR should be written under the ## Upcoming Release header in the past tense.
    • For autorest-based development mode, include the changelog in the PR description.
    • Should not change ChangeLog.md if no new release is required, such as fixing test case only.
  • SHOULD regenerate markdown help files if there is cmdlet API change. Instruction
  • SHOULD have proper test coverage for changes in pull request.
  • SHOULD NOT adjust version of module manually in pull request

Copy link

Thanks for your contribution! The pull request validation has started. Please revisit this comment for updated status.

@amrita-hegde
Copy link
Author

amrita-hegde commented Jan 21, 2025 via email

@amrita-hegde amrita-hegde self-assigned this Jan 27, 2025
@amrita-hegde amrita-hegde marked this pull request as ready for review March 17, 2025 05:56
@notyashhh
Copy link
Member

/azp run

Copy link
Contributor

Azure Pipelines successfully started running 3 pipeline(s).

Copy link

This PR was labeled "needs-revision" because it has unresolved review comments or CI failures.
Please resolve all open review comments and make sure all CI checks are green. Refer to our guide to troubleshoot common CI failures.

@amrita-hegde amrita-hegde marked this pull request as draft April 2, 2025 04:28
@amrita-hegde amrita-hegde marked this pull request as ready for review April 21, 2025 17:33
@dolauli
Copy link
Contributor

dolauli commented Apr 22, 2025

/azp run

Copy link
Contributor

Azure Pipelines successfully started running 3 pipeline(s).

@amrita-hegde
Copy link
Author

/azp run

Copy link
Contributor

Commenter does not have sufficient privileges for PR 26974 in repo Azure/azure-powershell

@vidai-msft
Copy link
Contributor

/azp run

Copy link
Contributor

Azure Pipelines successfully started running 3 pipeline(s).

@dolauli dolauli removed this from the Az 13.5.0 (05/06/2025) milestone May 6, 2025
@msJinLei msJinLei changed the base branch from main to release-2025-06-03 May 27, 2025 05:31
Copy link

This PR was labeled "needs-revision" because it has unresolved review comments or CI failures.
Please resolve all open review comments and make sure all CI checks are green. Refer to our guide to troubleshoot common CI failures.

auto-merge was automatically disabled May 28, 2025 05:20

Head branch was pushed to by a user without write access

@vidai-msft
Copy link
Contributor

/azp run

Copy link
Contributor

Azure Pipelines successfully started running 3 pipeline(s).

return new object();
}
)
Account = AzureRmProfileProvider.Instance.Profile.DefaultContext.Account,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have 2 modes: recording and playback. In playback mode, you should use MockCredential. Specifically in you case, you should set AdlsClientFactory.IsTest = true. In pipeline, the test cases are all run in playback mode, which no access to EntraID service.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have updated the code to use MockCredentials for test. Please take a look.

@isra-fel isra-fel force-pushed the release-2025-06-03 branch from 85ea2c5 to ba9dc3f Compare May 29, 2025 03:25
@vidai-msft
Copy link
Contributor

/azp run

Copy link
Contributor

Azure Pipelines successfully started running 3 pipeline(s).

@vidai-msft vidai-msft changed the base branch from release-2025-06-03 to main June 10, 2025 13:19
@vidai-msft
Copy link
Contributor

/azp run

Copy link
Contributor

Azure Pipelines successfully started running 3 pipeline(s).

@vidai-msft
Copy link
Contributor

/azp run

Copy link
Contributor

Azure Pipelines successfully started running 3 pipeline(s).

Copy link
Author

@amrita-hegde amrita-hegde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The CustomDelegatingHAndler is not passed to the AdlsClient. This method is removed from the 2.0.2 SDK. By passing the custom delegating handler, it will inject custom handler into the normal HttpClient. So marked 7 tests as LiveOnly mode since they were trying to call azure environment in playback mode.

Mandatory = false,
ParameterSetName = DefaultParameterSet,
HelpMessage = "Token returned by system in the previous invocation")]
public string listAfter { get; set; }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per .NET and PowerShell naming convention, the parameter name should be ListAfter (capital 'L')

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

WriteObject(toReturn, enumerateCollection: true);

// Write the continuation token.
WriteObject($"Continuation Token: {continuationToken}");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The WriteObject is kind of special in PowerShell SDK. It is used to output the returned object instead of a result message. Any reason why the token should be piped out?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Continuation token will be the input to the next request. So user has to know the continuation token to fetch the next page.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WriteObject will output multiple objects with different types which is not recommended. It may break the existing automation script and looks like a breaking change. Could you please write a test case to verify? Still I don't quite understand why the token is necessary here. Could it be wrapped into the object model?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We were using WriteObject to output the trash entries earlier. Only the continuation token is the addition here. The whole purpose of this change is to give back a continuation token to the user so that they can pass it in the next command to get the subsequent pages of the deleted item entries. If it has to be wrapped into an object, it needs multiple change in dotnet sdk.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the comment of @vidai-msft does make sense. WriteObject 2 times should not supported in Azure PowerShell. Could you prepare a cmdlet test example with screenshots to describes your case?

@@ -47,17 +49,17 @@ internal static AdlsClient GetAdlsClient(string accntNm, IAzureContext context)
AdlsClient client = null;
if (IsTest)
{
client = AdlsClient.CreateClient(accntNm, MockCredentials, DataLakeStoreFileSystemClient.ImportExportMaxThreads, CustomDelegatingHAndler);
client = AdlsClient.CreateClient(accntNm, MockCredentials, new string[] { });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The IsTest looks like has no use here. Can you please refactor the code here to avoid any confuse in the future?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

{
AdlsClientFactory.IsTest = true;

var serviceClientCredentials = context.GetClientCredentials("https://datalake.azure.net");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a class to predefine all the well-known URLs. datalake.azure.net could be referenced by AzureEnvironment.Endpoint.DataLakeEndpointResourceId. Can consider replacing with this variable.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class is not needed anymore. removed

null,
ShowDialog.Never,
null,
"https://datalake.azure.net/");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better use AzureEnvironment.Endpoint.DataLakeEndpointResourceId to reference the endpoint here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -14,8 +14,8 @@ Searches for deleted entries in trash which match the filter.
## SYNTAX

```
Get-AzDataLakeStoreDeletedItem [-Account] <String> [-Filter] <String> [-Count <Int32>] [-AsJob]
[-DefaultProfile <IAzureContextContainer>] [<CommonParameters>]
Get-AzDataLakeStoreDeletedItem [-Account] <String> [-Filter] <String> [-Count <Int32>] [-listAfter <String>]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update the listAfter to ListAfter and re-generate the help docs.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@vidai-msft
Copy link
Contributor

/azp run

Copy link
Contributor

Azure Pipelines successfully started running 3 pipeline(s).

@@ -70,7 +70,7 @@ public void TestAdlsTrustedIdProvider()
}

[Fact]
[Trait(Category.AcceptanceType, Category.CheckIn)]
[Trait(Category.AcceptanceType, Category.LiveOnly)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason to remove the test cases?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#26974 (review) As mentioned here, passing the CustomDelegatingHAndler to adls client is not supported in the new .net sdk. Discussed this with @vidai-msft and marked the testcases as LiveOnly.

WriteObject(toReturn, enumerateCollection: true);

// Write the continuation token.
WriteObject($"Continuation Token: {continuationToken}");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the comment of @vidai-msft does make sense. WriteObject 2 times should not supported in Azure PowerShell. Could you prepare a cmdlet test example with screenshots to describes your case?

@YanaXu YanaXu added the Breaking Change Release This PR contains breaking change label Jun 25, 2025
@YanaXu YanaXu assigned YanaXu and unassigned msJinLei Jun 25, 2025
Copy link

To the author of the pull request,
This PR was labeled "Breaking Change Release" because it contains breaking changes.

  • According to our policy, breaking changes can only take place during major release and they must be preannounced.
  • Please follow our guide on the detailed steps.
  • Required: Please fill in the task below to facilitate our contact,you will receive notifications related to breaking changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking Change Release This PR contains breaking change needs-revision
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants