-
Notifications
You must be signed in to change notification settings - Fork 15
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
(#173) Support for async operations across all repositories. #196
base: main
Are you sure you want to change the base?
Conversation
…accommodate changes
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.
Copilot reviewed 5 out of 14 changed files in this pull request and generated no comments.
Files not reviewed (9)
- src/CommunityToolkit.Datasync.Server/Models/DatasyncServiceOptions.cs: Evaluated as low risk
- src/CommunityToolkit.Datasync.Server.EntityFrameworkCore/BaseEntityTableData.cs: Evaluated as low risk
- tests/CommunityToolkit.Datasync.TestCommon/Databases/CosmosDb/CosmosDbContext.cs: Evaluated as low risk
- tests/CommunityToolkit.Datasync.TestCommon/Databases/Base/BaseDbContext.cs: Evaluated as low risk
- tests/CommunityToolkit.Datasync.Server.EntityFrameworkCore.Test/AzureSqlEntityTableRepository_Tests.cs: Evaluated as low risk
- src/CommunityToolkit.Datasync.Server.EntityFrameworkCore/EntityTableData.cs: Evaluated as low risk
- src/CommunityToolkit.Datasync.Server.EntityFrameworkCore/EntityTableRepository.cs: Evaluated as low risk
- tests/CommunityToolkit.Datasync.TestCommon/RepositoryTests.cs: Evaluated as low risk
- tests/CommunityToolkit.Datasync.Server.Test/Controllers/TableController_Query_Tests.cs: Evaluated as low risk
Comments suppressed due to low confidence (1)
tests/CommunityToolkit.Datasync.Server.EntityFrameworkCore.Test/CosmosEntityTableRepository_Tests.cs:40
- The DisposeAsync method is empty. Consider adding a comment to explain why explicit disposal is not needed or ensure that the context is properly disposed of.
public Task DisposeAsync()
This PR will sit here until Friday before merge to allow folks to review. |
Hi @adrianhall, Looks good I'll pull the branch and give it a whirl :). I suspect for EF9 this will still fail for cosmosdb. The initial code to evaluate the queryOptions will throw because a sync call is forced when PageSize is specified. I've walked back through the QueryOptions.ApplyTo code and this class seems to be the culprit. Then when it falls into the client side evaluation the ToList should be the repositories ToListAsync as the ToList will throw as well. Should the CountAsync be called on the repository as well? Would it be too much to ask for the BuildPagedResult and ExecuteQueryWithClientEvaluationAsync be marked protected? I note your comments regarding changing they Query action functionality to much, but I think by applying them in the piecemeal fashion I suggested would allow the ToListAsync repository function to be typed and limited the result set returned from the datasource. Hope this helps. Cheers Rich |
Good call on the CountAsync - I had moved it but not checked the change in. I'm not seeing an exception when using Cosmos (and there are 31 specific Cosmos tests in the test suite that cover the entire repository). I will see about adding some specific controller tests when there is a cosmos database defined to cover the case you mention later on today. |
I've added a whole bunch of query tests for cosmos to the test suite (coming as soon as I push). Using "DisableClientSideEvaluation" clearly shows the problem. Agreed with you that the TruncatedCollection is the problem here. Since we can't modify either Cosmos or the OData library, I need to think on the appropriate fix here. I suspect removing the paging setting and making PageSize the "Take" at the end would be the appropriate thing to do - will play around a little. |
... Update PageSize can be worked around at the expense of "stable ordering". Not sure why this is the case, but will delve deeper. This only affects the in-memory repository as far as I can tell right now. I may have to add "$orderBy" clauses to some query tests to ensure ordering for comparisons. |
Resolves #173