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.

Sign up for GitHub

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

Jump to bottom

ClientModel: Add async and sync enumerable client results #43392

Closed
wants to merge 7 commits into from
Closed

ClientModel: Add async and sync enumerable client results #43392

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
54f822b
Add async and sync enumerable client results
annelo-msft Apr 12, 2024
d239e17
Implement IAsyncDisposable
annelo-msft Apr 15, 2024
115295a
Merge remote-tracking branch 'upstream/main' into clientmodel-asyncen…
annelo-msft Apr 19, 2024
3b12f1f
Remove constrant and disposable; update ClientResult so response can …
annelo-msft Apr 19, 2024
dbf18ad
rename and upate tests
annelo-msft Apr 20, 2024
a798fd9
Merge remote-tracking branch 'upstream/main' into clientmodel-asyncen…
annelo-msft May 1, 2024
72951b9
Merge remote-tracking branch 'upstream/main' into clientmodel-asyncen…
annelo-msft May 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions sdk/core/System.ClientModel/api/System.ClientModel.net6.0.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,14 @@ public ApiKeyCredential(string key) { }
public static implicit operator System.ClientModel.ApiKeyCredential (string key) { throw null; }
public void Update(string key) { }
}
public abstract partial class AsyncEnumerableResult<T> : System.ClientModel.ClientResult, System.Collections.Generic.IAsyncEnumerable<T>, System.IAsyncDisposable where T : System.ClientModel.Primitives.IPersistableModel<T>
{
protected internal AsyncEnumerableResult(System.ClientModel.Primitives.PipelineResponse response) : base (default(System.ClientModel.Primitives.PipelineResponse)) { }
protected abstract void Dispose(bool disposing);
public System.Threading.Tasks.ValueTask DisposeAsync() { throw null; }
protected abstract System.Threading.Tasks.ValueTask DisposeAsyncCore();
public abstract System.Collections.Generic.IAsyncEnumerator<T> GetAsyncEnumerator(System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken));
}
public abstract partial class BinaryContent : System.IDisposable
{
protected BinaryContent() { }
Expand Down Expand Up @@ -40,6 +48,14 @@ protected internal ClientResult(T value, System.ClientModel.Primitives.PipelineR
public virtual T Value { get { throw null; } }
public static implicit operator T (System.ClientModel.ClientResult<T> result) { throw null; }
}
public abstract partial class EnumerableClientResult<T> : System.ClientModel.ClientResult, System.Collections.Generic.IEnumerable<T>, System.Collections.IEnumerable, System.IDisposable where T : System.ClientModel.Primitives.IPersistableModel<T>
Copy link
Member

Choose a reason for hiding this comment

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

The names don't seem symetrical. What about the following:

AsyncEnumerableResult
EnumerableResult
//or
ClientResults
ClientResultsAsync

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm actually not wild about ClientResults because I think it complicates what we're defining a "result" to be (although I am open to debate here).

To take a concrete example, Pageable<T> is a collection of T's. Does the "result" mean the T or with the response? In a simple ClientResult<T> case there's a 1-1 relationship between them. But ClientResult itself just exposes GetRawResponse so it seems like result maps more closely to the response than the T. Given that, it feels strange to me to make the "result" plural when the T is the plural thing and there might only ever be one response.

Let me think.

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like 3.5.2 in the FDG recommends using Collection in the name of types that derive from IEnumerable<T> ... what about ClientResultCollection<T> and AsyncClientResultCollection<T>? Those are getting long, though, and don't really address my question about whether it is a collection of results, but I think I'm willing to drop argument since we defined ClientResult in the refdocs as "the result of a cloud service operation." A sequence of such results could easily be a result collection. Is it too long?

{
protected internal EnumerableClientResult(System.ClientModel.Primitives.PipelineResponse response) : base (default(System.ClientModel.Primitives.PipelineResponse)) { }
public void Dispose() { }
protected abstract void Dispose(bool disposing);
public abstract System.Collections.Generic.IEnumerator<T> GetEnumerator();
System.Collections.IEnumerator System.Collections.IEnumerable.GetEnumerator() { throw null; }
}
}
namespace System.ClientModel.Primitives
{
Expand Down
16 changes: 16 additions & 0 deletions sdk/core/System.ClientModel/api/System.ClientModel.netstandard2.0.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,14 @@ public ApiKeyCredential(string key) { }
public static implicit operator System.ClientModel.ApiKeyCredential (string key) { throw null; }
public void Update(string key) { }
}
public abstract partial class AsyncEnumerableResult<T> : System.ClientModel.ClientResult, System.Collections.Generic.IAsyncEnumerable<T>, System.IAsyncDisposable where T : System.ClientModel.Primitives.IPersistableModel<T>
{
protected internal AsyncEnumerableResult(System.ClientModel.Primitives.PipelineResponse response) : base (default(System.ClientModel.Primitives.PipelineResponse)) { }
protected abstract void Dispose(bool disposing);
public System.Threading.Tasks.ValueTask DisposeAsync() { throw null; }
protected abstract System.Threading.Tasks.ValueTask DisposeAsyncCore();
public abstract System.Collections.Generic.IAsyncEnumerator<T> GetAsyncEnumerator(System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken));
}
public abstract partial class BinaryContent : System.IDisposable
{
protected BinaryContent() { }
Expand Down Expand Up @@ -40,6 +48,14 @@ protected internal ClientResult(T value, System.ClientModel.Primitives.PipelineR
public virtual T Value { get { throw null; } }
public static implicit operator T (System.ClientModel.ClientResult<T> result) { throw null; }
}
public abstract partial class EnumerableClientResult<T> : System.ClientModel.ClientResult, System.Collections.Generic.IEnumerable<T>, System.Collections.IEnumerable, System.IDisposable where T : System.ClientModel.Primitives.IPersistableModel<T>
{
protected internal EnumerableClientResult(System.ClientModel.Primitives.PipelineResponse response) : base (default(System.ClientModel.Primitives.PipelineResponse)) { }
public void Dispose() { }
protected abstract void Dispose(bool disposing);
public abstract System.Collections.Generic.IEnumerator<T> GetEnumerator();
System.Collections.IEnumerator System.Collections.IEnumerable.GetEnumerator() { throw null; }
}
}
namespace System.ClientModel.Primitives
{
Expand Down
37 changes: 37 additions & 0 deletions sdk/core/System.ClientModel/src/Convenience/AsyncEnumerableResultOfT.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

using System.ClientModel.Primitives;
using System.Collections.Generic;
using System.Threading;
using System.Threading.Tasks;

namespace System.ClientModel;

#pragma warning disable CS1591 // public XML comments
public abstract class AsyncEnumerableResult<T> : ClientResult, IAsyncEnumerable<T>, IAsyncDisposable
where T : IPersistableModel<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.

Another question is whether or not we want to add this type constraint. The benefit is that it could simplify implementations by allowing them to use ModelReaderWriter to deserialize values coming across the wire, rather than requiring them to use other ways to pass in bespoke deserialization routines. A drawback is that it limits the use of the type to models that implement the interface -- if we wanted to enable scenarios like pages of primitives, that could be a problem.

Copy link
Member

Choose a reason for hiding this comment

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

Since we did not constrain ClientResult<T>, maybe we should keep not constraining. Though in retrospect, maybe we should have.

Copy link
Member Author

Choose a reason for hiding this comment

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

The libraries haven't GA'ed yet, so we could still constrain it if we feel strongly about it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, but not if we want Azure.Core Pageable<T> to be able to inherit from it. Alas! I will remove it.

{
protected internal AsyncEnumerableResult(PipelineResponse response) : base(response)
Copy link
Member

Choose a reason for hiding this comment

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

can the response be updated/changed?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think it can. Is there a scenario where we would want that?
(Not sure I understand the implications of your question.)

Copy link
Member

Choose a reason for hiding this comment

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

The scenario is using this type to expose pageable REST APIs. In such scenarios there are multiple responses representing all items in the collection/pageable

Copy link
Member Author

Choose a reason for hiding this comment

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

Would you want to do that by changing the value returned from the response? Why not return a collection of PipelineResponses instead? (i.e. not inherit from ClientResult)

Copy link
Member Author

Choose a reason for hiding this comment

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

Addressed. I see that we already do that in Azure.Core Operation implementations such as KeyVaultBackupOperation and FuzzySearchBatchOperation.

{
}

public abstract IAsyncEnumerator<T> GetAsyncEnumerator(CancellationToken cancellationToken = default);

protected abstract ValueTask DisposeAsyncCore();
Copy link
Member

Choose a reason for hiding this comment

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

FDGs say to make it virtual

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed per discussion about moving disposal of resources into the enumerator.


protected abstract void Dispose(bool disposing);
Copy link
Member

Choose a reason for hiding this comment

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

what is this for? i.e. who call it? BTW, FDGs (that Jeremy wrote) say to implement both IAsyncDisposabel and IDisposable

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed per discussion about moving disposal of resources into the enumerator.


public async ValueTask DisposeAsync()
{
await DisposeAsyncCore().ConfigureAwait(false);

// Dispose of unmanaged resources. Note that DisposeAsyncCore disposes
// of managed resources asychronously -- we pass false to Dispose so we
// don't attempt to dispose of them synchronously as well.
Dispose(disposing: false);
jsquire marked this conversation as resolved.
Show resolved Hide resolved

GC.SuppressFinalize(this);
}
}
#pragma warning restore CS1591 // public XML comments
30 changes: 30 additions & 0 deletions sdk/core/System.ClientModel/src/Convenience/EnumerableClientResultOfT.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

using System.ClientModel.Primitives;
using System.Collections;
using System.Collections.Generic;

namespace System.ClientModel;

#pragma warning disable CS1591 // public XML comments
public abstract class EnumerableClientResult<T> : ClientResult, IEnumerable<T>, IDisposable
where T : IPersistableModel<T>
{
protected internal EnumerableClientResult(PipelineResponse response) : base(response)
{
}

public abstract IEnumerator<T> GetEnumerator();

IEnumerator IEnumerable.GetEnumerator() => GetEnumerator();

public void Dispose()
{
Dispose(disposing: true);
GC.SuppressFinalize(this);
}

protected abstract void Dispose(bool disposing);
}
#pragma warning restore CS1591 // public XML comments