-
Notifications
You must be signed in to change notification settings - Fork 155
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
Async query enumeration and cancellation support for long running queries #423
Conversation
@KenitoInc Is there anything I can to to speed up the review? |
@@ -115,7 +116,7 @@ await WriteObjectInlineAsync(graph, resourceSetType, writer, writeContext) | |||
IEdmStructuredTypeReference elementType = GetResourceType(resourceSetType); | |||
ODataResourceSet resourceSet = CreateResourceSet(enumerable, resourceSetType.AsCollection(), writeContext); | |||
|
|||
Func<object, Uri> nextLinkGenerator = GetNextLinkGenerator(resourceSet, enumerable, writeContext); | |||
Func<Func<object, Uri>> nextLinkGenerator = () => GetNextLinkGenerator(resourceSet, enumerable, writeContext); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not follow the need for the extra Func
. Can you help me understand?
resourceSet.NextPageLink = originalNexPageLink; | ||
} | ||
|
||
resourceSet.NextPageLink = nextLinkGenerator()(lastResource); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be related to my other question about nextLinkGenerator
, but doesn't calling this after setting the original mean that a new link is not generated?
@@ -25,5 +26,8 @@ public interface ITruncatedCollection : IEnumerable | |||
/// Gets a value representing if the collection is truncated or not. | |||
/// </summary> | |||
bool IsTruncated { get; } | |||
|
|||
bool IsAsyncEnumerationPossible { get; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that these will be breaking changes for any current implementers of this interface
|
||
namespace Microsoft.AspNetCore.OData.Query.Container | ||
{ | ||
/// <summary> | ||
/// Represents a class that truncates a collection to a given page size. | ||
/// </summary> | ||
/// <typeparam name="T">The collection element type.</typeparam> | ||
public class TruncatedCollection<T> : List<T>, ITruncatedCollection, IEnumerable<T>, ICountOptionCollection |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Despite that I agree we should remove List<T>
, this is certainly a breaking change.
// the enumerable version just enumerates and is inefficient. | ||
public TruncatedCollection(IQueryable<T> source, int pageSize, long? totalCount, bool parameterize) | ||
: base(pageSize > 0 ? Take(source, pageSize, parameterize) : source) | ||
public bool IsAsyncEnumerationPossible => _items is IAsyncEnumerable<T>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is trying to follow the same pattern as truncation? I don't think I agree with either approach, and I'm not sure we should be adding more precedence for this type of pattern. I probably have more feedback about this class, but I think getting the design correct first is important.
{ | ||
MethodInfo genericMethod = _limitResultsGenericMethod.MakeGenericMethod(queryable.ElementType); | ||
object[] args = new object[] { queryable, limit, parameterize, null }; | ||
IQueryable results = genericMethod.Invoke(null, args) as IQueryable; | ||
resultsLimited = (bool)args[3]; | ||
resultsLimited = (Func<bool>)args[3]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't get it. parameterize
is not a reference or anything, it always has whatever value was passed into LimitResults
, doesn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logic bubbles up I think all the way up to the change of pageSize
to a delegate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, there seems to be a lack of test coverage in general, or there is something critical that I've missed about why these delegates exist, because (Func<bool>)((object)false)
I believe will throw InvalidCastException
@@ -134,6 +134,8 @@ public void CreatePropertyContainer_PageSize() | |||
PropertyContainer container = ToContainer(containerExpression); | |||
var result = container.ToDictionary(new IdentityPropertyMapper())["PropertyName"]; | |||
var truncatedCollection = Assert.IsType<TruncatedCollection<int>>(result); | |||
// Enumerate the list so that the PageSize gets initialized. | |||
truncatedCollection.ToList(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if we don't enumerate the sequence? Requiring enumeration to get PageSize
is a breaking change, I think
using Xunit; | ||
|
||
namespace Microsoft.AspNetCore.OData.Tests.Query.Container | ||
{ | ||
public class TruncatedCollectionTest | ||
{ | ||
[Fact] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand correctly, these tests are being removed because the constructors were removed. TruncatedCollection
is public
, we can't just remove public constructors
@@ -794,12 +794,14 @@ public void LimitResults_LimitsResults(int limit, bool resultsLimitedExpected) | |||
ODataQueryContext context = new ODataQueryContext(model, typeof(Customer)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't actually see any tests for the new functionality, can you please add them?
Thanks for the review @corranrogue9. I'm a little swamped right now but will get back to this ASAP. |
@henning-krause hoping you'd be able to complete this sometime! Would be cool if cancellation token is going to be supported. |
@henning-krause any chance? |
@SymbioticKilla Unfortunately, I won't have time for this in the foreseeable future. |
Hi, could you consider to push this PR info the next major version that is coming? Thanks |
I don't think we can merge this into main. A lot has changes since I opened this PR. Apart from this, it should be much easier now to come up with a new PR, sind IAsyncEnumerable is now supported. |
Note: I've reopened this PR since the target branch name changes to main. The original PR can be found here: #324 and was already approved by @corranrogue9.
A long time ago I created an issue to properly cancel long running queries (when the HttpContext.RequestAborted cancellation token is triggered) for the 7.9 version of this code. This was never solved, I think mainly because most of the code is still synchronous.
Now that I moved on to AspNetCoreOData I'm facing the same problem and gave it another shot.
@xuzhg I know that this PR is far from complete. But I wanted to get an opinion from you if I'm moving in the right direction with this and if you would approve this PR once it's complete before I put more effort in this.
For now, I have mainly redesigned the TruncatedCollection to properly support async enumeration (and remove the buffering of the whole response along with this change) as well as the ODataResourceSetSerializer.
So, for paged queries, this approach works.