Skip to content

Support special casing with Fabric endpoints#3084

Merged
paulmedynski merged 5 commits into
dotnet:mainfrom
ErikEJ:az-endpoints
Mar 21, 2025
Merged

Support special casing with Fabric endpoints#3084
paulmedynski merged 5 commits into
dotnet:mainfrom
ErikEJ:az-endpoints

Conversation

@ErikEJ

@ErikEJ ErikEJ commented Dec 17, 2024

Copy link
Copy Markdown
Contributor

Support the special casing for Fabric endpoints

@codecov

codecov Bot commented Dec 17, 2024

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 72.88%. Comparing base (b8e0c9f) to head (4c73ef8).
Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3084      +/-   ##
==========================================
+ Coverage   72.70%   72.88%   +0.18%     
==========================================
  Files         283      283              
  Lines       58975    58978       +3     
==========================================
+ Hits        42875    42984     +109     
+ Misses      16100    15994     -106     
Flag Coverage Δ
addons 92.58% <ø> (ø)
netcore 75.58% <100.00%> (+0.07%) ⬆️
netfx 71.33% <100.00%> (+0.19%) ⬆️

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.

@edwardneal edwardneal left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only one comment on the code style, but the checks themselves are correct.

I think there are a handful of feature differences between Azure SQL and Fabric SQL which merit a test suite and documentation updates though. At a glance, the driver would be impacted by the lack of support for Always Encrypted and the BULK INSERT statement.

Comment thread src/Microsoft.Data.SqlClient/src/Microsoft/Data/Common/AdapterUtil.cs Outdated
@ErikEJ

ErikEJ commented Dec 17, 2024

Copy link
Copy Markdown
Contributor Author

@edwardneal There are a number of feature differences between the many engine variations this driver supports or happen to work with - is it really the responsibility of the driver to document this, or is the page you are linking to not enough?

@edwardneal

Copy link
Copy Markdown
Contributor

I think the page I linked to would cover most cases, I'm thinking primarily about a once-over to make sure that the SqlClient documentation doesn't contradict it and is clear enough for people to understand what functionality the feature depends upon. The example I had in mind was SqlBulkCopy - it might not be obvious that the lack of BULK INSERT support blocks this feature. I think that's probably the root cause of #2995.

@ErikEJ

ErikEJ commented Dec 17, 2024

Copy link
Copy Markdown
Contributor Author

#2995 refers to Fabric warehouse, which is something completely different from Fabric SQL Database

@edwardneal

Copy link
Copy Markdown
Contributor

Sorry, slightly delayed here!

You're right, but the root cause is similar: some of SqlClient's functionality can't be supported on a particular engine variation, because that functionality relies upon a feature the engine doesn't support (in this case, SqlBulkCopy relies upon the BULK INSERT statement, which neither Fabric Warehouse nor Fabric SQL Database support.)

My broader point is that we should make sure that the SqlClient documentation for functionality which depends on BULK INSERT or Always Encrypted doesn't claim to be compatible across all engine variations, and that specifically for SqlBulkCopy we should make it clear that the feature depends upon server-side BULK INSERT support (since that's hard to discover in the documentation, and without that insight it's hard to know what won't support the feature.) If you want to keep this PR scoped specifically to the endpoint detection, I'm happy enough to check that documentation in a separate PR.

@ErikEJ

ErikEJ commented Dec 19, 2024

Copy link
Copy Markdown
Contributor Author

Sounds to me like a separate issue and PR would be suitable to prevent scope creep.

@mdaigle mdaigle added this to the 7.0-preview1 milestone Jan 7, 2025
@mdaigle mdaigle requested a review from a team January 7, 2025 18:21
@ErikEJ

ErikEJ commented Feb 27, 2025

Copy link
Copy Markdown
Contributor Author

@mdaigle any thoughts on this?

Comment thread src/Microsoft.Data.SqlClient/src/Microsoft/Data/Common/AdapterUtil.cs Outdated

@mdaigle mdaigle left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for cleaning up the resource files!
I just have two small requests.

Comment thread src/Microsoft.Data.SqlClient/src/Microsoft/Data/Common/AdapterUtil.cs Outdated
Comment thread src/Microsoft.Data.SqlClient/netfx/src/Microsoft.Data.SqlClient.csproj Outdated
@cheenamalhotra

Copy link
Copy Markdown
Member

@ErikEJ do review and update docs where the current endpoints are mentioned. Thanks!

@ErikEJ

ErikEJ commented Mar 20, 2025

Copy link
Copy Markdown
Contributor Author

@cheenamalhotra @mdaigle Docs updated and PR feedback addressed

@ErikEJ

ErikEJ commented Mar 20, 2025

Copy link
Copy Markdown
Contributor Author

@cheenamalhotra @mdaigle CI never kicked in??

@ErikEJ

ErikEJ commented Mar 20, 2025

Copy link
Copy Markdown
Contributor Author

I used the new resource designer in VS for the removal, please make sure to review the Designer file changes.

@paulmedynski

Copy link
Copy Markdown
Contributor

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@ErikEJ

ErikEJ commented Mar 20, 2025

Copy link
Copy Markdown
Contributor Author

I will revert the Designer changes..

@ErikEJ

ErikEJ commented Mar 21, 2025

Copy link
Copy Markdown
Contributor Author

@mdaigle Designer changes reverted

@paulmedynski

Copy link
Copy Markdown
Contributor

@mdaigle Designer changes reverted

I liked the Designer changes - mostly fixing typos from what I could see. Did this cause problems elsewhere?

Also, you seem to have rebased and pushed 1 new commit to revert the Designer changes, but GitHub still has me marked as Approved. I haven't reviewed the rebase nor the 1 new commit, so that's weird.

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.

5 participants