Skip to content

v1.0.0

Choose a tag to compare

View all tags
@tziazas tziazas released this 24 May 20:24
· 5 commits to main since this release
v1.0.0
19e78a7
This commit was created on GitHub.com and signed with GitHub’s verified signature.
GPG key ID: B5690EEEBB952194
Verified
Learn about vigilant mode.

Summary

Full code-review pass on the library ahead of the 1.0.0 release. Sixteen findings addressed across security, correctness, performance, design, and documentation, plus a complete README rewrite and a new graded library assessment.

Security

#1 — SQL injection: IN clause values not parameterized
GetAllByArrayPropertyAsync was building the IN (...) clause by string-concatenating values directly into the query. Values are now parameterized (@p0, @p1, …) via the PropertyFilterExtensions helper.

#2 — SQL injection: array query property names interpolated into SQL
arrayName and elementPropertyName were interpolated directly into query strings without validation. Both are now validated against CosmosValidationConstants.SafePropertyNamePattern (^[a-zA-Z_][a-zA-Z0-9_.]*$) before the query is built. Unit tests added in CosmosValidatorArrayQueryTests.

Correctness

#11 — Soft delete had no ETag / optimistic concurrency
SoftDeleteAsync was reading the document via GetItemAsync (which discards the ItemResponse<T> and therefore the ETag), then replacing without a concurrency check. Two concurrent soft-delete requests could silently overwrite each other. Fixed by calling ReadItemAsync directly inside SoftDeleteAsync to capture the ETag, then passing IfMatchEtag on the replace call.

#12 — Count query regex was fragile
ConvertToCountQuery only matched SELECT * FROM, silently producing a broken query for any other projection (named fields, SELECT VALUE, etc.), and left ORDER BY and OFFSET/LIMIT clauses intact — both invalid in a COUNT context. Replaced with three compiled static Regex fields that handle any SELECT projection, strip ORDER BY, and strip OFFSET/LIMIT. Unit tests added in SqlQueryExtensionsTests (17 cases).

Performance

#6 — Reflection on every write
GetPartitionKeyValue called typeof(T).GetProperty(name).GetValue(item) on every single write operation. Replaced with a compiled expression tree (Expression.Lambda<Func<T, string>>().Compile()) built once in the constructor and stored as _getPartitionKey. Zero reflection overhead at call time.

#8 — Manual cache expiry duplicated IMemoryCache
GetCountWithCacheAsync was running a manual age-check against a CachedCountEntry.CachedAt timestamp in parallel with AbsoluteExpirationRelativeToNow already set on the cache entry. Removed CachedCountEntry entirely. IMemoryCache now owns the TTL exclusively; the cache stores int directly.

Design

#3 — Polly registered but never used
AddCosmoBase was registering a Polly retry pipeline that was never wired to any Cosmos SDK call. The SDK's own built-in retry (MaxRetryAttemptsOnRateLimitedRequests, MaxRetryWaitTimeOnRateLimitedRequests) was already handling retries. Polly dependency and all related registration code removed.

#7 — NotImplementedException explicit interface implementations
CosmosDataReadService and CosmosDataWriteService contained explicit IDataReadService<TDto, string> and IDataWriteService<TDto, string> method implementations that threw NotImplementedException. These dead code paths have been removed along with the open-generic DI registrations for those interfaces.

#10 — new() constraint not present on interface
CosmosRepository<T> had where T : class, ICosmosDataModel, new() but ICosmosRepository<T>, ICosmosValidator<T>, and CosmosValidator<T> did not. The new() constraint was removed from all four — DAO types no longer need a public parameterless constructor.

#16 — Wrong exception type for unsupported specification types
QueryAsync and BulkReadAsyncEnumerable threw ArgumentException when a non-SqlSpecification<T> was passed. The signatures intentionally keep ISpecification<T> to allow future specification types without a breaking API change. The guard now throws NotSupportedException with the message "Only SqlSpecification<T> is currently supported. Received: {type.Name}" — correctly signalling a temporary implementation limitation rather than a caller contract violation.

Cleanliness / Dead Code

#5 — Dead Newtonsoft.Json dependency
Newtonsoft.Json was referenced in CosmoBase.Core.csproj and used nowhere. Package reference removed. The Cosmos SDK v3 build guard that requires Newtonsoft to be present is suppressed via <AzureCosmosDisableNewtonsoftJsonCheck>true</AzureCosmosDisableNewtonsoftJsonCheck> in Directory.Build.props.

#9 — _disposed dead code
CosmosRepository<T> declared private bool _disposed but never read or wrote it in any meaningful way. Field removed.

Clarity

#14 — GetAllAsync(limit, offset, count) parameter naming
The three parameters had names that didn't match their semantics:

  • limit → pageSize (controls LIMIT per SDK round-trip, not total results)
  • count → maxItems (in-process yield cap across all pages)

Updated across ICosmosRepository<T>, ICosmosDataReadService<TDto, TDao>, CosmosRepository<T>, CosmosDataReadService<TDto, TDao>, and all call sites in tests.

Documentation / Observability

#4 — CI pipeline had no dotnet test step
Unit and integration test jobs added to .github/workflows/publish.yml. Unit tests run without external dependencies; integration tests use Testcontainers.CosmosDb. The publish job is gated on both test jobs passing.

#13 — Debug tests removed from test suite
20 Debug_* test methods in DataServicesIntegrationTests were exploratory scaffolding — not assertions, not repeatable, and not representative of correct behaviour. Removed. 16 real integration tests remain.

#15 — Cross-partition GetAllAsync() had no cost warning
The no-argument GetAllAsync() overload fans out to every physical partition with no signal that it does so. Added XML doc warnings on ICosmosRepository<T> and ICosmosDataReadService<TDto, TDao>, and a LogWarning in both CosmosRepository and CosmosDataReadService implementations that fires on every call.

New Files

  • src/CosmoBase.Core/AssemblyAttributes.cs — [assembly: InternalsVisibleTo("CosmoBase.Tests")] so unit tests can reach internal helpers like SqlQueryExtensions
  • tests/CosmoBase.Tests/Unit/Extensions/SqlQueryExtensionsTests.cs — 17 unit tests for count query conversion
  • docs/library-grade.md — structured grade assessment across eight dimensions with specific findings and recommended next steps

README

Complete rewrite:

  • Added ICosmosDataModel contract so developers know exactly what their DAO needs to implement
  • Fixed broken code block and BuildServiceProvider() anti-pattern in the web registration section
  • All query overloads documented with cross-partition warnings where applicable
  • SqlSpecification<T> with parameterized query examples
  • PropertyFilter / GetAllByPropertyComparisonAsync with PropertyComparison constants
  • Patch operations (PatchDocumentAsync, PatchDocumentListItemAsync) with audit-field caveats
  • BulkReadAsyncEnumerable documented
  • Both pagination methods (GetPageWithTokenAsync, GetPageWithTokenAndCountAsync) with runnable examples
  • Metrics table with accurate instrument names and OpenTelemetry wiring
  • Migration notes for 0.1.4 breaking changes
  • Version updated to 0.1.4

Test Plan

  • dotnet build CosmoBase.sln — 0 errors, 0 warnings
  • dotnet test --filter "FullyQualifiedName~.Unit." — all 49 unit tests pass
  • dotnet test --filter "FullyQualifiedName~.Integration." — all 16 integration tests pass (requires Docker for Testcontainers)
Assets 2
Loading