Skip to content

Remove nullable annotation from stored procedure List return types#3294

Draft
Copilot wants to merge 7 commits intomasterfrom
copilot/add-non-nullable-return-types
Draft

Remove nullable annotation from stored procedure List return types#3294
Copilot wants to merge 7 commits intomasterfrom
copilot/add-non-nullable-return-types

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Jan 25, 2026

Implementation Plan for Non-Nullable Stored Procedure Result Sets

Understanding

When nullable reference types are enabled, stored procedures currently return Task<List<T>?> (nullable list). The issue is that:

  1. The list itself should never be null (it should be an empty list instead)
  2. Empty result sets (with no properties) trigger a check in DbExtensions that returns default (null)
  3. This causes unnecessary nullable annotations on the return type

Changes Required

  • Add new CLI/VS configuration option: GenerateEmptyResultType (bool) - Now per-procedure instead of global
  • Update DbContextExtensions template to remove property count check and simplify implementation
  • Remove nullable annotation (?) from List<T> return types when nullable references are enabled
  • Update stored procedure scaffolder to only generate empty result classes when GenerateEmptyResultType is true for that specific procedure
  • Update SQL Server and Postgres procedure scaffolders to handle empty result sets properly
  • Improve error messages in SqlServerRoutineModelFactory for better guidance
  • Update configuration classes, mappers, and schemas
  • Build and test the changes - all 78 tests pass
  • Validate with CLI tool end-to-end
  • Run code review and address feedback
  • Fix CancellationToken to be non-nullable value type
  • Move GenerateEmptyResultType to per-procedure configuration (per reviewer feedback)
  • Fix build: Update file count check to reflect new files
  • Remove trailing whitespaces to resolve lint issues

Key Files Modified

  1. Configuration: StoredProcedure.cs (added per-procedure option), CodeGeneration.cs (removed global option), CliConfigMapper.cs, efcpt-config.schema.json, ReverseEngineerCommandOptions.cs, ReverseEngineerOptions.cs
  2. DbExtensions: src/Core/RevEng.Core.80/DbContextExtensionsSqlQuery
  3. Scaffolders: SqlServerStoredProcedureScaffolder.cs, PostgresStoredProcedureScaffolder.cs, ProcedureScaffolder.cs
  4. Options: ModuleModelFactoryOptions.cs (added dictionary), ModuleScaffolderOptions.cs (removed global), ReverseEngineerScaffolder.cs
  5. Metadata: Routine.cs (added per-routine property), SqlServerRoutineModelFactory.cs
  6. Serialization: SerializationTableModel.cs (added property)
  7. Error messages: SqlServerRoutineModelFactory.cs
  8. Tests: CliConfigMapperTest.cs
  9. Build: .github/workflows/vsix.yml (updated file count check)

Configuration Example

{
  "stored-procedures": [
    {
      "name": "[dbo].[MyProcedure]",
      "generate-empty-result-type": true
    }
  ]
}

Verified Results

✅ Generated stored procedures return Task<List<T>> instead of Task<List<T>?>
✅ CancellationToken is non-nullable (not CancellationToken?)
✅ DbContextExtensions simplified - no longer checks property count
✅ All existing tests pass (78 passed, 4 skipped)
✅ GenerateEmptyResultType is now configured per stored procedure
✅ Code review feedback addressed
✅ Build fixed with correct file count
✅ Trailing whitespaces removed

Original prompt

This section details on the original issue you should resolve

<issue_title>Add option to have nullable reference types but return non-nullable from stored procedures</issue_title>
<issue_description>### What problem are you trying to solve?

With the nullable support, all of my stored procedures return Task<List?>. This may be technically correct, but in reality, my team will find this annoying to have to suppress the null warning for all return values from stored procedures. The vast majority of the time it will not be helpful.

Describe the solution you'd like

It would be great to have an option to make the stored procedures return non-nullable types but still have the other nullable support.

Thank you</issue_description>

Comments on the Issue (you are @copilot in this section)

@ErikEJ @iamwyza this sounds familiar?

Any suggestions for how we can get to that state?

One option could be to stop generating empty result sets perhaps? </comment_new>
<comment_new>@ErikEJ
@iamwyza @omccully

How about this:

Add a new option, and make it default to true:

UseNonNullableModuleResultSets

It will only have effect when UseNullableReferences = true

When set, it will always execute stored procedures with this one liner (no need for the DbExtensions extension method)

return await db.Database.SqlQueryRaw<T>(sql, parameters ?? Array.Empty<object>()).ToListAsync(cancellationToken ?? CancellationToken.None;);

If there is need for generating an empty result set with this option on, an empty result set will be generated, but it will have code injected to cause a build error with advice in comments on how to proceed:

  1. Update the stored procedure to rexturn a proper, discoverable result set. (Link to wiki)
  2. use alternate result set discovery for the procedure (Link to wiki)
  3. remove the procedure from being included in the generated code
  4. Swith to the old behaviour with: UseNonNullableModuleResultSets = false
@ErikEJ I think I will have a close look at the conditions that cause an empty resultset to be generated in the first place. @ErikEJ @iamwyza Or maybe even simpler:

I like "GenerateEmptyResultType": - I think extremely few users actually use this for anything.

So we could just add support for this, and only create the empty result set if this is set, assuming that they will then always add properties to it.

The will allow the returned list from the DbExtensions to be always non-nullable, and we can get rid of the check for property count.

We could improve the error message here to be much more actionable:

https://github.com/ErikEJ/EFCorePowerTools/blob/master/src/Core/RevEng.Core.80/Routines/SqlServerRoutineModelFactory.cs#L113

Suggesting

  • try the other discovery method for that procedure, and how to do it
  • add the GenerateEmptyResultType to handcraft a known result set
@ErikEJ @iamwyza

My new place is doing code first migrations

You just have to guide them to the proper path 😄

I might have a look at this soon - I think we have boiled it down to someting even I can manage,</comment_new>
<comment_new>@ErikEJ
@omccully If you need other post-processing, feel free to create issues so we can consider if they are general enough to be considered addressed.</comment_new>
<comment_new>@ErikEJ
@toglenergy Agree, but the current implementation adds support for result sets with no properties, and this is what this issue is about fixing, as per my proposal here: #3167 (comment) </comment_new>
<comment_new>@ErikEJ
Implementation notes - When nullable is enabled:

  • Task<List<CustOrdersOrdersResult>?> => Task<List<CustOrdersOrdersResult>> (also on interface)
  • Update DbExtension method
  • Improve error message from scaffolder
  • Add stored procedure property to both VS and CLI options - bool: generate-empty-result-type / GenerateEmptyResultType
  • Only generate an empty result class if this is enabled
public static async Task<List<T>> SqlQueryAsync<T>(this DbContext db, string sql, object[]? parameters = null, CancellationToken? cancellationToken = default)
   where T : class
{
    parameters ??= Array.Empty<object>();
    cancellationToken ??= CancellationToken.None;

    return await db.Database
        .SqlQueryRaw<T>(sql, parameters)
        .ToListAsync(cancellationToken.Value);
}

```</body></comment_new>
</comments>

💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

Copilot AI and others added 3 commits January 25, 2026 16:23
…m List return types

- Added GenerateEmptyResultType configuration option to CLI and VS options
- Updated DbContextExtensions template to remove property count check and simplify implementation
- Removed nullable annotation from List<T> return types in stored procedure scaffolders
- Updated ProcedureScaffolder to only generate empty result classes when GenerateEmptyResultType is enabled
- Improved error messages in SqlServerRoutineModelFactory with actionable suggestions
- Updated JSON schema with new configuration option

Co-authored-by: ErikEJ <4169187+ErikEJ@users.noreply.github.com>
…ionality

- Updated CliConfigMapperTest to expect 56 properties (added GenerateEmptyResultType)
- Verified end-to-end CLI tool functionality with NorthwindProcedures.dacpac
- Confirmed generated stored procedures return non-nullable List<T> instead of List<T>?
- Confirmed DbContextExtensions uses simplified implementation without property count check
- All 78 tests pass successfully

Co-authored-by: ErikEJ <4169187+ErikEJ@users.noreply.github.com>
- Changed CancellationToken? to CancellationToken (non-nullable value type)
- Removed unnecessary null-coalescing for CancellationToken
- Removed .Value access on CancellationToken
- Updated both SqlServerStoredProcedureScaffolder and PostgresStoredProcedureScaffolder
- Updated DbContextExtensions template
- All tests still pass
- End-to-end validation successful

Co-authored-by: ErikEJ <4169187+ErikEJ@users.noreply.github.com>
Copilot AI changed the title [WIP] Add option for non-nullable return from stored procedures Remove nullable annotation from stored procedure List return types Jan 25, 2026
Copilot AI requested a review from ErikEJ January 25, 2026 16:36
@ErikEJ
Copy link
Copy Markdown
Owner

ErikEJ commented Jan 28, 2026

@copilot The new config option should apply on procedure level, not generally

@ErikEJ
Copy link
Copy Markdown
Owner

ErikEJ commented Jan 28, 2026

@copilot Update vsix.yaml from findstr "139" filecount.txt to findstr "141" filecount.txt to fix the failing build

- Removed GenerateEmptyResultType from global CodeGeneration config
- Added GenerateEmptyResultType to per-procedure StoredProcedure configuration
- Added ModulesGeneratingEmptyResultTypes dictionary to ModuleModelFactoryOptions
- Added GenerateEmptyResultType property to Routine class
- Updated SqlServerRoutineModelFactory to set per-procedure flag from config
- Updated ProcedureScaffolder to check per-routine flag instead of global option
- Updated SerializationTableModel to include GenerateEmptyResultType property
- Updated CliConfigMapper to map per-procedure settings
- Updated ReverseEngineerScaffolder to build ModulesGeneratingEmptyResultTypes dictionary
- Updated JSON schema to show option at procedure level
- Updated tests to reflect property count changes
- All 78 tests pass

Co-authored-by: ErikEJ <4169187+ErikEJ@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Jan 28, 2026

@copilot The new config option should apply on procedure level, not generally

Changed to per-procedure configuration. The generate-empty-result-type option is now set on individual stored procedures in the config file rather than globally. Commit 89f7370

The addition of new files (GenerateEmptyResultType property and related changes) increased the file count in the efreveng100 build output from 139 to 141 files.

Co-authored-by: ErikEJ <4169187+ErikEJ@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Jan 28, 2026

@copilot Update vsix.yaml from findstr "139" filecount.txt to findstr "141" filecount.txt to fix the failing build

Updated vsix.yml to check for 141 files instead of 139 in efreveng100.exe.zip. The file count increased due to the new files added for the per-procedure GenerateEmptyResultType feature. Commit e3f5c07

@ErikEJ
Copy link
Copy Markdown
Owner

ErikEJ commented Jan 28, 2026

@copilot Remove trailing whitespaces in CliConfigMapper to resolve the build issue

Removed trailing whitespaces on lines 355 and 363 to resolve build/lint issues.

Co-authored-by: ErikEJ <4169187+ErikEJ@users.noreply.github.com>
Copy link
Copy Markdown
Contributor Author

Copilot AI commented Jan 28, 2026

@copilot Remove trailing whitespaces in CliConfigMapper to resolve the build issue

Removed trailing whitespaces from CliConfigMapper.cs (lines 355 and 363). Commit 3015fdc

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add option to have nullable reference types but return non-nullable from stored procedures

2 participants