Repository navigation
Observabilidad, fixes de concurrencia/seguridad, 3 nuevos proveedores NoSQL y pipeline de release - #5
Merged
Merged
Conversation
Adds ActivitySource-based StartActivity/RecordException helper used across all packages for Activity-based tracing (OpenTelemetry-compatible), plus null guards in ExpressionInspector and a dedicated Abstractions.Tests project.
…lder, instrument with diagnostics - GenericRepository<T,TKey> wraps ValiFlowEvaluator<T> (GetById/GetAll/GetPaged/Add/Update/Delete/SaveChanges) - Extract query-construction logic from ValiFlowEvaluator into QuerySpecificationBuilder (SRP) - Wrap read/write evaluator methods with ValiFlowDiagnostics Activity tracing, tags derived from specification shape - Fix UpsertRangeAsync duplicate-key bug (dictionary accumulator instead of list)
…ak, add diagnostics tags - Split ValiFlowEvaluator<T,TProperty> (904 lines) into Bridge/Read/Write/Grouped partial files - Fix Update(entity, externalList) leaking into internal store on a later bare SaveChanges() - Add optional tag parameter across read/write/grouped methods, wired to ValiFlowDiagnostics Activity
…s, add SetAllFrom and diagnostics - Add SqlIdentifierGuard (regex whitelist) to validate table/schema/type names before interpolation - Fix OrIgnore/OrReplace dialect guard bug, Set()-after-SelectFrom() silent data loss, Where() double-call overwrite - Add AllowDeleteUnmatched() guard before WhenNotMatchedBySourceDelete in SqlMergeBuilder - Add SetAllFrom(entity, exclude...) reflection-based bulk column mapper to Insert/Update builders - Wrap all 6 builders' Build() with ValiFlowDiagnostics Activity tracing using the existing .Tag(...) value
…iteral null x.Field == nullVariable (closure) threw ArgumentNullException unlike x.Field == null (literal) — TranslateComparison now builds a NullNode when the evaluated value is null, same as the literal path.
…h IN values, add diagnostics - Escape RediSearch query-syntax special characters in LIKE patterns before adding wildcards, fixing a query injection vulnerability (e.g. pattern containing ')' or '|') - Fix customConverter not applied in Redis VisitComparison/numeric VisitIn - Fix swallowed inner exception in Elasticsearch ToDouble - Add MaxInValues caps (Mongo 10_000, Elasticsearch 65_536 matching index.max_terms_count) - Wrap Translate(...) with ValiFlowDiagnostics Activity tracing (tag + entityType)
Translate(...) now opens a ValiFlowDiagnostics Activity (tag + entityType), consistent with the other NoSql providers.
- Vali-Flow.NoSql.Couchbase: N1QL (SQL++) WHERE-fragment translator, no SDK dependency - Vali-Flow.NoSql.CosmosDb: Cosmos SQL API WHERE-fragment translator, no SDK dependency - Vali-Flow.NoSql.Firestore: native Google.Cloud.Firestore.Filter translator (LikeNode and generic NotNode throw NotSupportedException — SDK has no pattern-matching query and no NOT over an arbitrary sub-filter) - Add bilingual docs (en/es) and wire all 3 into the solution
Adds EfCoreBenchmarks + BenchmarkDbContext, referencing Vali-Flow.csproj directly (ProjectReference, consistent with this project's existing pattern for InMemory/Sql).
Add the 3 new NoSql providers (Couchbase, CosmosDb, Firestore) to the package table and architecture diagram; update benchmarks docs for the EF Core benchmark. Extend .gitignore for internal-tooling files that must never be tracked.
…itionUpsertRange CurrentValues.SetValues(entity) copies every scalar property by name, including the primary key. When upserting by a business key (e.g. a Sku), the caller's incoming entity normally carries a freshly-generated Id unrelated to the existing row's PK, and EF throws 'is part of a key and so cannot be modified' instead of updating in place. Align the incoming entity's PK value(s) with the tracked entity's PK before SetValues, via FindPrimaryKey() + reflection (supports composite keys). Found while stress-testing the EF Core evaluator under realistic upsert payloads (UpsertRangeAsync with duplicate business keys under concurrent load). Also bump Vali-Flow.Abstractions pin from 1.0.0 to 1.1.0 to match the version every other package in the solution already pins.
Add 96 tests covering previously-untested paths: the explicit IQueryReader<T>/ IQueryAggregator<T> interface members on ValiFlowEvaluator, BasicSpecification/ QuerySpecification fluent setters and QuerySpecificationBuilder guard clauses, long/float/double overloads of *ByGroupAsync, GenericRepository.SaveChangesAsync, ServiceCollectionExtensions.AddValiFlowEvaluator, remaining catch/RecordException branches, pagination guards, the relational DeleteByConditionCoreAsync path, and the rollback-itself-fails branch of RollbackTransactionAsync. Remaining uncovered lines are dead/unreachable defensive code, documented inline rather than forced with artificial tests.
…r entity type + key columns
EFCore.BulkExtensions' PostgreSQL adapter implements the upsert via a temporary
unique index with a deterministic name (tempUniqueIndex_{schema}_{table}_{column}),
created then dropped around the merge. Concurrent calls against the same
table/key columns race on that shared temp index, surfacing as
'42704: index "tempUniqueIndex_..." does not exist' — reproduced with as few
as 8 parallel single-row requests in the same process during stress testing.
Add an in-process SemaphoreSlim gate keyed by (entity type, UpdateByProperties)
to serialize same-key calls. Does NOT cover cross-process concurrency (multiple
API instances) — that collision still exists at the database level and would
need a Postgres advisory lock or equivalent; documented as a known limitation.
…cross SQL Server, PostgreSQL, MySQL, and SQLite Adds BulkUpsertRetryPolicy: a provider-agnostic retry (exponential backoff + jitter, no hard dependency on any ADO.NET provider) for the cross-process concurrency window the in-process semaphore gate (added for the stress-test finding on BulkInsertOrUpdateAsync) doesn't cover. Detects transiency via message-chain markers for the documented concurrency failure mode of each of the four most-used relational engines this ecosystem targets: - PostgreSQL/MySQL: EFCore.BulkExtensions' temporary unique index/constraint race when matching by BulkConfig.UpdateByProperties (the original finding). - SQL Server: deadlock victim on the bulk merge (documented upstream, EFCore.BulkExtensions issue #46). - SQLite: SQLITE_BUSY 'database is locked' under a concurrent writer. Oracle is deliberately not covered -- supported by EFCore.BulkExtensions but a materially less common deployment target, and its failure wording for this race hasn't been characterized. Wires the same gate+retry path into BulkUpdateAsync (previously only BulkInsertOrUpdateAsync was covered) since both share the identical BulkConfig.UpdateByProperties exposure. Adds InternalsVisibleTo so the retry policy can be unit-tested in isolation (21 new tests, no EF Core/database involved -- the race itself can't be reproduced deterministically against the InMemory/SQLite test providers) plus a BulkUpdateAsync concurrency regression test mirroring the existing BulkInsertOrUpdateAsync one.
…7 attempts Validated the retry against the real cross-process scenario it targets: 2 API instances, 50 req/s combined, 15 shared business keys via BulkInsertOrUpdateAsync. The mechanism worked (113 retries triggered, 0 duplicate rows), but the original 4-attempt / 400ms-max-delay budget (~450ms total) left ~2% of requests (8/405) exhausting all retries and propagating a 500 (40P01 deadlock detected, 42704 index does not exist) under sustained contention. Raised to 7 attempts / 900ms max delay to give the losing side of the race more chances to land after the winner releases the temp index/lock. Also confirmed Postgres' actual deadlock error code (40P01, not the originally assumed 42704) still matches the existing generic "deadlock" substring marker without any change needed there.
Second measured iteration: 7 attempts cut the propagated-error rate from ~2% (8/405) to 0.12% (1/855) under the same artificially-high-contention benchmark (2 API instances, 50 req/s combined, 15 shared keys) -- real improvement, not zero. Raised to 12 attempts (~5.4s worst-case backoff budget) to push residual failure rate further down under this deliberately narrow pool. Documented in the constant's XML remarks that no finite retry budget can offer a 0% proof for a queueing race -- only diminishing residual probability at the cost of added worst-case latency.
…ByProperties key set A stress run sweeping the shared business-key pool from 15 up to 1,000 and 10,000 distinct SKUs found the retry activation rate unchanged across every pool size (consistently ~20-25 activations per instance), ruling out key collision as the cause. The real contention is PostgreSQL's CREATE INDEX CONCURRENTLY (part of the merge EFCore.BulkExtensions runs for a non-PK match), which serializes against any other transaction on the same table regardless of which rows/columns it touches. The in-process gate was keyed by (type, UpdateByProperties columns), which would let two same-process calls with different UpdateByProperties on the same table run concurrently and still race -- re-keyed to type (table) alone, matching the real contention boundary. GetBulkUpsertGate no longer needs the BulkConfig parameter.
…/InMemory test assemblies
A full-solution test run surfaced an intermittent failure in Vali-Flow.Sql.Tests
('Sequence contains no matching element') in the diagnostics tests added by an
earlier commit. Root cause: each test parents its captured System.Diagnostics.
Activity under a private root and filters by ParentId to survive cross-test
pollution on the shared process-wide ValiFlowDiagnostics.Source listener -- but
under xUnit's default test-class-level parallelism this is not fully
deterministic; it just has lower collision probability with fewer competing
Activity-asserting test classes, which is why it hadn't surfaced yet in
Vali-Flow.Tests/Vali-Flow.InMemory.Tests (same pattern, fewer classes) and did
in Vali-Flow.Sql.Tests (6 builder test classes all asserting on Activities).
Fixes, in order:
1. Rewrote the 6 duplicated per-file ActivityListener setups in Sql.Tests
(SqlDeleteBuilderTests, SqlInsertBuilderTests, SqlMergeBuilderTests,
SqlTruncateBuilderTests, SqlUpdateBuilderTests, SqlQueryBuilderTests) to use
the same AttachScopedListener/GetOwn helper pattern already used elsewhere,
removing a dead/unused AttachListener() helper and a redundant double-listener
in SqlQueryBuilderTests along the way.
2. Added [assembly: CollectionBehavior(DisableTestParallelization = true)] to
Vali-Flow.Sql.Tests, Vali-Flow.Tests, and Vali-Flow.InMemory.Tests -- removes
the race at its source instead of chasing per-test correlation tokens.
Verified clean across 8 consecutive Sql.Tests runs and 5 consecutive
full-solution runs. Cost: Sql.Tests went from ~250ms to ~800ms, negligible.
… strings ResolveValue converted decimal to a string (m.ToString(InvariantCulture)) for every operator (=, <>, <, <=, >, >=). N1QL has no decimal type and does NOT coerce between string and number in comparisons (its collation order puts number < string unconditionally) -- a string-bound parameter silently matched zero rows against a numeric field, for every operator, 100% of the time. Found running the translator's N1QL output against a real Couchbase Server (Caso A stress validation): EqualNode(decimal) and ComparisonNode(decimal) failed in all 3 tested scales (1K/20K/100K docs) while the equivalent int field worked correctly. Mongo/Elasticsearch/DynamoDB translators already preserve the native numeric type for decimal; Couchbase was the only one converting it to string.
…ackageReference 1.2.0
…eference 1.1.1 in the 7 NoSQL providers
These 3 providers were scaffolded without the package icon file this session, causing dotnet pack to fail with NU5046 (PackageIcon set in csproj but file absent). Copied from Vali-Flow.NoSql, which ships the same icon.
…GenericRepository, diagnostics, security fixes) - Root CHANGELOG.md [Unreleased]: documents everything shipped since the last entry (1.3.3, April) that was never logged — 3 new NoSQL translator packages (Couchbase, CosmosDb, Firestore), GenericRepository<T,TKey>, ValiFlowDiagnostics observability helper, the SQL identifier injection fix, the RediSearch query injection fix, Mongo/Elasticsearch IN-value caps, the bulk upsert retry policy (and its budget tuning 4->7->12), and several smaller bug fixes across Sql/NoSql/InMemory/EF Core. - Created CHANGELOG.md for the 3 new packages (Couchbase/CosmosDb/Firestore had none), following the existing per-package format. - Fixed Vali-Flow.NoSql.MongoDB/CHANGELOG.md: described a stale API (ToMongoFilter<T>() -> FilterDefinition<T>) that doesn't match the real current one (ToMongo<T>() -> BsonDocument, verified against docs/en/vali-flow-nosql-mongodb.md and the actual extension method source) — found independently by 3 separate doc-porting passes while researching the new providers.
All packages build for net8.0/net9.0 only, but run unmodified on .NET 10 via forward compatibility. Documented that explicitly everywhere a .NET version requirement is stated (README, architecture overview, getting started guide, EF Core evaluator requirements) — en + es.
Vali-Flow/InMemory/Sql/NoSql and the 7 NoSQL providers already reference Vali-Flow.Abstractions >= 1.2.0 / Vali-Flow.NoSql >= 1.1.1 in their PackageReference, but those versions aren't published to NuGet.org yet (that's this pipeline's job, once run). Pack both leaf packages from the in-repo source to a local folder and register it as an extra NuGet source before restoring the solution, so CI can validate before the real publish instead of requiring it to have already happened. Applied to the build job, each test matrix cell, and the pack job (which already packed in dependency order but never exposed its own output folder as a source for the next package in the loop to resolve against).
… the real run - All 11 test projects were single-target net9.0, but ci.yml's test matrix forces -f net8.0/-f net9.0 against every one of them uniformly. Multi-target them to net8.0;net9.0 (matching the 12 shipping packages' convention) so the net8.0 matrix cell actually has something to build and run instead of failing on every project. - dotnet test "$project" (bare directory) intermittently mis-resolves the test assembly on some SDK/TFM combinations; use the explicit "$project/$project.csproj" path instead, matching the pack job's own convention. - The "prime local feed" step used $(pwd) under Git Bash, which yields a POSIX path that dotnet nuget add source rejects on Windows runners; convert via cygpath -w when RUNNER_OS is Windows. Verified locally: build 0 errors, all 11 projects pass on net8.0 explicitly (1794/1794, matching the existing net9.0 baseline).
The first real CI run against this branch measured actual per-library coverage: 8 of 12 packages came in under the blanket 95% placeholder (Vali-Flow.InMemory 76.6%, Vali-Flow.NoSql.Elasticsearch 84.9%, Vali-Flow.NoSql 84.2%, Vali-Flow.NoSql.Firestore 88.6%, Vali-Flow.Sql 91.4%, Vali-Flow.NoSql.Redis 91.0%, Vali-Flow.NoSql.Couchbase 92.5%, Vali-Flow.NoSql.CosmosDb 92.7%). Set each library's threshold a few points under its measured number instead of dropping the gate entirely, so it still catches a real regression from where things stand today. The 4 libraries already at or above 95% (Abstractions, Vali-Flow, MongoDB, DynamoDB) keep that bar.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resumen
Reemplaza a los PR #3 y #4 (cerrados). Consolida de nuevo todo el trabajo en un solo PR, ahora con el fix real:
ci.ymlempaquetaVali-Flow.AbstractionsyVali-Flow.NoSqldesde el código fuente del repo a un feed local antes de restaurar, así el restore resuelve las versiones nuevas (1.2.0 / 1.1.1) aunque todavía no estén publicadas en NuGet.org. Los.csprojsiguen siendo 100%PackageReference— el fix solo cambia cómo CI valida antes de publicar, no cómo se empaquetan ni se publican los paquetes.ValiFlowDiagnostics(ActivitySource) en todos los paquetes.Vali-Flow.Sql, fix de inyección en RediSearch + caps IN, fix de decimales en Couchbase, fix de leak enUpsertRangeAsync, tests flaky deActivityeliminados.ProjectReferencetemporales aPackageReference,.github/workflows/ci.yml(con el fix de feed local) y.github/workflows/release.yml(3 olas de publicación real).Tabla de versiones
Plan de pruebas
dotnet build/dotnet testlocal — 0 errores, 1794/1794 testsdotnet packlocal — 12/12 paquetesworkflow_dispatchreal conpublish: false(pendiente, post-merge)