Fix NRT code smells: QueueEntry, LogEntry, conventions by niemyjski · Pull Request #498 · FoundatioFx/Foundatio

niemyjski · 2026-04-22T22:36:26Z

Summary

QueueEntry.EntryType: Changed from non-nullable Type to Type? on both IQueueEntry and QueueEntry<T>. Poison messages have Value == null, so Value?.GetType() legitimately returns null. All callers already use ?. defensively (e.g., entry.EntryType?.Name). Updated the test wrapper in InMemoryQueueTests.cs to match.
LogEntry: Replace = null! initializers with required keyword on CategoryName, Scopes, and Formatter properties (both xunit and xunit.v3). This is an intentional source-breaking change -- callers must provide these values. Both internal construction sites already set all three properties.
AGENTS.md: Document acceptable NRT patterns (expression tree !, entity null!, post-filter !, vendored code) so future contributors know which ! usages are intentional vs code smells.

Test plan

CI passes -- 0 warnings, 0 errors on dotnet build
Existing tests pass with no regressions
QueueEntry.EntryType Type? doesn't break callers (verified: all callers use ?. defensively)

…onventions docs - QueueEntry.EntryType: throw InvalidOperationException instead of ! suppression when Value is null (poison message) - LogEntry: use required keyword instead of null! for CategoryName, Scopes, Formatter (both xunit and xunit.v3 variants) - AGENTS.md: document acceptable NRT patterns (expression tree !, entity null!, post-filter !, vendored code)

Copilot

Pull request overview

This PR aims to reduce nullable-reference-type (NRT) “code smells” across Foundatio by tightening null semantics in queue/log helper types and documenting accepted NRT patterns for contributors.

Changes:

Update QueueEntry<T>.EntryType to stop using null-forgiving and instead throw when Value is null (poison messages).
Replace = null! initializers in LogEntry (xunit + xunit.v3) with required members.
Add documented NRT conventions to AGENTS.md to clarify when !/null! are acceptable.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
src/Foundatio/Queues/QueueEntry.cs	Changes poison-message behavior for `EntryType` to throw when `Value` is null.
src/Foundatio.Xunit/Logging/LogEntry.cs	Uses `required` members instead of `null!` for key properties.
src/Foundatio.Xunit.v3/Logging/LogEntry.cs	Same `required` member change for the v3 package.
AGENTS.md	Documents repo-specific NRT patterns and guidance for `!` usage.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

…issing index/type Foundatio: - QueueEntry.EntryType: change to Type? instead of throwing -- Copilot correctly identified that StartProcessQueueEntryActivity accesses EntryType?.Name BEFORE the poison message check, so throwing would break poison message handling - IQueueEntry.EntryType: update interface to Type? to match - Test wrapper: update to match new Type? return Foundatio.Repositories: - IOptions.Get<T>/SafeGetOption: add [return: NotNullIfNotNull(nameof(defaultValue))] so callers passing non-null defaults don't need ! suppressions - Remove all 6 ! suppressions in IOptions.cs that were only compensating for the missing NotNullIfNotNull - Remove ! from TimeProviderOptions.cs and ElasticCommandOptions.ParentDocumentType - ChildQueryBuilder: throw InvalidOperationException if index is null (not silent return) - ParentQueryBuilder: throw InvalidOperationException if index is null when parent queries exist - SoftDeletesQueryBuilder: throw InvalidOperationException if DocumentType is null when SupportsSoftDeletes is enabled

Copilot

Pull request overview

This PR tightens nullable reference type (NRT) correctness across queue entries and test logging utilities, and documents agreed-upon NRT patterns to reduce future ! “code smell” usage.

Changes:

Make IQueueEntry.EntryType nullable (Type?) and update QueueEntry<T> + a test wrapper accordingly.
Switch LogEntry (xunit + xunit.v3) from = null! initializers to required properties.
Document NRT conventions in AGENTS.md to clarify when ! and null! are acceptable.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
tests/Foundatio.Tests/Queue/InMemoryQueueTests.cs	Updates test wrapper to match nullable `EntryType`.
src/Foundatio/Queues/QueueEntry.cs	Makes `EntryType` return nullable `Type?` instead of using `!`.
src/Foundatio/Queues/IQueueEntry.cs	Updates interface contract/docs: `EntryType` is now nullable.
src/Foundatio.Xunit/Logging/LogEntry.cs	Replaces `null!` defaults with `required` for key properties.
src/Foundatio.Xunit.v3/Logging/LogEntry.cs	Same `required` change for xunit.v3 variant.
AGENTS.md	Adds documented NRT patterns/conventions for contributors.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

niemyjski · 2026-04-22T23:18:50Z

+    public required string CategoryName { get; set; }
    public LogLevel LogLevel { get; set; }
-    public object[] Scopes { get; set; } = null!;
+    public required object[] Scopes { get; set; }
    public EventId EventId { get; set; }
    public object? State { get; set; }
    public Exception? Exception { get; set; }
-    public Func<object?, Exception?, string> Formatter { get; set; } = null!;
+    public required Func<object?, Exception?, string> Formatter { get; set; }


Good catch. Replaced
equired\ with non-null defaults: \string.Empty\ for CategoryName, []\ for Scopes, and a static default formatter. This preserves NRT safety without breaking downstream consumers who construct \LogEntry\ via object initializer. Fixed in e8b5904.

Reverted back to
equired. This is an intentional breaking change -- \LogEntry\ callers should be forced to provide these values. The = null!\ was the real problem.

niemyjski · 2026-04-22T23:18:51Z

+    public required string CategoryName { get; set; }
    public LogLevel LogLevel { get; set; }
-    public object[] Scopes { get; set; } = null!;
+    public required object[] Scopes { get; set; }
    public EventId EventId { get; set; }
    public object? State { get; set; }
    public Exception? Exception { get; set; }
-    public Func<object?, Exception?, string> Formatter { get; set; } = null!;
+    public required Func<object?, Exception?, string> Formatter { get; set; }


Same fix applied here -- replaced
equired\ with non-null defaults (\string.Empty, [], default formatter). Both xunit and xunit.v3 LogEntry are now consistent and non-breaking. Fixed in e8b5904.

Same -- reverted to
equired. Intentional breaking change.

niemyjski · 2026-04-22T23:18:52Z

+    public Type? EntryType => Value?.GetType();
    public object GetValue() => Value;


Updated the PR description to accurately reflect the \Type?\ return behavior instead of the stale throw description. Done.

…void source-breaking API change

Copilot

Pull request overview

This PR tightens nullable reference type (NRT) correctness across queue entries and test logging utilities, aligning the public APIs with real runtime behavior (notably poison queue entries) and documenting accepted NRT suppression patterns for contributors.

Changes:

Make IQueueEntry.EntryType / QueueEntry<T>.EntryType nullable (Type?) to support poison entries with Value == null.
Replace null! initializers in LogEntry (xunit + xunit.v3) with safe non-null defaults.
Add documented NRT conventions to AGENTS.md to clarify when !/null! is acceptable.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
tests/Foundatio.Tests/Queue/InMemoryQueueTests.cs	Updates a test wrapper to match `EntryType` nullability.
src/Foundatio/Queues/QueueEntry.cs	Makes `EntryType` return `null` when `Value` is `null` instead of using `!`.
src/Foundatio/Queues/IQueueEntry.cs	Updates `EntryType` to `Type?` and adjusts the XML docs for poison entries.
src/Foundatio.Xunit/Logging/LogEntry.cs	Replaces `null!` defaults with `string.Empty`, empty arrays, and a default formatter.
src/Foundatio.Xunit.v3/Logging/LogEntry.cs	Mirrors the same safe defaults as the xunit package.
AGENTS.md	Documents repo guidance for NRT suppression patterns.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

+The codebase has NRT enabled. Follow these patterns:
+
+- **Entity/model properties** (`= null!`): Acceptable for properties set by deserialization or ORM frameworks. Prefer `required` keyword where construction is controlled.
+- **Expression tree `!`**: Required when expressions like `SortDescending(x => x.DateCreatedUtc!)` box `DateTime?` to `object?` but the delegate expects `object`. The value is never evaluated -- only used for field path extraction. This is correct and unavoidable.


niemyjski requested review from Copilot and ejsmith April 22, 2026 22:36

niemyjski self-assigned this Apr 22, 2026

Copilot started reviewing on behalf of niemyjski April 22, 2026 22:59 View session

Copilot AI reviewed Apr 22, 2026

View reviewed changes

Comment thread src/Foundatio/Queues/QueueEntry.cs Outdated

niemyjski requested a review from Copilot April 22, 2026 23:09

Copilot started reviewing on behalf of niemyjski April 22, 2026 23:09 View session

Copilot AI reviewed Apr 22, 2026

View reviewed changes

fix: replace required keyword with non-null defaults on LogEntry to a…

e8b5904

…void source-breaking API change

niemyjski requested a review from Copilot April 22, 2026 23:58

Copilot started reviewing on behalf of niemyjski April 22, 2026 23:58 View session

Copilot AI reviewed Apr 23, 2026

View reviewed changes

revert: restore required keyword on LogEntry properties

b463046

niemyjski merged commit 9d7d3cd into main Apr 23, 2026
4 checks passed

niemyjski deleted the fix/nrt-code-smells branch April 23, 2026 00:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix NRT code smells: QueueEntry, LogEntry, conventions#498

Fix NRT code smells: QueueEntry, LogEntry, conventions#498
niemyjski merged 4 commits into
mainfrom
fix/nrt-code-smells

niemyjski commented Apr 22, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

niemyjski Apr 22, 2026

Uh oh!

niemyjski Apr 23, 2026

Uh oh!

niemyjski Apr 22, 2026

Uh oh!

niemyjski Apr 23, 2026

Uh oh!

niemyjski Apr 22, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		public Type? EntryType => Value?.GetType();
		public object GetValue() => Value;

Uh oh!

Conversation

niemyjski commented Apr 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

niemyjski Apr 22, 2026

Choose a reason for hiding this comment

Uh oh!

niemyjski Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

niemyjski Apr 22, 2026

Choose a reason for hiding this comment

Uh oh!

niemyjski Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

niemyjski Apr 22, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

niemyjski commented Apr 22, 2026 •

edited

Loading