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

[omccully]

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

[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?

[iamwyza]

There are a few choices here. But it really comes down to how much we should be opinionated about the right way to do things.

The reality is that before we started supporting NRTs for Sprocs, you still should have been checking for null because it could have been returned as null according to the contract. Task<List<T>> in a namespace that doesn't have #nullable enable is, according to the compiler, nullable. That you didn't know it, doesn't mean that it wasn't the case. That's a big reason why I wanted to add full nullable support to sprocs, to properly provide the correct information up to the developers.

We have a few options:

1. Add a new option that will control only nullable for sprocs instead of sharing the same nullable option with everything else. I don't see this as a great option, but it would be the one that is the least opinionated. It would mean, of course, that folks could continue to have this landmine out there waiting for them.

2. Completely revert to pre-nullable support for sprocs. This undoes everything, puts us back where we were before, and leaves it up to end developers to intrinsically know that sprocs that can't return data will be sometimes return null (but sometimes not due to the autogenerated empty result object), and sprocs that can return data will be not null. I also don't like this option because it doesn't move us toward the end goal of being able to completely support nullable reference types across all of the generated code.

3. Actually fix the issues we found in SPROC: 'Sequence contains no elements' for SP that shouldn't return records #3121 surrounding the fact that existing folks were depending on the fact that we generated empty result objects by default for any sproc where it couldn't figure out what the return type was. This would be the ideal because we could then go back to the way this was originally envisioned where sprocs that return values were properly counted as never null. It's a much bigger lift though as it requires having some better way of handling the case of "this sproc does return data but we couldn't figure out what, so we generated an empty result class for you to fill in" vs the "we couldn't figure out the return value because in fact, there isn't one, so we don't need to generate the empty result class". These would likely be breaking changes to some % of the users (though we don't know precisely how big of an impact it'd be). Most likely we'd need an option that would enable this behavior plus altering the config file to include the ability to mark if a given sproc should generate an empty partial result class when it can't be auto-determined (so that it can be filled out by the developer). That way the default when it can't figure out if it has a return or not, is to not generate one, and it'd be opt-in per sproc. Then we could go back to properly having the 2 methods under the covers that properly returns null for non-returnable sprocs and returns not-null for sprocs that have return values.

[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

[iamwyza]

Feels like we still need to provide some mechanism to help them force when they want a sproc to not be treated as having a return type. Will UseLegacyResultSetDiscovery reliably handle the scenarios for non-value returning sprocs like we ran into in the other issue? I'd prefer to be able to let them reliably generate the sproc code properly based on the configs when they know for certain that it's a execute only sproc that returns nothing; or the opposite, where they know it will return something but neither of the auto-discoverability mechanisms will work and they want to force the generation of the empty partial class.

Maybe something like:

{
    "Name": "[dbo].[MultiSet]",
    "ObjectType": 1,
    "GenerateEmptyResultType": true // defaults to false.
},

Then we could make the default behavior under UseNonNullableModuleResultSets be that if it would normally generate an empty partial class, it will now instead not do that. It's similar to your proposal, in that it'll still cause things to break in their code if they had sprocs that they were adding partial implementations to, but the default going forward will mean that folks will generally have valid generation for their sproc calls if none of the discoverability mechanisms worked.

[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

[iamwyza]

I like that. I don't know when I'll be able to tackle it though. Maybe later this month, not sure at the moment. My new place is doing code first migrations and thus it doesn't have a use for EFCPT, so while I'd still like to contribute when I can, I won't have much opportunity to actually use the tooling in my day-to-day for the foreseeable future.

[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,

[tglnrg]

omccully, you might get some mileage out of post-processing the file yourself. If you place a file called efpt.postrun.cmd alongside your efpt.config.json then EFCPT will call it after it's done generating. I use this very effectively to change enums from storing as ints to storing as strings by adding [ColumnType] attributes but your use case is probably simpler: have the .cmd file call a powershell script so you can use better find/replace comamnds (regex), and it will then run the generated C# through a regex that makes the changes you want to nullability etc

Here's a starter for ten that you could try:

.cmd file:

@echo off
REM === efpt.postrun.cmd ===
powershell -NoLogo -NoProfile -ExecutionPolicy Bypass -File "%~dp0efpt.postrun.ps1"

.ps1 file:

<#
 efpt.postrun.ps1
 Runs after EF Core Power Tools scaffolding
#>

# Full path to the generated context classes
$ContextDir = Join-Path $PSScriptRoot -ChildPath 'PATH OF THE FOLDER YOU GENERATE YOUR CONTEXT INTO, RELATIVE TO THE PS1 FILE'

# Process every procedures file in that folder (non‑recursive; add -Recurse if needed)
Get-ChildItem -Path $ContextDir -Filter '*Procedures.cs' -File | ForEach-Object {
    $text = Get-Content -Path $_.FullName -Raw

    $text = $text `
        -replace '>[?]>', '>>' `
        -replace 'return _;', 'return _??[];'  

    Set-Content -Path $_.FullName -Value $text

}

[omccully]

@toglenergy I am actually running the EFCPT CLI from a C# program, so I actually do a lot of post-processing myself from there, and I did come up with a solution for this myself. However, I bet other people will benefit from this feature as well. Ideally, I would not have to do this much post-processing, so I previously reported other issues that required me to do so as well.

[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.

[tglnrg]

Yeah tbh i'm not quite sure why it's logical to emit null if there is a resultset, but it's empty. I'd rather zero rows become an empty set. Given that sprocs always return an int, i think it sensible that always be an out param, rather than a return value

[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)