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

Async support for ToDynamicList() #96

Closed
borisdj opened this issue Jul 20, 2017 · 8 comments
Closed

Async support for ToDynamicList() #96

borisdj opened this issue Jul 20, 2017 · 8 comments
Assignees
Labels

Comments

@borisdj
Copy link

borisdj commented Jul 20, 2017

After dynamic select when enumerating I there is ToDynamicList() method but no async version of the same ToDynamicListAsync() that would propagate async call till bottom.
Could this feature be added, because I am using EFCore(1.1.2) and have made most methods in Controller and Repository to be async for efficiency.

@StefH StefH self-assigned this Jul 20, 2017
@StefH StefH added the feature label Jul 20, 2017
@StefH
Copy link
Collaborator

StefH commented Jul 21, 2017

Can you provide a code-example how you intend to use this ?

@borisdj
Copy link
Author

borisdj commented Jul 21, 2017

For example in Controller (Generic one) there is regular Get method and it is Async:

public virtual async Task<IActionResult> Get(string args)
{
    var q = repository.Query();
    // some more code for paging and filtering
    // ...
    return Ok(await q.ToListAsync());

Then in the same Controller I also have action Lookup method like this:

[HttpGet("Lookup")]
public IActionResult Lookup(string search)
{
    var q = repository.Query();
    string idProperty = typeof(Entity).Name + "Id";
    string textProperty = GetLookupPropertyName();
    string clause = $"{textProperty}.Contains(@0)";
    if (!String.IsNullOrWhiteSpace(search))
    {
        q = q.Where(clause, search);
    }
    q = q.OrderBy(textProperty);
    q = q.Take(100);
    
    var result = q.Select($"new ({idProperty} as id, {textProperty} as text)");
    return Ok(result.ToDynamicList());
}

So I was considering making that one also async but that would require ToDynamicListAsync();

@StefH
Copy link
Collaborator

StefH commented Jul 21, 2017

OK, I will take a look.

StefH added a commit that referenced this issue Jul 21, 2017
@StefH
Copy link
Collaborator

StefH commented Jul 21, 2017

Some notes:

1] Your example code allows sql-injection (you provide the search parameter external input)

2] The extension methods I've created (see point 3) are fake, they are not really async.

3] Code be found here : ed57240#diff-94779ab3305c4d04396ee3e68380908c
if this is ook, I create a new Nuget.

@borisdj
Copy link
Author

borisdj commented Jul 21, 2017

  1. Shouldn't the @(placeholder) be made into lambda argument(=>) instead of concatenating, that makes it safe from "Linq injection" attack.
    If not, what would be best way to escape the string?

  2. I was thinking more of real async, fake will have the same efficiency so no gain there.
    I haven't yet look deep into source of the library, so I'm not sure how you implemented this under the hood and if it even possible to make full async or if it is too complex.

@StefH
Copy link
Collaborator

StefH commented Jul 21, 2017

1] It's not a real sql-injection, but you expose the search criteria to the outside world, which means that you can enter whatever you like and that will be executed.

2] This file https://github.com/StefH/System.Linq.Dynamic.Core/blob/master/src/Microsoft.EntityFrameworkCore.DynamicLinq/EFDynamicQueryableExtensions.cs defines some methods which are really ASYNC.

@borisdj
Copy link
Author

borisdj commented Jul 21, 2017

  1. Search has to be exposed to client, how else could lookup work?
    Is there some better way to achieve the same ?

  2. I'll take a look into Asynchronous implementation.

StefH added a commit that referenced this issue Aug 1, 2017
* DynamicEnumerableAsyncExtensions (#96)

* Fix for ToDynamicArrayAsync<T> method
@StefH
Copy link
Collaborator

StefH commented Oct 3, 2017

If you need some more help for search, open a new issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

2 participants