Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SqlBatch: Add missing overrides and tests #2223

Merged
merged 2 commits into from Nov 22, 2023

Conversation

Wraith2
Copy link
Contributor

@Wraith2 Wraith2 commented Nov 16, 2023

fixes #2222

Add missing overrides for SqlConnection CanCreateBatch and CreateDbBatch() and tests
Add test for SqlBatchCommand CanCreateParameter and CreateParameter() wrapped in #ifdef
Adding a net8 build is out of scope for this PR. That needs approval at very least from the MS side. When it is added the net8 features and tests related to the sql batch feature should light up automatically.

@David-Engel we'll want to get this in before the next preview.

/cc @mgravell

@roji
Copy link
Member

roji commented Nov 16, 2023

@Wraith2 I think you also want to override the related methods missing on SqlClientFactory (here are the original specs):

public virtual DbBatch CreateBatch() => throw new NotSupportedException();
public virtual DbBatchCommand CreateBatchCommand() => throw new NotSupportedException();
    
// Covers both CreateBatch and CreateBatchCommand
public virtual bool CanCreateBatch => false;

@Wraith2
Copy link
Contributor Author

Wraith2 commented Nov 16, 2023

Those are in a separate file:

public sealed partial class SqlClientFactory : DbProviderFactory
{
/// <include file='../../../../../../../doc/snippets/Microsoft.Data.SqlClient/SqlClientFactory.xml' path='docs/members[@name="SqlClientFactory"]/CanCreateBatch/*'/>
public override bool CanCreateBatch => true;
/// <include file='../../../../../../../doc/snippets/Microsoft.Data.SqlClient/SqlClientFactory.xml' path='docs/members[@name="SqlClientFactory"]/CreateBatch/*'/>
public override DbBatch CreateBatch() => new SqlBatch();
/// <include file='../../../../../../../doc/snippets/Microsoft.Data.SqlClient/SqlClientFactory.xml' path='docs/members[@name="SqlClientFactory"]/CreateBatchCommand/*'/>
public override DbBatchCommand CreateBatchCommand() => new SqlBatchCommand();

@roji
Copy link
Member

roji commented Nov 16, 2023

My bad - I missed that when looking at the source code! Great that they're there.

@Wraith2
Copy link
Contributor Author

Wraith2 commented Nov 16, 2023

They should light up as soon as the team add a net8 build.

@David-Engel
Copy link
Contributor

Do these APIs need to be in the ref assemblies, too?

@Wraith2
Copy link
Contributor Author

Wraith2 commented Nov 16, 2023

They're overrides so I wouldn't expect to see them in the ref assembly since they're defined on the base class and we didn't merge our own copies of the base classes.

@David-Engel David-Engel added this to the 5.2.0-preview4 milestone Nov 17, 2023
Copy link

codecov bot commented Nov 17, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (d30d9d1) 71.49% compared to head (6de6b91) 68.42%.
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2223      +/-   ##
==========================================
- Coverage   71.49%   68.42%   -3.08%     
==========================================
  Files         309      309              
  Lines       62181    62183       +2     
==========================================
- Hits        44459    42547    -1912     
- Misses      17722    19636    +1914     
Flag Coverage Δ
addons 92.88% <ø> (ø)
netcore 66.24% <100.00%> (-8.32%) ⬇️
netfx 69.85% <ø> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@DavoudEshtehari DavoudEshtehari added 🐛 Bug! Something isn't right ! netcore labels Nov 20, 2023
@DavoudEshtehari DavoudEshtehari added this to In progress in SqlClient v5.2 via automation Nov 20, 2023
SqlClient v5.2 automation moved this from In progress to Reviewer approved Nov 21, 2023
@DavoudEshtehari DavoudEshtehari merged commit e6109f8 into dotnet:main Nov 22, 2023
132 checks passed
SqlClient v5.2 automation moved this from Reviewer approved to Done Nov 22, 2023
@Wraith2 Wraith2 deleted the batch-fixes-1 branch March 17, 2024 23:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 Bug! Something isn't right ! netcore
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Missing functionality in batch API
4 participants