Skip to content
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

Utilize AsyncPageable for query return types #3165

Merged
merged 20 commits into from Mar 21, 2023

Conversation

timtay-microsoft
Copy link
Member

@timtay-microsoft timtay-microsoft commented Mar 14, 2023

The new APIs look like:

public virtual AsyncPageable<T> CreateAsync<T>(string query, CancellationToken cancellationToken = default)

A couple points worth discussing:

  • Unlike in any of our other APIs, the AsyncPageable APIs contain the Response type that allows users to see the http request contents.
    • This is generally a good thing to let users see, but it may beg the question as to why the other APIs don't allow this same thing.
  • Some code (PageableHelpers) has to be copied from Azure.Core since constructing an AsyncPageable instance is non-trivial
    • The public facing return types are the Azure.Core defined classes, though.
  • The page size and continuation token can be set through the AsyncPageable itself, so we can remove the QueryOptions class that we had that served the same purpose.
    • The only real loss here is that users cannot set page size or continuation token unless they are iterating page by page. Users iterating item by item won't actually be capable of getting the continuation token, but would they still care about page size?
// Old pagination configuration using our defined QueryOptions
var options = new QueryOptions()
{
    ContinuationToken = "someToken",
    PageSize = 4
};
AsyncPageable<ClientTwin> twinQuery = serviceClient.Query.CreateAsync("some twin query", queryOptions);
await foreach (Page<ClientTwin> queriedTwinPage in twinQuery.AsPages())
{
    // some application logic
}
// Pagination configuration using AsyncEnumerable methods
int pageSize = 5;
AsyncPageable<ClientTwin> twinQuery = serviceClient.Query.CreateAsync("some twin query");
await foreach (Page<ClientTwin> queriedTwinPage in twinQuery.AsPages("continuationToken", pageSize))
{
    // some application logic
}

Once this PR is merged in, I'll apply the same changes to the DPS service client since it also has a query API

This change requires we copy some internal code from the Azure.Core library, but the public facing classes are the Azure.Core defined classes.
/// this type should be type <see cref="ClientTwin"/>. When using a query such as "SELECT * FROM devices.jobs",
/// this type should be type <see cref="ScheduledJob"/>.
/// </typeparam>
public class QueryResponse<T>
Copy link
Member Author

Choose a reason for hiding this comment

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

This class used to be the "AsyncPageable" implementation we had locally, but now it is the "Response" implementation since the Azure.Core AsyncPageable pages all contain the Response type embedded in them

Copy link
Contributor

@drwill-ms drwill-ms left a comment

Choose a reason for hiding this comment

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

Needs an update to migration guide too.

Co-authored-by: David R. Williamson <drwill@microsoft.com>
Copy link
Contributor

@drwill-ms drwill-ms left a comment

Choose a reason for hiding this comment

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

Still needs an update to migration guide.

@timtay-microsoft
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@timtay-microsoft
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@timtay-microsoft
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@timtay-microsoft
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@timtay-microsoft
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@timtay-microsoft
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@timtay-microsoft
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@timtay-microsoft timtay-microsoft merged commit 0501363 into previews/v2 Mar 21, 2023
9 of 11 checks passed
@timtay-microsoft timtay-microsoft deleted the timtay/AsyncPageable branch March 21, 2023 17:35
timtay-microsoft added a commit that referenced this pull request Mar 23, 2023
see #3165, but for the DPS service client this time.

This PR also removes some superfluous classes like ```ContractApiResponse``` since they were needless abstractions that made it harder to implement this change

I also change all the ```CreateAsync``` Query APIs to just ```Create``` since the function itself is not async. The first service request isn't made until the iteration begins.

I also removed the ```IContractApiHttp``` interface since it only had one implementation and we don't currently allow users to override this implementation in any way.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants