Uh oh!
There was an error while loading. Please reload this page.
Conversation
- annotate NRTs on GridReader API - add protected access to some of the GridReader internals - switch GridReader callbacks to be more type-independent docs lib updates; deal with yellow warnings and test failures (SQL server certs and cancellation surfacing differently) fix break simplify proposed GridReader changes to protected OnBeforeGrid and OnAfterGrid[Async] Builds: Bump library versions (#1935) Tests: Upgrade dependencies for Dependabot (#1936) I'll do a pass at all of these later, but getting CVE versions out of the pipe. Resolving 4 alerts here: https://github.com/DapperLib/Dapper/security/dependabot merge and lib updates allow Identity to be constructed on-the-fly inside GridReader fix build warnings include readme rev minor - enable [SkipLocalsInit] everywhere - use NET5_0_OR_GREATER for remoting check # Conflicts: # Dapper/CommandDefinition.cs # Dapper/DefaultTypeMap.cs # Dapper/SqlMapper.Async.cs # Dapper/SqlMapper.IDataReader.cs # Dapper/SqlMapper.Link.cs # Dapper/SqlMapper.cs # Directory.Build.props
mgravell
commented
Aug 18, 2023
@NickCraver I've added API proof to this; do you have an opinion on this? otherwise, ima merge |
NickCraver
commented
Aug 18, 2023
@mgravell I can this afternoon or tonight - but if not doing a release and this is blocking, happy to comment after the merge too and we can tidy to parallel - your call! |
mgravell
commented
Aug 18, 2023
Not immediately blocking; however it is a pain to have to keep rebasing it (which is why it is heavily squashed and force-pushed), as it touches a lot |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
NickCraver
commented
Aug 19, 2023
@mgravell Tried to give good eyes here, admittedly glossed over the NRT exclusions in the Emit section as the last reviewed thing, I'll review it fresh next round (just a lot to consume in one go - nothing wrong on PR structure) |
mgravell
commented
Aug 20, 2023
addressed all, I think |
| /// <param name="parameter">The parameter to configure</param> | ||
| /// <param name="value">Parameter value</param> | ||
| public abstract void SetValue(IDbDataParameter parameter, T value); | ||
| public abstract void SetValue(IDbDataParameter parameter, T? value); |
There was a problem hiding this comment.
@mgravell Since this causes implementers of the class to have to annotate their T value parameter as nullable, how would we handle null being passed here? What would it mean?
I know for SqlParameter.Value, DBNull means "set to null" and null means "unset." Does null carry the same "unset" connotation if implementers see it here?
Thanks!
There was a problem hiding this comment.
Since there isn't a constraint on T, this doesn't strictly change the API shape - in particular, a TypeHandler<SomeValueType> still has SetValue(..., SomeValueType) and not SetValue(..., SomeValueType?) ; however, to answer your question: it should mean DBNull, but with the opportunity to configure the parameter correctly, and mostly just applies in the TypeHandler<SomeReferenceType> scenario, it acknowledges that the incoming parameter might benull.
Functionally, the only real changes here are in a few API surfaces (not a real change, but NRT changes things slightly):
NOTE: the intention is to do a minor rev here, i.e. ship this as 2.1.0
?{Query|Read}{First|Single}OrDefault[Async]return?ExecuteScalar[Async]return?readme.md(this is the current NuGet recommendation)[SkipLocalsInit]everywhere; simplify the remoting warning logicfor Dapper.AOT purposes (although everything here applies to any consumer), I'm also applying some tweaks to make
GridReadermore usable from externals;GridReaderis notsealed, but was mostly unusable from custom implementations - added some things:protectedconstructorAction<object?>?/object?callback rather than type-specific (so external consumers aren't bound to same types)protected int OnBeforeGrid()(pre-grid book-keeping)protected void OnAfterGrid(int)andprotected Task OnAfterGridAsync(int)(post-grid book-keeping)OnBeforeGrid/OnAfterGridAPIprotectedAPIsIdentityto be constructed on-the-fly if/when needed(the NRT issue was also partly driven from AOT)