Repository navigation
Cosmos, DocumentDB, PgVector: resolve embedding generator per DI scope - #80
Conversation
|
LGTM. I ran the new unit tests locally on net10.0, including the Cosmos and PgVector ones: 12 out of 12 pass with the fix, and all 12 fail without it. Nothing else to report, apart from one missing test: the Cosmos overload that takes a connection string, EDIT: the missing test was added in d32b0e7. I ran it: it fails without the fix and passes with it. |
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues are reported for the change addressing cross-scope embedding-generator retention.
No open issues 🎉
What changed in this PR
Fixes #74 by preventing Cosmos, DocumentDB, and PgVector dependency-injection factories from retaining an embedding generator from an earlier scope.
Changes:
- Uses local resolved options instead of overwriting captured registration options.
- Adds two-scope regression tests for scoped and transient registrations.
- Updates test dependencies and bumps all three provider patch versions.
| File | Description |
|---|---|
| MEVD/test/PgVector.UnitTests/PostgresServiceCollectionExtensionsTests.cs | Tests embedding-generator resolution across scopes. |
| MEVD/test/PgVector.UnitTests/PgVector.UnitTests.csproj | Includes the shared fake embedding generator. |
| MEVD/test/AzureDocumentDB.UnitTests/DocumentDBServiceCollectionExtensionsTests.cs | Tests both store overloads and collection registration across scopes. |
| MEVD/test/AzureDocumentDB.UnitTests/AzureDocumentDB.UnitTests.csproj | Includes the shared fake embedding generator. |
| MEVD/test/AzureCosmosDB.UnitTests/CosmosServiceCollectionExtensionsTests.cs | Tests store and collection registration across scopes. |
| MEVD/test/AzureCosmosDB.UnitTests/AzureCosmosDB.UnitTests.csproj | Adds dependency-injection support for tests. |
| MEVD/src/PgVector/PostgresServiceCollectionExtensions.cs | Keeps resolved store options local to each factory invocation. |
| MEVD/src/PgVector/PgVector.csproj | Bumps version to 1.0.2. |
| MEVD/src/AzureDocumentDB/DocumentDBServiceCollectionExtensions.cs | Keeps resolved store and collection options local. |
| MEVD/src/AzureDocumentDB/AzureDocumentDB.csproj | Bumps version to 1.1.1. |
| MEVD/src/AzureCosmosDB/CosmosServiceCollectionExtensions.cs | Keeps resolved store and collection options local. |
| MEVD/src/AzureCosmosDB/AzureCosmosDB.csproj | Bumps version to 1.0.2. |
Review effort: Balanced
|
Thanks for the review and for running the tests locally! I added the same test as for DocumentDB (Scoped and Transient), so it should be covered now |
adamsitnik
left a comment
adamsitnik
left a comment
There was a problem hiding this comment.
@Empiree Thank you for providing the fixes! Do you think we could extend the conformance tests in https://github.com/dotnet/extensions/blob/main/src/Libraries/Microsoft.Extensions.VectorData.ConformanceTests/DependencyInjectionTests.cs to catch similar bugs?
@Laurianti thank you for reviewing the code!
|
@adamsitnik thanks for the merge! Yes, makes sense to me. Right now the DI conformance tests check the generator only on the first resolve, so they can’t see this bug. I’ll try to add a test that resolves the store and collection in two scopes and checks each scope gets its own generator. Will send a PR to dotnet/extensions |
The DI factories in Cosmos, DocumentDB and PgVector wrote the options with the embedding generator back into the captured options variable. With Scoped or Transient lifetime the first resolve kept the generator from its scope, and every next resolve reused it, even after that scope was disposed. Now they use a local copy, same as AddPostgresCollection already does.
Added tests that resolve the store or collection in two scopes and check the generator is resolved twice. The DocumentDB ones fail before the fix and pass after, I ran them locally. I couldnt run the Cosmos and PgVector tests here, so CI will be the first run for them. Bumped the three provider versions.
Fixes #74