Code review sweep: harden every CoreEx package, close doc/reality gaps - #181
Merged
Conversation
- ValueFormatter: always use provided FormatProvider for string.Format - PropertyContext: add RefreshFromEntity() to fix struct value sync in rule chains - ComparePropertyRule: wrap OverflowException as InvalidCastException - CompareValuesRule: fix overrideValueWhereMatched bug, propagate overrides - DecimalRule: handle decimal.CreateChecked OverflowException as validation error - MandatoryRule/NullNoneEmptyRule: dispose enumerators from non-collection IEnumerable - ValidationExtensions: materialize allowed enum values, add regex ReDoS remarks - ValidatingInlineValidator: ensure .ConfigureAwait(false) for async - Add regression/unit tests for bounds, misconfig, disposal, exceptions, formatting, and override propagation - Improves correctness, robustness, and resource management
- Distinguish CancellationToken vs. unrelated cancellations in event subscriber/publisher; only host shutdowns now bubble up. - EventFormatter now applies InvariantCulture casing for Title/Source; added remarks and regression tests (e.g., Turkish "I"). - SubscribeAttribute and SubscribedBase glob-matching now use RegexOptions.CultureInvariant for deterministic case-insensitive matching; regression tests added. - EventSubscriberMetrics now classifies all IEventSubscriberException outcomes and records "subscribed" tag for SubscribedManager. - Removed AddAssignableFrom overloads from ErrorHandler; clarified XML docs. - Removed debug event payload logging from EventPublisherBase (now test-only via EventPublisherDecorator). - EventPublisherDecorator supports optional debug logging for tests. - Added/updated unit tests for all above behaviors. - Minor cleanups: null/empty checks, ordering, comments, and global usings.
- Fixed ServiceBusReceiver/SessionReceiver StopAsync to avoid OnStopAsync if never started; ensured all semaphore waits use .ConfigureAwait(false). - Improved ServiceBusErrorClassifier: handle SessionCannotBeLocked as info, not lock-lost; added tests. - Suppressed broker error descriptions for dead-lettered messages to avoid log duplication. - Refactored DI registration for receivers/subscribers to prevent key confusion and resolve WithKeyedSubscriber(...).WithHostedService() bug. - Changed session assignment for events without partition key to use "$none" (not GUID), bounding session growth and preserving order; added logging and tests. - Ensured CloudEvent trace context and identity attributes are preserved in Binary-mode round-trip, even when includeAttributes is false; added regression tests. - Hardened circuit-breaker and catastrophic error pause/resume logic to log failures instead of unobserved task exceptions. - Updated comments, fixed doc remarks, and added/updated unit tests for all new and regression scenarios.
- Qualify all tags via ICacheKeyProvider to prevent cross-tenant/domain invalidation when sharing IFusionCache. - FusionHybridCache.ConfigureEntryOptions now accepts both HybridCacheEntryOptions and FusionCacheEntryOptions, enabling richer configuration. - AddFusionHybridCache supports an optional configure callback for DI-time configuration. - Add/extend unit tests for tag qualification, entry options configuration, and DI callback behavior.
- Health check detailed endpoints are now disabled by default for security; opt-in and securing guidance added, with warnings if enabled unsafely. - Paging logic uses long arithmetic to prevent overflow in next/prev links. - Idempotency: Set-Cookie headers excluded from cache, request body hashed in chunks, only relevant headers overwritten on replay, tracing tags set post-validation. - Web API: PATCH endpoints enforce correct media type, improved cancellation/exception handling, delete behavior configurable, GraphQL errors include codes. - Usings consolidated in GlobalUsing.cs. - Docs updated for new health check defaults and security. - Regression tests added for all new behaviors and edge cases.
Add optional Action<OpenApiOptions> to AddCoreExConfiguration for custom JsonSerializerOptions propagation to schema generation. Update AcceptsAttribute handling to set OpenApiRequestBody.IsRequired based on IsOptional. Use indexer assignment for OpenAPI headers/content for idempotency. Correct documentation for ProducesNotFoundProblemAttribute (404) and QueryAttribute.SupportsFilter (default true). Add POST endpoint (optional-body) to OtherController for AcceptsAttribute demo. Add integration/unit tests for OpenAPI request body requiredness and JsonSerializerOptions flow.
Improved CoreEx query filter and order-by parsing: - Fixed argument placeholder renumbering to prevent collisions. - QueryFilterParser now errors on duplicate fields; supports 32/36-char Guids. - Enforced stricter token validation in logical/function expressions. - QueryOrderByParser enforces direction allow-lists and fixes default formatting. - Added comprehensive tests for filters, order-by, ref-data fields, and edge cases. - Updated docs and project references.
- `GetIdentifier<TId>` now converts boxed values using `TId.Parse`, supporting variable-supplied types (e.g., `long` to `int`, `string` to `Guid`). - Added comprehensive tests for identifier conversion and error cases. - Filter translator consults `QueryFilterParser` for quoting, ensuring correct OData syntax for types like `Guid`, `DateTime`, etc. - Unquoted "Other" values are now injection-safe (no spaces, parentheses, or commas). - Improved error messages for invalid boolean filter usage. - Debug logging omits sensitive filter/order-by text by default; opt-in with `EnableSensitiveDataLogging`. - PageInfo fields in connection results are now case-insensitive. - Introspection schema builder detects and rejects CLR type name collisions. - Float parsing prefers `decimal` for precision, falling back to `double` if needed. - Updated tests and docs to cover new behaviors and edge cases.
Aggregate<TId, TSelf> now throws if AddEvent or ClearEvents are called on a read-only aggregate. EntityBase explicit IIdentifierCore.Id/IdType implementations throw NotSupportedException. The Remove method now fires the Mutated event before making the entity read-only, matching documentation. AggregateTests expanded to cover Remove return values, event order, and read-only enforcement for AddEvent/ClearEvents. These changes improve mutation safety and test coverage.
Introduce `TextJsonName` on `[ReferenceData]` to allow custom JSON property names for the "Text" property. The code generator now emits the `RefDataText` property only when `Text = true` (default), and applies the custom JSON name if provided. Update `PropertyModel` and `ContractModel` to support these options. Handlebars templates now conditionally emit the property and its attributes. Add unit tests to verify omission and custom naming behavior.
Added CoreEx.UnitTesting.Test.Unit project with full solution wiring and comprehensive unit tests for event expectations, JSON data reader, person entity helpers, validation extensions, and AwesomeAssertions. Improved event expectation configuration with new AssertWithValue factory overload, better path handling, and clearer error messages. Updated docs to clarify event assertion APIs and usage. Fixed minor issues including typos, comments, Service Bus test reliability, and validation error handling. Ensured all tests use correct hosts and DI patterns.
- Clarified `ref-data.yaml` placement and updated outputs table with exact file names/locations and default folders. - Documented new `ApiGenerator` and clarified generator responsibilities in README. - Fixed duplicate `IdType` assignment in `EntityConfig`. - Updated repository interface template to use `EntitiesWithRepository`. - Enhanced documentation for configuration and generator class roles.
Replaces "app-name-lower" with "app-name-kebab-case" for service naming in docker-compose.yml and template.json. Adds "DotToKebabCase" and "KebabCase" transformations to template.json for consistent kebab-case formatting. Updates docker-compose.yml to use the new variable.
Updated XML docs for MapHealthChecks and MapHostedServices to clarify that only the absence of the configuration delegate is checked, not enforcement of authentication/authorization. Added explicit warnings recommending delegate usage for security. MapHostedServices now logs a runtime warning if groupConfigure is not supplied, alerting developers that hosted service endpoints will be accessible anonymously. This aligns with the health check endpoint warning pattern.
Added .claude/commands/coreex-scaffold.md and updated CoreEx.Template to support /coreex-scaffold in Claude Code and Copilot Chat. Clarified documentation in README and getting-started guides. Fixed appsettings.Development.json template to only include Npgsql log suppression for Postgres, not SQL Server. Added regression tests to validate correct log config gating. Enabled app.UseAuthorization() by default in Program.cs.
Contributor
There was a problem hiding this comment.
Pull request overview
Repository-wide hardening and doc/reality alignment across CoreEx packages, focusing on fixing concrete behavioral bugs, tightening security defaults, and adding regression coverage to prevent regressions in core primitives used by many packages.
Changes:
- Fixes multiple correctness issues across validation, querying, eventing, Service Bus receiving, and ASP.NET Core WebApi behaviors.
- Tightens security posture and operational robustness (health-check/hosted-service endpoint handling, idempotency replay safety, circuit-breaker pause/resume error visibility).
- Adds/extends regression test coverage (including a new dedicated
CoreEx.UnitTesting.Test.Unitproject) and updates templates/docs/instructions to match runtime behavior.
Reviewed changes
Copilot reviewed 190 out of 190 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/CoreEx.Validation.Test.Unit/ValidatorTests.cs | Adds regression coverage for explicit format-provider behavior under localization. |
| tests/CoreEx.Validation.Test.Unit/Rules/NullNoneEmptyRuleTests.cs | Adds regression test ensuring enumerators are disposed for non-collection IEnumerable. |
| tests/CoreEx.Validation.Test.Unit/Rules/MandatoryRuleTests.cs | Adds regression test ensuring enumerators are disposed for non-collection IEnumerable. |
| tests/CoreEx.Validation.Test.Unit/Rules/EnumRuleTests.cs | Adds regression test ensuring allowed-values IEnumerable is enumerated once at config time. |
| tests/CoreEx.Validation.Test.Unit/Rules/DictionaryRuleTests.cs | Adds misconfiguration tests for min/max count validation. |
| tests/CoreEx.Validation.Test.Unit/Rules/DecimalRuleTests.cs | Adds regression tests for overflow/NaN/Infinity handling in precision/scale validation. |
| tests/CoreEx.Validation.Test.Unit/Rules/CompareValuesRuleTests.cs | Adds regression tests for override behavior and chained-rule propagation. |
| tests/CoreEx.Validation.Test.Unit/Rules/ComparePropertyRuleTests.cs | Adds regression test for overflow handling and consistent exception wrapping. |
| tests/CoreEx.Validation.Test.Unit/Rules/CollectionRuleTests.cs | Adds misconfiguration tests for min/max count validation. |
| tests/CoreEx.Validation.Test.Unit/Rules/BetweenRuleTests.cs | Adds regression coverage for exclusive-between semantics. |
| tests/CoreEx.Validation.Test.Unit/CommonValidatorTests.cs | Extends coverage for common validators (success + error cases). |
| tests/CoreEx.UnitTesting.Test.Unit/EntryPoint.cs | New unit-test host entrypoint for CoreEx.UnitTesting package verification. |
| tests/CoreEx.UnitTesting.Test.Unit/CoreEx.UnitTesting.Test.Unit.csproj | New test project definition for CoreEx.UnitTesting package. |
| tests/CoreEx.RefData.Test.Unit/ReferenceDataAttributeCodeGenTests.cs | Adds tests for [ReferenceData] Text/TextJsonName codegen behavior. |
| tests/CoreEx.Data.Test.Unit/CoreEx.Data.Test.Unit.csproj | Adds CoreEx.RefData reference to support new/updated test scenarios. |
| tests/CoreEx.Data.GraphQL.Test.Unit/Internal/GraphQLArgsMapperTests.cs | Updates tests to new BuildQueryArgs(args, filterParser) signature. |
| tests/CoreEx.Azure.Messaging.ServiceBus.Test.Unit/Subscribers/ProductSubscriber.cs | Adjusts subscriber to exercise cancellation classification + result behaviors. |
| tests/CoreEx.Azure.Messaging.ServiceBus.Test.Unit/CoreEx.Azure.Messaging.ServiceBus.Test.Unit.csproj | Adds CoreEx.UnitTesting reference for new test/assertion usage. |
| tests/CoreEx.AspNetCore.Test.Unit/WebApiTestsBase.MergePatchWithResult.cs | Updates merge-patch content-type error messaging assertion. |
| tests/CoreEx.AspNetCore.Test.Unit/WebApiTestsBase.MergePatch.cs | Updates merge-patch messaging + adds explicit rejection test for application/json. |
| tests/CoreEx.AspNetCore.Test.Unit/WebApiTestsBase.GetWithResult.cs | Adds regression test preventing paging-link overflow for large $skip. |
| tests/CoreEx.AspNetCore.Test.Unit/WebApiTestsBase.Exceptions.cs | Adds regression tests for “own token” vs unrelated-token cancellation handling. |
| tests/CoreEx.AspNetCore.Test.Unit/WebApiTestsBase.Delete.cs | Adds regression test for configurable delete not-found conversion behavior. |
| tests/CoreEx.AspNetCore.Test.Unit/OtherApiTests.cs | Adds regression coverage for AcceptsAttribute requestBody required/optional in Swagger JSON. |
| tests/CoreEx.AspNetCore.Test.Unit/NSwagExtensionsTests.cs | New unit tests ensuring custom JsonSerializerOptions flow into schema generation. |
| tests/CoreEx.AspNetCore.Test.Unit/ExecutionContextMiddlewareTests.cs | New unit test ensuring headers aren’t mutated after response started. |
| tests/CoreEx.AspNetCore.Test.Api/Program.cs | Ensures test host maps health checks with detailed endpoints explicitly enabled for coverage. |
| tests/CoreEx.AspNetCore.Test.Api/Controllers/OtherController.cs | Adds optional-body endpoint for OpenAPI coverage; extends test controller surface. |
| src/CoreEx.Validation/ValidationExtensions.EnumRule.cs | Avoids re-enumeration of allowed values by materializing once at configuration time. |
| src/CoreEx.Validation/ValidatingInlineValidator.cs | Adds missing ConfigureAwait(false) consistency. |
| src/CoreEx.Validation/Rules/PropertyRuleBase.cs | Refreshes property context from entity after rule execution to support overrides in chains. |
| src/CoreEx.Validation/Rules/NullNoneEmptyRule.cs | Ensures enumerators are disposed when enumerating non-collection IEnumerable. |
| src/CoreEx.Validation/Rules/MandatoryRule.cs | Ensures enumerators are disposed when enumerating non-collection IEnumerable. |
| src/CoreEx.Validation/Rules/DecimalRule.cs | Converts overflow/NaN/Infinity into validation errors (not unhandled exceptions). |
| src/CoreEx.Validation/Rules/CompareValuesRule.cs | Fixes override/no-match crash and ensures override uses matched value reliably. |
| src/CoreEx.Validation/Rules/ComparePropertyRule.cs | Wraps OverflowException consistently as InvalidCastException for comparison compatibility. |
| src/CoreEx.Validation/PropertyContext.cs | Adds RefreshFromEntity to sync struct context across chained rule invocations. |
| src/CoreEx.Validation/Abstractions/ValueFormatter.cs | Ensures explicit IFormatProvider is honored for composite formatting. |
| src/CoreEx.UnitTesting/UnitTestExOneOffTestSetUp.cs | Adjusts validation error extraction behavior for null text cases. |
| src/CoreEx.UnitTesting/UnitTestExExtensions.Validation.cs | Removes incorrect/outdated XML doc entry. |
| src/CoreEx.UnitTesting/UnitTestExExtensions.ServiceBus.cs | Increases receive wait time to avoid latency-induced message drops; fixes typo. |
| src/CoreEx.UnitTesting/UnitTestExExtensions.Events.cs | Passes logger to EventPublisherDecorator. |
| src/CoreEx.UnitTesting/UnitTestExExpectations.Identifier.cs | Fixes identifier expectation to use the correct read-only identifier interface/type info. |
| src/CoreEx.UnitTesting/UnitTestExExpectations.Events.cs | Fixes request-state keying to use consistent suffix constant. |
| src/CoreEx.UnitTesting/UnitTestExExpectations.ETag.cs | Uses read-only ETag interface for expectations. |
| src/CoreEx.UnitTesting/UnitTestExExpectations.ChangeLog.cs | Fixes created/updated-by checks and empty-string expectation semantics. |
| src/CoreEx.UnitTesting/GlobalUsings.cs | Removes incorrect global-usings file (renamed/consolidated). |
| src/CoreEx.UnitTesting/GlobalUsing.cs | Consolidates/updates global usings (including logger availability). |
| src/CoreEx.Template/README.md | Documents .claude/commands wrappers (including scaffold) emitted by template. |
| src/CoreEx.Template/content/CoreEx.Subscribe/src/app-name.Subscribe/Program.cs | Updates template host health-check mapping and related behavior. |
| src/CoreEx.Template/content/CoreEx.Subscribe/src/app-name.Subscribe/appsettings.Development.json | Fixes incorrect conditional guard for Npgsql logging configuration. |
| src/CoreEx.Template/content/CoreEx.Relay/src/app-name.Relay/Program.cs | Updates relay template middleware/health-check mapping (notably authorization + detailed endpoints). |
| src/CoreEx.Template/content/CoreEx.Relay/src/app-name.Relay/appsettings.Development.json | Removes unused log-level blocks and corrects ordering/conditions. |
| src/CoreEx.Template/content/CoreEx.Core/docker-compose.yml | Fixes template compose project name transformation for dotted names (kebab-case). |
| src/CoreEx.Template/content/CoreEx.Core/.template.config/template.json | Adds/uses kebab-case transform and dot-to-kebab transform for derived symbols. |
| src/CoreEx.Template/content/CoreEx.Api/src/app-name.Api/Program.cs | Updates API template health-check mapping behavior for detailed endpoints. |
| src/CoreEx.Template/content/CoreEx.Api/src/app-name.Api/appsettings.Development.json | Fixes conditional guard drift and removes dead/unreachable template logging blocks. |
| src/CoreEx.Events/Subscribing/SubscribedManager.cs | Tightens cancellation classification to only treat cancellations from the receiver token as bubble-up. |
| src/CoreEx.Events/Subscribing/SubscribedBase.Static.cs | Makes glob matching culture-invariant by default; clarifies intent in docs. |
| src/CoreEx.Events/Subscribing/SubscribeAttribute.cs | Corrects match logic for explicit Regex vs glob pattern usage. |
| src/CoreEx.Events/Publishing/EventPublisherBase.cs | Removes debug payload logging that could leak sensitive event data. |
| src/CoreEx.Events/GlobalUsing.cs | Adds missing System.Globalization import used by culture-invariant casing. |
| src/CoreEx.Events/EventData.With.cs | Fixes exclude-paths empty handling logic. |
| src/CoreEx.DomainDriven/EntityBase.cs | Uses NotSupportedException for unsupported identifier members and fixes Remove mutation ordering. |
| src/CoreEx.DomainDriven/Aggregate.cs | Enforces read-only guard on event mutation APIs. |
| src/CoreEx.Data/README.md | Fixes API/documentation drift for IUnitOfWork and reference-data model properties. |
| src/CoreEx.Data/Querying/QueryFilterParserWriter.cs | Fixes argument placeholder renumbering collisions and introduces regex-based renumbering. |
| src/CoreEx.Data/Querying/QueryFilterFieldConfigBase.cs | Improves token validation and error messaging for boolean constants. |
| src/CoreEx.Data/Querying/Expressions/QueryFilterStringFunctionExpression.cs | Adds stricter token validation for function expressions. |
| src/CoreEx.Data/Querying/Expressions/QueryFilterLogicalExpression.cs | Fixes logical-expression token handling edge case. |
| src/CoreEx.Data/GlobalUsing.cs | Adds regex namespace import required by new filter-writer logic. |
| src/CoreEx.Data/AGENTS.md | Fixes unit-of-work transaction example to correctly flow CancellationToken. |
| src/CoreEx.Data.GraphQL/Internal/GraphQLConnectionResolver.cs | Makes pageInfo field handling case-insensitive to match GraphQL expectations. |
| src/CoreEx.Data.GraphQL/Internal/GraphQLArgsMapper.cs | Plumbs QueryFilterParser into where-translation to preserve correct quoting/typing behavior. |
| src/CoreEx.CodeGen/RefData/Templates/IRepository_cs.hbs | Fixes template to emit repository members only for entities with repositories. |
| src/CoreEx.CodeGen/RefData/Config/EntityConfig.cs | Removes duplicated IdType defaulting assignment. |
| src/CoreEx.CodeGen/README.md | Updates docs to match actual generator responsibilities/split. |
| src/CoreEx.CodeGen/AGENTS.md | Corrects example config and output mapping tables to match reality. |
| src/CoreEx.Caching.FusionCache/CoreExFusionCacheExtensions.DependencyInjection.cs | Adds DI-time configure callback for FusionHybridCache construction. |
| src/CoreEx.Azure.Messaging.ServiceBus/ServiceBusReceiverResiliency.cs | Prevents unobserved task exceptions from circuit-breaker pause/resume flow by logging failures. |
| src/CoreEx.Azure.Messaging.ServiceBus/ServiceBusReceiverInvoker.cs | Guards resiliency pipeline against exception outcomes by converting to Result.Fail. |
| src/CoreEx.Azure.Messaging.ServiceBus/Abstractions/ServiceBusReceiverOptionsBase.cs | Fixes/clarifies doc for UnhandledErrorHandling behavior. |
| src/CoreEx.Azure.Messaging.ServiceBus/Abstractions/ServiceBusReceiverBaseT.cs | Improves cancellation handling and logs failures for catastrophic pause to avoid unobserved tasks. |
| src/CoreEx.Azure.Messaging.ServiceBus/Abstractions/ServiceBusMessageActionsBase.cs | Centralizes truncation behavior via ServiceBusReceiverBase constant/logic. |
| src/CoreEx.Azure.Messaging.ServiceBus/Abstractions/ProcessSessionMessageEventArgsActions.cs | Avoids dead-letter metadata exposure by not persisting stack traces into broker metadata. |
| src/CoreEx.Azure.Messaging.ServiceBus/Abstractions/ProcessMessageEventArgsActions.cs | Avoids dead-letter metadata exposure by not persisting stack traces into broker metadata. |
| src/CoreEx.AspNetCore/OpenApiOptions.cs | Fixes/clarifies XML doc for fields request-headers inclusion. |
| src/CoreEx.AspNetCore/Mvc/WebApiInvoker.cs | Removes redundant file-level using after GlobalUsing consolidation. |
| src/CoreEx.AspNetCore/Mvc/WebApi.cs | Removes redundant usings; uses constant for IncludeExceptionInProblemDetails config lookup. |
| src/CoreEx.AspNetCore/Mvc/QueryAttribute.cs | Fixes SupportsFilter default remark to match actual default. |
| src/CoreEx.AspNetCore/Mvc/ProducesNotFoundProblemAttribute.cs | Fixes remark to correctly state 404 rather than 200. |
| src/CoreEx.AspNetCore/Idempotency/IIdempotencyProvider.cs | Removes redundant using after GlobalUsing consolidation. |
| src/CoreEx.AspNetCore/Idempotency/IdempotencyKey.cs | Fixes replay header handling and hashes request bodies in chunks to avoid unbounded buffering. |
| src/CoreEx.AspNetCore/Http/WebApi.cs | Removes redundant using after GlobalUsing consolidation. |
| src/CoreEx.AspNetCore/Http/AspNetCoreHttpExtensions.cs | Removes redundant usings after GlobalUsing consolidation. |
| src/CoreEx.AspNetCore/HealthChecks/README.md | Updates docs to reflect detailed endpoints being disabled by default. |
| src/CoreEx.AspNetCore/HealthChecks/HealthCheckOptions.cs | Changes default for detailed endpoints to false (secure by default) and clarifies config invocation semantics. |
| src/CoreEx.AspNetCore/GlobalUsing.cs | Consolidates usings into project GlobalUsing (incl. Mvc, Options, aliases). |
| src/CoreEx.AspNetCore/ExecutionContextMiddleware.cs | Prevents header mutation after response start. |
| src/CoreEx.AspNetCore/CoreExAspNetCoreExtensions.OpenTelemetry.cs | Removes redundant null-check and ensures correct fluent return. |
| src/CoreEx.AspNetCore/CoreExAspNetCoreExtensions.GraphQLLite.cs | Adds GraphQL-lite error codes for syntax/query-required errors. |
| src/CoreEx.AspNetCore/CoreExAspNetCoreExtensions.DependencyInjection.cs | Fixes doc typo for IdempotencyKeyMiddleware registration. |
| src/CoreEx.AspNetCore/AGENTS.md | Documents new secure-by-default detailed health-check behavior and opt-in pattern. |
| src/CoreEx.AspNetCore/Abstractions/WebApiPagingResult.cs | Prevents overflow in prev/next paging computations by using long arithmetic + clamping. |
| src/CoreEx.AspNetCore/Abstractions/WebApiBase.cs | Makes delete not-found conversion configurable (settable). |
| src/CoreEx.AspNetCore/Abstractions/WebApi.MergePatch.cs | Tightens merge-patch content-type enforcement and updates error messaging. |
| src/CoreEx.AspNetCore/Abstractions/WebApi.Delete.cs | Adds missing ConfigureAwait(false) to async path. |
| src/CoreEx.AspNetCore/Abstractions/WebApi.cs | Ensures own-token cancellation bubbles even when converting unhandled exceptions to ProblemDetails. |
| src/CoreEx.AspNetCore.NSwag/CoreExNSwagExtensions.DependencyInjection.cs | Adds configuration callback to flow JsonSerializerOptions into schema generation. |
| samples/src/Contoso.Shopping.Subscribe/Program.cs | Updates health-check mapping to explicitly opt into detailed endpoints. |
| samples/src/Contoso.Shopping.Relay/Program.cs | Updates health-check mapping to explicitly opt into detailed endpoints. |
| samples/src/Contoso.Shopping.Api/Program.cs | Updates health-check mapping to explicitly opt into detailed endpoints. |
| samples/src/Contoso.Products.Subscribe/Program.cs | Updates health-check mapping to explicitly opt into detailed endpoints. |
| samples/src/Contoso.Products.Relay/Program.cs | Updates health-check mapping to explicitly opt into detailed endpoints. |
| samples/src/Contoso.Products.Api/Program.cs | Updates health-check mapping to explicitly opt into detailed endpoints. |
| samples/src/Contoso.Orders.Api/Program.cs | Updates health-check mapping to explicitly opt into detailed endpoints. |
| gen/CoreEx.Generator/Templates/ReferenceData.cs.hb | Adds conditional text property emission and optional JsonPropertyName override support. |
| gen/CoreEx.Generator/Templates/Contract.cs.hb | Adds conditional text property emission and optional JsonPropertyName override support. |
| gen/CoreEx.Generator/PropertyModel.cs | Adds HasRefDataTextJsonName and minor hash-code formatting fix. |
| gen/CoreEx.Generator/ContractModel.cs | Implements [ReferenceData] Text/TextJsonName handling in model creation. |
| docs/getting-started.md | Updates docs to reflect scaffold command availability in Claude Code too. |
| CoreEx.slnx | Adds new CoreEx.UnitTesting.Test.Unit project to solution. |
| CoreEx.Core.Test.Parallel.slnf | Adds new CoreEx.UnitTesting.Test.Unit project to core parallel test filter. |
| CoreEx.Core.slnf | Adds new CoreEx.UnitTesting.Test.Unit project to core solution filter. |
| consumer-instructions/README.md | Clarifies Claude Code command wrapper requirement for /coreex-scaffold. |
| .github/skills/coreex-test-api/references/workflow.md | Documents the new AssertWithValue(valueFactory, ...) overload usage. |
| .github/instructions/coreex-event-subscribers.instructions.md | Updates guidance snippet for secure-by-default detailed health checks. |
| .claude/commands/coreex-scaffold.md | Adds missing Claude Code command wrapper for the solution scaffolder skill. |
Suppressed comments (1)
src/CoreEx.Template/content/CoreEx.Relay/src/app-name.Relay/Program.cs:81
- The relay template enables detailed health-check endpoints by default. Now that HealthCheckOptions.AreDetailedEndpointsEnabled defaults to false for security, scaffolds should not opt in to detailed endpoints unless they’re also secured (e.g. RequireAuthorization). Consider leaving the default (disabled) and documenting how to enable + secure when needed.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
… in templates/samples - CoreEx.Template: Relay's Program.cs enabled UseAuthorization() without registering authorization services (no AddControllers() to bring them in transitively like Api/Subscribe) - crashed at startup. Added AddAuthorization() explicitly. - CoreEx.Template + samples: removed the AreDetailedEndpointsEnabled = true override from all host Program.cs files so scaffolded/sample apps inherit the secure-by-default health check behavior; preserved existing Health_Detailed test coverage via a CoreEx.AspNetCore.HealthChecks config section in each affected test project's appsettings.unittest.json. - tests/CoreEx.AspNetCore.Test.Api: OtherController now extends ControllerBase instead of Controller, matching sibling test controllers. - CoreEx.CodeGen: reverted the IRepository_cs.hbs change from the previous commit - the interface-declares-all/implementation-declares-some asymmetry for repository: None entities is intentional (forces a manual partial-class implementation), not a bug. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 196 out of 196 changed files in this pull request and generated no new comments.
Suppressed comments (3)
src/CoreEx.UnitTesting/UnitTestExExtensions.ServiceBus.cs:32
- CoreEx library code convention is to use ConfigureAwait(false) on awaited calls. These SDK awaits currently omit it, which can lead to unnecessary context captures in non-test consumers.
This issue also appears on line 79 of the same file.
src/CoreEx.UnitTesting/UnitTestExExtensions.ServiceBus.cs:83
- CoreEx library code convention is to use ConfigureAwait(false) on awaited calls. These session receiver awaits currently omit it, which can lead to unnecessary context captures in non-test consumers.
src/CoreEx.Azure.Messaging.ServiceBus/ServiceBusReceiverResiliency.cs:61 - Use CoreEx's ambient clock (Runtime.UtcNow) instead of DateTimeOffset.UtcNow so pause/resume timestamps are ExecutionContext-aware and deterministic in tests.
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
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.
Summary
A full RPI (Research → Plan → Implement) code-review pass across every package in the repo — 15 commits, ~190 files. Each package was researched for real bugs (not style nits), verified against actual behavior (empirical repro where possible, revert-and-reproduce for the highest-priority fixes), fixed, and covered with regression tests. Breakdown below by project.
CoreEx.Events
OperationCanceledExceptions are handled normally.EventFormatterappliesInvariantCulturecasing forTitle/Source(fixes non-deterministic behavior under cultures like Turkish "I").SubscribeAttribute/SubscribedBaseglob matching now usesRegexOptions.CultureInvariant.EventSubscriberMetricsnow correctly classifies everyIEventSubscriberExceptionoutcome (previously most fell through to a generic bucket) and records asubscribedtag.ErrorHandler.AddAssignableFromoverloads; removed a debug-only event-payload log leak fromEventPublisherBase(moved to test-only viaEventPublisherDecorator).CoreEx.Azure.Messaging.ServiceBus
StopAsyncinvokingOnStopAsynceven when the receiver was never started..ConfigureAwait(false).ServiceBusErrorClassifiertreatsSessionCannotBeLockedas info, not a lock-lost condition.WithKeyedSubscriber(...).WithHostedService()to fail to resolve."$none"session (not a random GUID) — bounds session growth and preserves ordering.CloudEventtrace context/identity attributes are now preserved in Binary-mode round-trips even whenincludeAttributesis false.CoreEx.Caching.FusionCache
ICacheKeyProvider— previously unqualified tags could cause cross-tenant/cross-domain cache invalidation when multiple domains shared oneIFusionCache.ConfigureEntryOptionsaccepts bothHybridCacheEntryOptionsandFusionCacheEntryOptions;AddFusionHybridCachesupports a DI-time configure callback.CoreEx.AspNetCore
MapHealthChecks's detailed endpoints are now disabled by default (previously anonymous-by-default, exposing fullHealthReportdiagnostics); a warning is logged if enabled without securing them.MapHostedServices(pause/resume/status endpoints for background services) now logs a warning if mapped without a securing delegate — previously silent and unauthenticated by default. Both<remarks>now clarify that only the absence of a configure delegate is checked, not whether a supplied one is actually effective.Set-Cookieleak in the idempotency-key replay path; request body is now hashed in chunks instead of buffered wholesale; tracing tags are set after key validation, not before.longarithmetic to prevent overflow in next/prev links.GlobalUsing.csconsolidated per house convention.CoreEx.AspNetCore.NSwag
AcceptsAttribute.IsOptionalnow actually wires through toOpenApiRequestBody.IsRequired(previously declared but ignored).AddCoreExConfigurationaccepts anAction<OpenApiOptions>so customJsonSerializerOptionspropagate to schema generation..Add(...), preventing duplicate-key exceptions.ProducesNotFoundProblemAttributeis 404 not 200;QueryAttribute.SupportsFilterdefaultstruenotfalse).CoreEx.Data
QueryFilterParsernow rejects duplicate field registrations and correctly supports both 32- and 36-character GUID formats.QueryOrderByParserenforces its direction allow-list correctly and fixes default-ordering format.CoreEx.Data.GraphQL
GetIdentifier<TId>now correctly converts boxed variable-supplied values (e.g.long→int,string→Guid) instead of an unsafe direct cast.QueryFilterParserfor correct OData quoting ofGuid/DateTime/etc. (previously always quoted as string, breaking non-string filters) — with injection-safety guards on the unquoted path.decimalfilter values now round-trip viadecimalfirst (previously always widened throughdouble, losing precision).EnableSensitiveDataLogging), not on by default.CoreEx.DomainDriven
Aggregate<TId,TSelf>.AddEvent/ClearEventsnow correctly throw when the aggregate is read-only (previously silently allowed mutation).EntityBase.Removenow fires theMutatedevent before marking the entity read-only, matching its own documented contract (previously inverted).CoreEx.Generator
TextJsonNamesupport on[ReferenceData]for a customizable JSON name on the generated "Text" property, and the generator now correctly omits the property entirely whenText = false(previously always emitted regardless of the flag).CoreEx.UnitTesting
CoreEx.UnitTesting.Test.Unittest project — this package had no dedicated tests despite being the thing every other test suite trusts for correct assertions.ExpectIdentifier's non-default-value check (compared the entity's type instead of the identifier's type — never worked forGuid/int/longIDs).ExpectChangeLogCreated/Updatedthat checked the wrong field.ETag/Identifierexpectation casts to use the read-only interfaces actually verified, not the stricter mutable ones.ExpectChangeLogCreated/Updatedwhere an explicitly-emptycreatedBy/updatedBysilently matched any actual value.GetAndClearAzureServiceBusAsync's internal poll timeout from 1ms to 1s to avoid dropping messages under real broker latency (verified against the live emulator).AssertWithValue(valueFactory, ...)factory overload (for testers with no returned value to assert against) with tests and doc coverage.CoreEx.CodeGen
AGENTS.md's example config and the generator/template/output-mapping tables acrossAGENTS.md/README.md(previously described aRootGenerator/ApiGeneratorsplit that didn't match reality).IRepository_cs.hbsto filterrepository: Noneentities out of the generated interface, matching the implementation template. That was reverted after review — the asymmetry is intentional (the interface declares every entity's member as a compile-time forcing function sorepository: Noneentities must be hand-implemented in a partial-class extension; only the auto-generatable implementation is filtered). No functional change remains in this package from this PR beyond the doc corrections.CoreEx.Template
coreex-relayscaffold leftapp.UseAuthorization()commented out (unlikecoreex-api/coreex-subscribe) — meant theRequireAuthorization()guidance onMapHealthChecks/MapHostedServicessilently did nothing even if uncommented. Now enabled by default, matching the other hosts — withAddAuthorization()also registered explicitly, since Relay (unlike Api/Subscribe) has noAddControllers()to bring that in transitively (caught in review; verified by actually scaffolding, building, and running the host).coreex-api/coreex-relay/coreex-subscribeno longer overrideHealthCheckOptions.AreDetailedEndpointsEnabledtotrue— they now inherit the library's secure-by-default behavior. ExistingHealth_Detailedtest coverage is preserved via aCoreEx.AspNetCore.HealthChecksconfig section in each template'sappsettings.unittest.json(test-only; the generated app itself stays secure). The same change was applied to all 7 Contoso sample hosts for consistency.#if implement-sqlserver/implement-postgresguard on the Npgsql log-suppression entry incoreex-api/coreex-subscribe(copy-paste drift fromcoreex-relay, which had it correct) — regression-tested via two newvalidate-template-pack.ps1scenarios.implement-servicebusconditional incoreex-api(which has no messaging-provider parameter at all) and misspelled, unused FusionCache/Redis log config incoreex-relay(which has no such packages).docker-compose.yml's project name only being lower-cased (not kebab-cased) for dotted project names — rejected by podman. Addedvalidate-template-pack.ps1coverage for this..claude/commands/coreex-scaffold.mdwrapper so/coreex-scaffoldactually works in Claude Code (previously only worked in Copilot Chat despite being documented as available in both) — aligned every doc that referenced it (getting-started.md,consumer-instructions/README.md,CoreEx.Template/README.md).Response to Copilot review
Copilot flagged four issues on this PR; all addressed:
coreex-relay's enabledUseAuthorization()had noAddAuthorization()behind it — real startup crash, fixed and verified by actually running the scaffolded host (see CoreEx.Template section above).coreex-api/coreex-relay/coreex-subscribeopted into detailed health-check endpoints by default, undermining the library's new secure-by-default posture — fixed for both templates and samples (see CoreEx.Template section above).tests/CoreEx.AspNetCore.Test.Api/Controllers/OtherController.csextendedControllerinstead ofControllerBase(pre-existing, unrelated to this PR's changes, but cheap to fix) — nowControllerBase.CoreEx.Validation
ValueFormatteralways honors the providedFormatProvider.PropertyContext.RefreshFromEntity()added to fix struct value sync in rule chains.ComparePropertyRule/DecimalRulecorrectly handle numeric overflow as a validation error instead of an unhandled exception.CompareValuesRule'soverrideValueWhereMatchedbug fixed and overrides now propagate correctly.MandatoryRule/NullNoneEmptyRuledispose enumerators from non-collectionIEnumerablesources.ValidatingInlineValidatornow uses.ConfigureAwait(false)consistently.Verification
Every package's fixes were verified with
dotnet build/dotnet test(full suites green), and the highest-priority/highest-risk fixes used revert-and-reproduce (confirm the regression test fails against the pre-fix code, then reapply).CoreEx.Templatewas additionally verified viatools/validate-template-pack.ps1(14/14 scenarios passing, rebuilt from source) and manual scaffold+inspect for the security and doc fixes. The Relay authorization fix was verified by scaffolding a fullcoreex+coreex-relaysolution, building it, and actually running it — confirmed clean startup,/health/live→ 200,/health/live/detailed→ 404 (secure by default), and/hosted-services/all/statusreachable with the expected unsecured-warning logged. Full solution build (CoreEx.sln) passes with 0 warnings/errors.🤖 Generated with Claude Code