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

(#173) Support for async operations across all repositories. #196

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

adrianhall
Copy link
Collaborator

Resolves #173

@adrianhall adrianhall added this to the 9.0.0 milestone Jan 13, 2025
@adrianhall adrianhall requested a review from Copilot January 13, 2025 23:49
@adrianhall adrianhall self-assigned this Jan 13, 2025

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()
@adrianhall
Copy link
Collaborator Author

This PR will sit here until Friday before merge to allow folks to review.

@richard-einfinity
Copy link
Contributor

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.

https://github.com/OData/AspNetCoreOData/blob/main/src/Microsoft.AspNetCore.OData/Query/Container/TruncatedCollectionOfT.cs

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

@adrianhall
Copy link
Collaborator Author

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.

@adrianhall
Copy link
Collaborator Author

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.

@adrianhall
Copy link
Collaborator Author

... 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.

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.

Index attribute on BaseEntityTableData prevents CosmosEntityTableData with CosmosDB EF Provider
2 participants