-
Notifications
You must be signed in to change notification settings - Fork 36
EF-273: Pure refactoring (no behavior change) moving code out of MongoClientWrapper #252
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
base: main
Are you sure you want to change the base?
Conversation
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.
Pull Request Overview
This PR refactors the MongoDB Entity Framework Core provider by moving database creation and index management logic from MongoClientWrapper
to MongoDatabaseCreator
. This is a pure refactoring that maintains existing behavior while improving the separation of concerns.
Key changes:
- Moved database creation, deletion, and index management methods from
IMongoClientWrapper
/MongoClientWrapper
toIMongoDatabaseCreator
/MongoDatabaseCreator
- Updated tests to use
IMongoDatabaseCreator
instead ofIMongoClientWrapper
for database operations - Simplified
MongoDatabaseFacadeExtensions
by adding a helper method to reduce code duplication
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 9 comments.
Show a summary per file
File | Description |
---|---|
MongoDatabaseCreatorTests.cs | New test file with tests moved from MongoClientWrapperTests, updated to use IMongoDatabaseCreator |
MongoClientWrapperTests.cs | Deleted - tests moved to MongoDatabaseCreatorTests |
MongoDatabaseCreator.cs | Added database creation, deletion, and index management methods previously in MongoClientWrapper |
MongoClientWrapper.cs | Removed database/index methods; exposed Client, Database, and DatabaseName as public properties |
IMongoDatabaseCreator.cs | Added interface methods for database existence checks, index creation, and vector index operations |
IMongoClientWrapper.cs | Removed database/index method signatures; added Client, Database, and DatabaseName property definitions |
MongoDatabaseFacadeExtensions.cs | Simplified extension methods using new GetDatabaseCreator helper method |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
src/MongoDB.EntityFrameworkCore/Storage/MongoDatabaseCreator.cs
Outdated
Show resolved
Hide resolved
src/MongoDB.EntityFrameworkCore/Storage/MongoDatabaseCreator.cs
Outdated
Show resolved
Hide resolved
tests/MongoDB.EntityFrameworkCore.FunctionalTests/Storage/MongoDatabaseCreatorTests.cs
Show resolved
Hide resolved
tests/MongoDB.EntityFrameworkCore.FunctionalTests/Storage/MongoDatabaseCreatorTests.cs
Show resolved
Hide resolved
tests/MongoDB.EntityFrameworkCore.FunctionalTests/Storage/MongoDatabaseCreatorTests.cs
Show resolved
Hide resolved
tests/MongoDB.EntityFrameworkCore.FunctionalTests/Storage/MongoDatabaseCreatorTests.cs
Show resolved
Hide resolved
tests/MongoDB.EntityFrameworkCore.FunctionalTests/Storage/MongoDatabaseCreatorTests.cs
Show resolved
Hide resolved
tests/MongoDB.EntityFrameworkCore.FunctionalTests/Storage/MongoDatabaseCreatorTests.cs
Show resolved
Hide resolved
b751e43
to
f1fb915
Compare
Co-authored-by: Copilot <[email protected]>
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.
Pull Request Overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
// Internal because it is used by .NET 8 tests. | ||
internal void SeedFromModel() | ||
=> database.SaveChanges(AddModelData().GetEntriesToSave()); | ||
|
||
// Internal because it is used by .NET 9 tests. |
Copilot
AI
Oct 16, 2025
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.
The comment states this is 'used by .NET 8 tests', but given the async variant on line 185 states it's used by '.NET 9 tests', this distinction seems arbitrary. Consider clarifying why different .NET versions require different test methods or updating the comments to be more accurate.
// Internal because it is used by .NET 8 tests. | |
internal void SeedFromModel() | |
=> database.SaveChanges(AddModelData().GetEntriesToSave()); | |
// Internal because it is used by .NET 9 tests. | |
// Internal for test compatibility: .NET 8 tests use the synchronous method, while .NET 9 tests use the asynchronous method. | |
internal void SeedFromModel() | |
=> database.SaveChanges(AddModelData().GetEntriesToSave()); | |
// Internal for test compatibility: .NET 9 tests use the asynchronous method. |
Copilot uses AI. Check for mistakes.
public IMongoClient Client => _client ??= GetOrCreateMongoClient(_options, _serviceProvider); | ||
|
||
/// <inheritdoc /> | ||
public async Task<IClientSessionHandle> StartSessionAsync(CancellationToken cancellationToken = default) | ||
=> await Client.StartSessionAsync(null, cancellationToken).ConfigureAwait(false); | ||
public IMongoDatabase Database => _database ??= Client.GetDatabase(_databaseName); |
Copilot
AI
Oct 16, 2025
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.
[nitpick] These properties changed from private to public. While this appears intentional for the refactoring, consider documenting in the property XML comments that these may trigger client initialization as a side effect.
Copilot uses AI. Check for mistakes.
{ | ||
// Do anything that causes an actual database connection with no side effects | ||
clientWrapper.DatabaseExists(); | ||
DatabaseExists(); |
Copilot
AI
Oct 16, 2025
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.
This call to DatabaseExists()
is used only for its side effect of establishing a database connection. Consider adding a comment explaining this non-obvious behavior, or extracting to a method with a clearer name like EnsureConnected()
.
Copilot uses AI. Check for mistakes.
No description provided.