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

Add query root rewrite support #86

Merged
merged 7 commits into from
Oct 24, 2023
Merged

Add query root rewrite support #86

merged 7 commits into from
Oct 24, 2023

Conversation

zoriya
Copy link
Contributor

@zoriya zoriya commented Sep 24, 2023

First basic attempt to support Query Root rewriting to support things like #84.

For now, It only supports rewriting for properties that have a setter (since the data can't be stored after a select otherwise) and a default constructor.

There is no cache support for now.

@zoriya
Copy link
Contributor Author

zoriya commented Sep 24, 2023

After testing a little, this works well when there is a no complex select or other things filters using relations.
For example:

DbSet<User>().FirstOrDefault()

gets rewritten to:

dbContext.Users
          .Select(namelessParameter{0} => new User{ 
              <Id>k__BackingField = namelessParameter{0}.<Id>k__BackingField, 
              <FirstName>k__BackingField = namelessParameter{0}.<FirstName>k__BackingField, 
              <LastName>k__BackingField = namelessParameter{0}.<LastName>k__BackingField, 
              TotalSpent = namelessParameter{0}.Orders
                  .Sum(x => x.Price) 
          }
          )
          .FirstOrDefault()'

but something like:

dbContext.Users
    .Select(x => new {
        Name = x.FullName,
        x.TotalSpent
    })
	.FirstOrDefault();

gets rewritten to:

DbSet<User>()
          .Select(namelessParameter{0} => new User{ 
              <Id>k__BackingField = namelessParameter{0}.<Id>k__BackingField, 
              <FirstName>k__BackingField = namelessParameter{0}.<FirstName>k__BackingField, 
              <LastName>k__BackingField = namelessParameter{0}.<LastName>k__BackingField, 
              TotalSpent = namelessParameter{0}.Orders
                  .Sum(x => x.Price) 
          }
          )
          .Select(x => new { 
              Name = x.FullName, 
              TotalSpent = x.Orders
                  .Sum(x => x.Price)
           })
          .FirstOrDefault()'

so this fails because the second select references Order which is not loaded by EF.

I wonder if there is a way to only rewrite the QueryRootExpression if there is no expression following it.

@koenbeuk
Copy link
Owner

koenbeuk commented Sep 24, 2023

Thanks for taking a swing at this!

I wonder if there is a way to only rewrite the QueryRootExpression if there is no expression following it.

I think that's exactly what we will want to achieve for the exact problem that you described.

One way to approach this issue is by applying QueryRootExpression rewrite logic that you implemented here after applying all other rewrite rules (effectively after calling: ProjectableExpressionReplacer.Visit or at least at the end of the implementation there).

So that:

dbContext.Users
    .Where(x => x.TotalSpend > 100)
    .FirstOrDefault();

First gets rewritten as:

dbContext.Users
    .Where(x => x.Orders.Sum(x => x.Price) > 100) // Note: This logic would be client-evaluated with the current state of this PR
    .FirstOrDefault();

And then rewritten as:

dbContext.Users
    .Where(x => x.Orders.Sum(x => x.Price) > 100) 
    .Select(namelessParameter{0} => new User{ 
              <Id>k__BackingField = namelessParameter{0}.<Id>k__BackingField, 
              <FirstName>k__BackingField = namelessParameter{0}.<FirstName>k__BackingField, 
              <LastName>k__BackingField = namelessParameter{0}.<LastName>k__BackingField, 
              TotalSpent = namelessParameter{0}.Orders
                  .Sum(x => x.Price) 
          }
          )
          .FirstOrDefault()'

@zoriya Edit: left a proposal

@zoriya
Copy link
Contributor Author

zoriya commented Sep 25, 2023

I like the idea of running a second path for QueryRoot rewriting, but I think it will also need to stop rewriting if the query contains a Select statement, or it will probably make an error. I'm thinking about cases like:

int id = await db.Users
    .Select(x => x.Id)
    // This generated select should not be there.
    .Select(namelessParameter{0} => new User{ 
        <Id>k__BackingField = namelessParameter{0}.<Id>k__BackingField, 
        <FirstName>k__BackingField = namelessParameter{0}.<FirstName>k__BackingField, 
        <LastName>k__BackingField = namelessParameter{0}.<LastName>k__BackingField, 
        TotalSpent = namelessParameter{0}.Orders
            .Sum(x => x.Price) 
    })
    .First()

@zoriya
Copy link
Contributor Author

zoriya commented Oct 10, 2023

The previous cases now work perfectly. The .Select is added at the very end of the query, just before a .First()/.ToList() or a the very end when there is no call to those methods. It also handles only the query root or .Sum() and calls where the rewrite should not happen.

I'm going to do more testing in the week to see if I can find cases where it breaks, please comment if you think of one.

@zoriya
Copy link
Contributor Author

zoriya commented Oct 10, 2023

Since this PR is way more useful when there is a baking field for a projectable property like:

[Projectable(UseMemberBody = nameof(_FullName))]
public string FullName { get; set; }
private string _FullName => FirstName + " " + LastName;

I think it would be good to have a way to automatically generate the baking field, allowing to write like below and get the same behavior as above:

[Projectable(WithBaking = true)]
public string FullName => FirstName + " " + LastName;

I have absolutely no idea how the generator works, so I have not really looked to do that for now.

@zoriya zoriya marked this pull request as ready for review October 13, 2023 22:28
@zoriya
Copy link
Contributor Author

zoriya commented Oct 13, 2023

I tested it on my project, and it works exactly as expected! Since the API for QueryRootExpression changed when dotnet7 was released, it needs to have different behaving between the two versions, so I added that on the last commit.

I reused the caching system that already existed for properties, but I did not cache the new Select statement since it would require caching the whole query and I don't think that it's a good tradeoff.

I did not include the helper API I proposed in my last comment as I want this PR to be small, I might make a follow-up PR for it if I have the courage to learn how the generator works.

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.

2 participants