Improve Context Propagation and Payload Analysis - #49
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThe PR replaces interface-based job context extractors with abstract base classes, adds recursive JSON serializability validation and diagnostics, refactors generator metadata and queue processing, updates related tests and snapshots, and changes repository configuration. ChangesSerialization and context pipeline
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant JobSource
participant PayloadAnalyzer
participant PayloadValidation
participant Generator
participant JsonMetadataEmitter
JobSource->>PayloadAnalyzer: analyze job and context symbols
PayloadAnalyzer->>PayloadValidation: validate serializability
PayloadValidation-->>PayloadAnalyzer: return validation result and locations
JobSource->>Generator: transform job attributes
Generator->>PayloadValidation: validate payload and context types
Generator->>JsonMetadataEmitter: create JSON metadata
JsonMetadataEmitter-->>Generator: return metadata model
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
tests/Immediate.Jobs.Tests/GeneratorTests/ImmediateJobsGeneratorTests.cs (1)
331-332: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winIncremental-caching coverage was dropped with the two removed tests.
EditingContextShapeInvalidatesOnlyOwningJobModelandEditingExtractorContractInvalidatesOnlyReferencingJobModelwere the only assertions that context/extractor edits invalidate exactly the owning job model. Since this PR reworks how the transform reads context types, a caching regression (over- or under-invalidation) now has no test guarding it. Please re-add equivalents against the new extractor contract.Want me to draft replacements for the new base-class shape?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Immediate.Jobs.Tests/GeneratorTests/ImmediateJobsGeneratorTests.cs` around lines 331 - 332, Re-add incremental-caching tests equivalent to EditingContextShapeInvalidatesOnlyOwningJobModel and EditingExtractorContractInvalidatesOnlyReferencingJobModel, adapted to the new extractor contract/base-class shape. Verify that context or extractor edits invalidate exactly the owning or referencing job model, respectively, without over- or under-invalidation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Common/ITypeSymbolExtensions.cs`:
- Around line 116-117: Update the RootNamespace property in
ITypeSymbolExtensions to null-propagate ContainingNamespace before accessing
RootNamespace, ensuring array symbols with no containing namespace return null
safely. Preserve the existing namespace resolution behavior for symbols that
have a containing namespace, including callers such as CreateTypeModel.
In `@src/Common/PayloadValidation.cs`:
- Around line 39-44: Update the type-argument and member traversal in the
relevant validation method so a false result from Visit is retained even when
reportError is supplied. Track whether any argument or member fails, continue
reporting remaining failures as needed, and return the accumulated failure
status at the method’s final return instead of unconditionally returning true.
- Around line 58-62: Update the rootNamespace == "System" handling in
PayloadValidation to avoid rejecting supported named collection shapes such as
List<T>, Dictionary<,>, and ImmutableArray<T>; add an allow path consistent with
the existing array handling. If named collections are intentionally unsupported,
retain the rejection and revise the reportError diagnostic to explicitly
describe that restriction.
In `@src/Immediate.Jobs.Generators/JsonMetadataEmitter.cs`:
- Around line 54-58: Update the HasParameterlessCreator assignment in the
metadata object initializer to require an accessible parameterless constructor,
rather than treating constructor == null as sufficient. Use the constructor
lookup result and its parameter list so ObjectCreator is emitted only when a
public zero-argument constructor exists.
---
Nitpick comments:
In `@tests/Immediate.Jobs.Tests/GeneratorTests/ImmediateJobsGeneratorTests.cs`:
- Around line 331-332: Re-add incremental-caching tests equivalent to
EditingContextShapeInvalidatesOnlyOwningJobModel and
EditingExtractorContractInvalidatesOnlyReferencingJobModel, adapted to the new
extractor contract/base-class shape. Verify that context or extractor edits
invalidate exactly the owning or referencing job model, respectively, without
over- or under-invalidation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 01937bf3-a938-481e-b68b-1cec7f2482ab
📒 Files selected for processing (35)
.editorconfigImmediate.Jobs.slnxcoderabbit.ymlsamples/Aspire/Api/Context/RequestContext.cssamples/NativeAot/Program.cssrc/Common/ITypeSymbolExtensions.cssrc/Common/JobDiscovery.cssrc/Common/PayloadValidation.cssrc/Immediate.Jobs.Analyzers/DiagnosticDescriptors.cssrc/Immediate.Jobs.Analyzers/DiagnosticIds.cssrc/Immediate.Jobs.Analyzers/ImmediateJobsAnalyzer.cssrc/Immediate.Jobs.Analyzers/JobClassAnalyzer.cssrc/Immediate.Jobs.Analyzers/PayloadAnalyzer.cssrc/Immediate.Jobs.Generators/ImmediateJobsGenerator.Transform.cssrc/Immediate.Jobs.Generators/JsonMetadataEmitter.cssrc/Immediate.Jobs.Generators/Templates/Job.sbntxtsrc/Immediate.Jobs.Shared/JobContextPropagation.cstests/Immediate.Jobs.FunctionalTests/ContextPropagationTests.cstests/Immediate.Jobs.Tests/AnalyzerTests/ImmediateJobsAnalyzerTests.cstests/Immediate.Jobs.Tests/AnalyzerTests/PayloadAnalyzerTests.cstests/Immediate.Jobs.Tests/GeneratorTests/AddJobsTests.cstests/Immediate.Jobs.Tests/GeneratorTests/GeneratorTestHelper.cstests/Immediate.Jobs.Tests/GeneratorTests/ImmediateJobsGeneratorTests.cstests/Immediate.Jobs.Tests/GeneratorTests/InvalidJobsTests.cstests/Immediate.Jobs.Tests/GeneratorTests/Snapshots/AddJobsTests.ServiceCollectionExtensionsUsesQueuesAndTaggedRegistrations_framework=net10.0#IJ..WorkJob.g.verified.cstests/Immediate.Jobs.Tests/GeneratorTests/Snapshots/ImmediateJobsGeneratorTests.ContextExtractorsGenerateOrderedCaptureRestoreMetadataAndScopedRegistrations#IJ..ContextualJob.g.verified.cstests/Immediate.Jobs.Tests/GeneratorTests/Snapshots/ImmediateJobsGeneratorTests.CronJobGeneratesPayloadlessRecurringScheduler#IJ..CleanupSessionsJob.g.verified.cstests/Immediate.Jobs.Tests/GeneratorTests/Snapshots/ImmediateJobsGeneratorTests.ExplicitJobDetailsOnValueTypeUsesConstrainedByReferenceAssignment#IJ..StructJob.g.verified.cstests/Immediate.Jobs.Tests/GeneratorTests/Snapshots/ImmediateJobsGeneratorTests.InvokerDelegatesExecutionToImmediateHandlersPipeline#IJ..WorkJob.g.verified.cstests/Immediate.Jobs.Tests/GeneratorTests/Snapshots/ImmediateJobsGeneratorTests.JobWithoutContextDoesNotEmitCaptureOrRestoreCode#IJ..PlainJob.g.verified.cstests/Immediate.Jobs.Tests/GeneratorTests/Snapshots/ImmediateJobsGeneratorTests.NodaTimeContextUsesConfiguredGeneratedMetadata#IJ..ClockJob.g.verified.cstests/Immediate.Jobs.Tests/GeneratorTests/Snapshots/ImmediateJobsGeneratorTests.PayloadJobGeneratesTypedSchedulerDirectInvokerAndRegistrations#IJ.Example.SendEmailJob.g.verified.cstests/Immediate.Jobs.Tests/GeneratorTests/Snapshots/ImmediateJobsGeneratorTests.PayloadlessJobWithoutCronGeneratesDynamicRecurringScheduler#IJ..TenantCleanupJob.g.verified.cstests/Immediate.Jobs.Tests/GeneratorTests/Snapshots/UnnameableJobsTests.ExplicitNameShouldRescueAnUnderivableClassName#IJ.Dummy.Job.g.verified.cstests/Immediate.Jobs.Tests/GeneratorTests/UnnameableJobsTests.cs
💤 Files with no reviewable changes (5)
- src/Common/JobDiscovery.cs
- src/Immediate.Jobs.Analyzers/DiagnosticDescriptors.cs
- src/Immediate.Jobs.Analyzers/ImmediateJobsAnalyzer.cs
- tests/Immediate.Jobs.Tests/AnalyzerTests/ImmediateJobsAnalyzerTests.cs
- tests/Immediate.Jobs.Tests/GeneratorTests/GeneratorTestHelper.cs
Comment on lines
+54
to
+58
| var constructorParameters = constructor?.Parameters ?? ImmutableArray<IParameterSymbol>.Empty; | ||
|
|
||
| return new() | ||
| { | ||
| HasParameterlessCreator = constructor is null || constructor.Parameters.Length == 0, | ||
| HasParameterlessCreator = constructor is null or { Parameters: [] }, |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how HasParameterlessCreator is consumed in the templates
rg -nP -C4 'HasParameterlessCreator' srcRepository: ImmediatePlatform/Immediate.Jobs
Length of output: 1821
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- JsonMetadataEmitter.cs (relevant sections) ---'
sed -n '1,170p' src/Immediate.Jobs.Generators/JsonMetadataEmitter.cs
echo
echo '--- any other HasParameterlessCreator usages ---'
rg -n 'HasParameterlessCreator' srcRepository: ImmediatePlatform/Immediate.Jobs
Length of output: 6535
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- references to JsonObjectRenderModel / ConstructorParameters / HasParameterlessCreator ---'
rg -n 'JsonObjectRenderModel|ConstructorParameters|HasParameterlessCreator' .
echo
echo '--- files that look like templates or generators ---'
git ls-files | rg '(\.tt$|Template|Generator|Emitter|RenderModel|Writer|CodeGen)'Repository: ImmediatePlatform/Immediate.Jobs
Length of output: 7921
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- JsonMetadataEmitter surrounding model definitions ---'
sed -n '170,260p' src/Immediate.Jobs.Generators/JsonMetadataEmitter.cs
echo
echo '--- render/transform pipeline references to ObjectInfo / metadata ---'
rg -n -C3 'ObjectInfo|JsonMetadata|HasParameterlessCreator|ConstructorParameters' src/Immediate.Jobs.Generators
echo
echo '--- templates ---'
sed -n '1,260p' src/Immediate.Jobs.Generators/Templates/Job.sbntxt
echo
sed -n '1,260p' src/Immediate.Jobs.Generators/Templates/ServiceCollectionExtensions.sbntxtRepository: ImmediatePlatform/Immediate.Jobs
Length of output: 28262
Check for an accessible parameterless ctor here, not constructor is null. GetConstructor returns null for any type with no public member-matching ctor, so this can still emit ObjectCreator = () => new T() for types that don’t actually have a public zero-arg constructor. src/Immediate.Jobs.Generators/JsonMetadataEmitter.cs:58
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/Immediate.Jobs.Generators/JsonMetadataEmitter.cs` around lines 54 - 58,
Update the HasParameterlessCreator assignment in the metadata object initializer
to require an accessible parameterless constructor, rather than treating
constructor == null as sufficient. Use the constructor lookup result and its
parameter list so ObjectCreator is emitted only when a public zero-argument
constructor exists.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@coverage-settings.xml`:
- Around line 4-15: Update the ModulePaths exclusion pattern to also match the
Immediate.Jobs.Analyzers.dll and Immediate.Jobs.Generators.dll assemblies, while
preserving the existing Immediate.Handlers.Generators.dll exclusion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6232ee7a-6ca3-4e41-be74-cb99c8ccc7f5
📒 Files selected for processing (1)
coverage-settings.xml
Coverage Report for CI Build 30488813255Coverage increased (+4.8%) to 79.176%Details
Uncovered Changes
Coverage Regressions27 previously-covered lines in 3 files lost coverage.
Coverage Stats
💛 - Coveralls |
This was referenced Jul 29, 2026
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 by CodeRabbit