fix(http-server-csharp): make optional error properties nullable - #11895
sophia-ramsey wants to merge 4 commits into
Conversation
commit: |
|
All changed packages have been documented.
|
|
You can try these changes here
|
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
8468205 to
f21fecd
Compare
There was a problem hiding this comment.
🟡 Changes recommended
A critical nested-union nullability issue, an inherited-property mismatch, and duplicate fix changesets remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Updates the C# HTTP server emitter to generate nullable optional properties and constructor parameters for error models, with related response handling and tests.
Changes:
- Adds nullable handling for error-model properties and constructors.
- Covers nullable models and error-only responses with tests.
- Adds Chronus changelog entries.
File summaries
| File | Reviewed changes |
|---|---|
packages/http-server-csharp/test/generation.test.ts |
Updates generated constructor expectations. |
packages/http-server-csharp/src/components/models/models.tsx |
Applies nullable handling to error properties; critical nested-union nullability issue remains. |
packages/http-server-csharp/src/components/models/error-models.tsx |
Updates error constructor parameters; moderate inherited-property mismatch remains. |
packages/http-server-csharp/src/components/models/error-models.test.tsx |
Adds error-model nullability coverage. |
packages/http-server-csharp/src/components/controller-action/response-analysis.ts |
Handles error-only response unions as bodyless responses. |
packages/http-server-csharp/src/components/controller-action/controller-action.test.tsx |
Tests error-only response behavior. |
.chronus/changes/sramsey-csharp-optional-error-properties-2026-8-14-17-1-42.md |
Adds a fix changeset; duplicate changesets should be consolidated. |
.chronus/changes/fix-csharp-optional-error-properties-2026-09-08.md |
Documents the related fix. |
Review details
Suppressed comments (2)
.chronus/changes/sramsey-csharp-optional-error-properties-2026-8-14-17-1-42.md:4
- These two added changesets both declare a
fixfor@typespec/http-server-csharp. Keep one changeset per package/change type by combining these messages (or move the separate response-analysis fix to another PR); otherwise this release will contain duplicate package fix entries and the PR description will not reflect all included behavior.
changeKind: fix
packages:
- "@typespec/http-server-csharp"
packages/http-server-csharp/src/components/models/error-models.tsx:68
getAllPropertiesincludes properties declared on non-error base models (model-helpers.ts:255-266), butServerPropertyonly applies the new nullable property treatment when the declaring/rendered class is an error (models.tsx:154-163,213-245). An@errormodel extending a regular base therefore gets a nullable constructor parameter here that is assigned to a non-nullable base property; with#nullable enablethis creates a nullable-flow warning and a mismatched generated contract. Derive the constructor parameter nullability from the declaring property's emitted type, or update both declaration and constructor together, and cover the inherited case.
prop.type.kind === "Union" ? getNullableUnionInnerType(prop.type) : undefined;
const typeExpressionIncludesNullable =
getNullableValueTypeUnionInnerType($, prop.type) !== undefined;
const needsNullable =
!typeExpressionIncludesNullable && (prop.optional || nullableUnionInnerType !== undefined);
- Files reviewed: 8/8 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🟡 Changes recommended
Address the critical header null-handling defect and related scope, inheritance, documentation, and duplicate changeset issues.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (4)
Previously missed (2) — in code that hasn't changed since the last review.
packages/http-server-csharp/src/components/controller-action/response-analysis.ts:77
- This hunk changes controller generation for a separate case: a union with no value/void/bodyless success now emits a 204/NoContent response. The PR title and description only describe nullable error-model properties, so please split this behavior into a separate change or document and intentionally cover this additional output change in the PR scope.
.chronus/changes/fix-csharp-optional-error-properties-2026-09-08.md:4 - Both new changeset files declare the same
fixchange for@typespec/http-server-csharp. The repository changelog guidance requires one entry per package and change type, so combine these two user-facing messages into a single fix entry to avoid duplicate release entries.
packages/http-server-csharp/src/components/models/error-models.tsx:68
- This also marks inherited properties nullable:
getErrorConstructorwalksgetAllProperties, but an optional reference property declared on a non-error base is still emitted byServerPropertywithout?. The existingBase/ApiErrorcase therefore assigns a nullabledataparameter to a non-nullable base property under#nullable enable, producing a nullability warning; only apply this to properties declared by the error model or emit the base property with matching nullability.
const needsNullable =
!typeExpressionIncludesNullable && (prop.optional || nullableUnionInnerType !== undefined);
packages/http-server-csharp/src/components/models/models.tsx:183
- The class-level documentation above still says there is no nullable
?suffix on reference types, which is now false for error models. Update that comment as well so it states the error-model exception instead of contradicting this new clarification.
* No `required` or `[JsonPropertyName]`. Nullable reference types are limited to error models.
- Files reviewed: 8/8 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
The unresolved nullable dictionary issue can cause warnings-as-errors failures; documentation and scope nits also remain.
Review details
Suppressed comments (3)
packages/http-server-csharp/src/components/controller-action/response-analysis.ts:77
- This hunk changes existing emitter output for operations whose return union contains only errors: they now generate a 204/no-body action instead of the previous value response. The separate changeset documents that behavior, but the PR title and description only cover error-model nullability; please split this independent fix or document its scope and rationale in the PR.
if (hasVoidSuccess || !hasValueSuccess) {
packages/http-server-csharp/src/components/models/error-models.tsx:68
- When this branch handles an optional
@header, it emits astring?constructor parameter and then inserts it intoheaders: new().HttpServiceExceptionstill declaresDictionary<string, string>for both the constructor andHeadersproperty, so the generated file is emitted under#nullable enablewith a possible-null-to-nonnullable value warning (and fails projects that treat warnings as errors). Make the exception/header dictionary types acceptstring?consistently, including the child-error constructor path.
const needsNullable =
!typeExpressionIncludesNullable && (prop.optional || nullableUnionInnerType !== undefined);
packages/http-server-csharp/src/components/models/models.tsx:183
- The class-level documentation above still says this component emits no nullable
?suffixes for reference types, but this new error-model path now does so. Please scope that statement to non-error models (or otherwise update it) so the class and property documentation remain consistent.
* No `required` or `[JsonPropertyName]`. Nullable reference types are limited to error models.
- Files reviewed: 10/11 changed files
- Comments generated: 0 new
- Review effort level: Lite
| IDictionary<string, int> counts = default, | ||
| int attempts = default, | ||
| string[] tags = default, | ||
| string? param = default, |
There was a problem hiding this comment.
actually, is that correct? can optional parameters have default in c#?
There was a problem hiding this comment.
I think yes; the default will end up being null for optional types
This pull request fixes the handling of optional properties on C# error models in the
@typespec/http-server-csharppackage. Now, optional error model properties and their constructor parameters are emitted as nullable types in C#, ensuring better alignment with C# conventions and improved correctness. The changes also ensure that only error models receive this nullable treatment for optional properties, leaving other models unchanged. Comprehensive tests were added to validate this behavior.C# Error Model Nullability Fixes:
string?,int?) when marked optional or explicitly nullable in the TypeSpec model. [1] [2] [3]Testing Improvements:
error-models.test.tsxto verify that optional error properties and constructor parameters are correctly emitted as nullable, and that non-error models are unaffected.Codebase Consistency:
getNullableUnionInnerTypefor improved code clarity and maintainability. [1] [2]