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

Improve IAsyncEnumerable usage #4486

Merged
merged 20 commits into from May 14, 2024
Merged

Improve IAsyncEnumerable usage #4486

merged 20 commits into from May 14, 2024

Conversation

viceroypenguin
Copy link
Contributor

@viceroypenguin viceroypenguin commented Apr 21, 2024

This PR does two things:

  • Updates InsertWithOutputAsync, UpdateWithOutputAsync, and DeleteWithOutputAsync to return IAsyncEnumerable properly. This is a binary-compatible change, as the existing methods are kept. However, these methods are now marked with [Obsolete] to encourage source migration to the newer IAsyncEnumerable method.
  • Adds QueryToAsyncEnumerable in places where QueryToArrayAsync and QueryToListAsync are provided. This allows consumers to use IAsyncEnumerable if desired. No obsoletions are planned here, though consumers are allowed and encouraged to upgrade if the query has many rows.

💣 Breaking Change

  • The new versions of XxxWithOutputAsync are not source compatible, so this is a v6.0 breaking change. Consumers will need to add .ToListAsync() or similar to address this change. This is to restore consistency between InsertWithOutputAsync/UpdateWithOutputAsync/DeleteWithOutputAsync with MergeWithOutputAsync; this consistency has been chosen to use the more flexible and modern approach.

Replaces #4323

@viceroypenguin
Copy link
Contributor Author

/azp run test-all

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Shane32
Copy link
Contributor

Shane32 commented Apr 21, 2024

Just for clarity when reviewing this PR, I'd like to point out the difference between the old and new signatures:

// old
public static Task<TOutput[]> DeleteWithOutputAsync<TSource,TOutput>(
    this IQueryable<TSource>           source,
    Expression<Func<TSource, TOutput>> outputExpression,
    CancellationToken                  token = default)

// new methods
public static IAsyncEnumerable<TOutput> DeleteWithOutputAsync<TSource,TOutput>(
    this IQueryable<TSource>           source,
    Expression<Func<TSource, TOutput>> outputExpression)

public static Task<TOutput[]> DeleteWithOutputAsync<TSource, TOutput>(
    this IQueryable<TSource> source,
    Expression<Func<TSource, TOutput>> outputExpression,
    CancellationToken token)

This means that the following code will immediately break compilation for v6 due to the cancellation token not being specified:

// old syntax; immediately broken with v6
var ret = await query.DeleteWithOutputAsync(x => x.Id);

// required fix to use obsolete method
var ret = await query.DeleteWithOutputAsync(x => x.Id, default);

As specified, however, the change is binary compatible - already-compiled code will continue to function with v6, as the optional parameter is built into the caller's compiled code and so will match on the obsolete method.

UPDATE: this is inaccurate because this was removed from the obsolete method. Users will be required to rewrite usage of these methods with the new syntax.

@viceroypenguin
Copy link
Contributor Author

/azp run test-all

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@linq2dbot
Copy link

Test baselines changed by this PR. Don't forget to merge/close baselines PR after this pr merged/closed.

@sdanyliv
Copy link
Member

@viceroypenguin , check mergeability with this PR #3401

@viceroypenguin
Copy link
Contributor Author

@viceroypenguin , check mergeability with this PR #3401

@sdanyliv no merge conflicts; and shouldn't have any impact on #3401.

sdanyliv
sdanyliv previously approved these changes Apr 23, 2024
@MaceWindu MaceWindu modified the milestones: 6.0.0, 6.0.0-preview.1 Apr 24, 2024
igor-tkachev
igor-tkachev previously approved these changes Apr 24, 2024
/// <param name="sql">Command text.</param>
/// <param name="parameter">Command parameter.</param>
/// <returns>Async sequence of records returned by the query.</returns>
public static IAsyncEnumerable<T> QueryToAsyncEnumerable<T>(this DataConnection connection, string sql, DataParameter parameter)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why we have this overload for linq2db-mapped method?

/// <param name="sql">Command text.</param>
/// <param name="parameters">Command parameters.</param>
/// <returns>Async sequence of records returned by the query.</returns>
public static IAsyncEnumerable<T> QueryToAsyncEnumerable<T>(this DataConnection connection, T template, string sql, params DataParameter[] parameters)
Copy link
Contributor

Choose a reason for hiding this comment

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

and why for template-based specification it is only 2 overloads?

Copy link
Contributor

@MaceWindu MaceWindu left a comment

Choose a reason for hiding this comment

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

minor updates needed

@viceroypenguin viceroypenguin dismissed stale reviews from igor-tkachev and sdanyliv via 934ef8e May 13, 2024 22:39
@MaceWindu MaceWindu merged commit df98940 into version_6 May 14, 2024
@MaceWindu MaceWindu deleted the features/async-enumerable branch May 14, 2024 07:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants