-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
Add SqlTypes members to remove SqlClient internal access hacks #51836
Comments
Tagging subscribers to this area: @cheenamalhotra, @David-Engel Issue DetailsBackground and MotivationIn the desktop framework the
Proposed APInamespace System.Data.SqlTypes
{
public struct SqlDateTime
{
+ public static DateTime FromInternalRepresentation(int daypart, int timepart)
}
public struct SqlMoney
{
+ public static SqlMoney FromInternalRepresentation(long value);
+ public long GetInternalRepresentation();
}
public struct SqlDecimal
{
+ public GetInternalDataRepresentation(Span<uint> value)
}
public struct SqlBinary
{
+ public static SqlBinary WrapBytes(byte[] bytes)
}
public struct SqlGuid
{
+ public static SqlBinary WrapBytes(byte[] bytes)
}
}
Usage ExamplesCreating an ...
case TdsEnums.SQLUNIQUEID:
value.SqlGuid = SqlTypeWorkarounds.SqlGuidCtor(unencryptedBytes, true); // doesn't copy the byte array
break;
...
public static class SqlTypeWorkarounds
{
internal static SqlGuid SqlGuidCtor(byte[] value, bool ignored)
{
var c = default(SqlGuidCaster);
c.Fake._value = value;
return c.Real;
}
[StructLayout(LayoutKind.Sequential)]
private struct SqlGuidLookalike
{
internal byte[] _value;
}
[StructLayout(LayoutKind.Explicit)]
private struct SqlGuidCaster
{
[FieldOffset(0)]
internal SqlGuid Real;
[FieldOffset(0)]
internal SqlGuidLookalike Fake;
}
} to: ...
case TdsEnums.SQLUNIQUEID:
value.SqlGuid = SqlGuid.WrapBytes(unencryptedBytes);
break;
... Alternative Designs/ReasoningSqlDateTime.FromInternalRepresentation
SqlMoney.ToSqlInternalRepresentation
SqlMoney.FromInternalRepresentation
SqlDecimal.GetInternalDataRepresentationThis method name was chosen for close to be similarity to the This method could have been called SqlBinary.WrapBytes and SqlGuid.WrapBytesThese could be called GeneralThe existing behaviour of accessing internal members will be present in pre NET6 framework and SqlClient binaries for as long as both exist. It would cause compatibility problems discoverable only at runtime if we tried to change that. Adding these members will allow the same access to be done using a legitimate channel on NET6 and future SqlClient versions that compile against it. Adding these new members simply makes the access to data used by SqlClient official and thus documented. They should have no performance impact. RisksThe new members will need to be documented. /cc area owners @ajcvickers, @roji and @David-Engel
|
@roji , @David-Engel triage? |
This makes sense to me. Seems like one of the easier internals hacks to resolve in SqlClient since the breakup. @roji @ajcvickers @saurabh500 @cheenamalhotra - Anyone object to exposing the proposed methods in |
My initial thoughts:
My thought is what is an internal data representation doesn't need to be called out specifically in API name. Conversion can be handled "From" and "To" a datatype just like every other API in base Types, unless the type itself does not represent the data and needs a specific definition. |
For the record, I'd have much preferred for these types to be copied into Microsoft.Data.SqlClient - but that does seem impractical. |
BTW, unless I'm mistaken the same holds for previous .NET Core versions - the earlier point when SqlTypeWorkarounds would be removable is when M.D.SqlClient drops support for net5.0, right? Is that worth pursuing? |
Admittedly, I don't know much about these SQL types, but I don't understand the request for these factory methods, given that both of those already have a constructor that takes in a |
@Joe4evr if you look at the implementation, you'll see these constructor copy the given byte array, which isn't good for perf and isn't needed when SqlClient is constructing the instance. |
I considered this but decided to use Wrap and Unwrap because it clearly communicates that the array passed in it taken and not copied which is important for correct use.
The values returned being internal representations is important. The value contained in the returned data type is not the usual format that .net programs expect and should not be treated as convertible by standard apis.
Yes, netcore only because netfx is frozen. There are always going to have to be separate code paths for the two frameworks so this doesn't add much extra burden. It will allow the type workarounds to be clearly marked as deprecated in tfms that use them, they should not be added to. It is not going to be possible to remove the existing hacks for the forseeable future. Being able to remove them and being able to say that they should not be used or extended anymore are two different goals. As an external library to the runtime SqlClient should not be using private implementation detail information like this, when it was done it was for pragmatic reasons but leaving it like this can be seen as implicit blessing and it should not be.
An existing operation handles the to direction conversion.
I agree but without that wouldn't be possible without breaking DataTable and DataAdapter interoperability. Those are still supported classes and likely to be in use a huge amount of LOB codebases. |
Not just netfx is frozen - also older versions of netcore. You'll need to multi-target to make use of this.
That's certainly true in theory, but once again, this is going to stay in place until M.D.SqlClient drops net5.0 (which I think is, well, an extremely long time). And while SqlTypes is technically part of the runtime, but in practice is part of SqlClient (we just can't copy those types out because of DataTable). So speaking very pragmatically, I think there's very little value here, and personally wouldn't spend any time on it (and the API design discussions it could spawn e.g. regarding naming). However, at the end of the day, since I view SqlTypes as internal to SqlClient, it's ultimately up to you guys. If you really think this is worth pursuing we can try. |
SqlClient already multi targets and probably always will. As you say, the future where net6 is the lowest supported runtime is a long time away and may never occur. I think it is worth doing because:
If someone feels that any of those points are incorrect then we can just carry on as we do currently. |
What is this waiting for please? |
Any chance of an update? This seems to be in limbo as untriaged. Is it not at least worth letting the design team look at it and decide whether it has any merit? |
Just to be clear, this would be waiting on the SqlClient team, should they decide they want to do this. I personally still believe this has very little value:
But as I wrote above, this is ultimately SqlClient's decision. |
While I see that this is the "right" thing to do, I'll be honest. We don't have the bandwidth to push for it. If you (@Wraith2) want to submit the API and PR on the runtime side and the equivalent changes on the SqlClient side, I'm not against that. But you'd be waiting for us to create a suitable .NET x specific target and this feature alone isn't strong enough to warrant its own target. We will likely be creating a .NET 6 target (skipping 5 since it's out of support in May) and skipping 7 since I don't think it will be I think you might get frustrated when people aren't prioritizing each required change. There are many things I would prioritize over this. Examples:
Regards, |
Ok. |
Re-opening issue for consideration in a future .NET release. |
Thanks, @Wraith2. I updated this specific issue and fired off an email to resolve the permissions snag. |
namespace System.Data.SqlTypes;
public partial struct SqlDateTime
{
public static SqlDateTime FromParts(int dayPart, int timePart);
}
public partial struct SqlMoney
{
public static SqlMoney FromTdsValue(long value);
public long GetTdsValue();
}
public partial struct SqlDecimal
{
public int WriteTdsValue(Span<uint> destination);
}
public partial struct SqlBinary
{
public static SqlBinary WrapBytes(byte[] bytes);
}
public partial struct SqlGuid
{
public static SqlGuid WrapBytes(byte[] bytes);
} |
I posted in Youtube chat during the api review but it doesn't look like people can see all of the chat messages. |
@Wraith2 Generally the API Review meeting doesn't talk about releases (other than "this looks scary big and you probably won't get feedback before it's frozen, you should probably wait for the next release"). The issue is marked as 7, so I assume the intent was to get it in for .NET 7. |
I missed review this morning due to a conflict but have a concern. I believe we should not create If we create public struct SqlGuid
{
private byte[] _value;
} And we would never be able to change the internal layout to this more optimized shape: public struct SqlGuid
{
private Guid? _value;
} This hampers our ability to make future improvements to the Framework. If we are really concerned about the performance of the |
Yes, but. The type is in the framework since .NET 1.0. It's not to say that we can't improve the type, it just seems unlikely at this point... |
Yet here we are, in .NET 7, talking about improving the type. And API review gave a stamp of approval to doing so. :) So I don't buy that it's "unlikely". This issue was opened literally because there's a performance problem with the existing implementation. Why aren't we doing the easy non-API-changing thing to try fixing the implementation first, before considering new API which permanently prevents us from fixing the implementation? |
LOL. I was more thinking "the fundamental shape or encoding of the type". Adding methods that operate over the same shape is much easier to reason about.
That's a great question and @David-Engel would be more qualified to answer that. I assume these types are kinda frozen in their shape due to being tied to the TDS spec. |
The TDS spec freezes the capabilities of these types, but it doesn't freeze their layout details. We had quite the in-depth email discussion on this a few months ago. |
To clarify, I interpret the "fix the implementation" statement to mean fix the System.Data.SqlTypes implementation of SqlClient simply needs methods that avoid so much unnecessary byte copying, as the whole job of the database client is to construct many, many objects like these. I'm fine with whatever API shape we land on to accomplish that. |
I think @GrabYourPitchforks 's version of "fix the implementation" is to change the field from |
Since the struct seems to keep a separate state of null, I'd say that |
Right. "Fixing the implementation" wouldn't require adding any new API shape at all. It would literally be changing this: private byte[] _value;
public SqlGuid(byte[] value)
{
// argument length check
_value = (byte[])value.Clone();
} To: private Guid? _value;
public SqlGuid(byte[] value)
{
// argument length check
_value = new Guid(value);
} Aside from the length check (which you'd need in all cases), it's literally a single |
FYI: optimizing the implementation would require implementing |
Optimizes the SqlGuid struct according to the idea from dotnet#51836.
I've opened a draft to show how the proposed implementation changes would look like: #72549 |
@David-Engel, what your thoughts on this? What @GrabYourPitchforks said makes sense to me. |
Agreed. I'm all for PR #72549. We'll have to adjust/service System.Data.SqlClient and Microsoft.Data.SqlClient to keep them compatible with .NET 7. With #72549, the API proposal here can eliminate the new |
Instead this is needed in order to maintain binary serialization (is a new API review needed?): public partial struct SqlGuid : ISerializable
{
void ISerializable.GetObjectData(SerializationInfo si, StreamingContext sc);
} |
Background and Motivation
In the desktop framework the
System.Data.SqlClient
namespace was originally part of theSystem.Data
assembly allowing SqlClient to access internal methods of some types in theSystem.Data.SqlTypes
namespace. In netcoreSqlClient
was split out into a separate assembly but rather than add new public apis to theSqlTypes
struct overlay hacks were used to maintain the existing access to internals.SqlClient
has now been split out of the framework entirely into a separate projectMicrosoft.Data.SqlClient
and that project still uses the overlay hack to access internals, all collected into the [SqlTypeWorkarounds](SqlClient/SqlTypeWorkarounds.cs at main · dotnet/SqlClient (github.com)) class. It is unlikely that the layout or definition of theSqlTypes
members will change but it is undesirable to have a first party library use such access when it is possible and relatively simple to extend the public api.Proposed API
Usage Examples
Creating an
SqlMoney
from it's internal TDS representation will change from:to:
Alternative Designs/Reasoning
SqlDateTime.FromInternalRepresentation
internal static DateTime ToDateTime(int daypart, int timepart)
which could have the access modifier changed to public. I chose not to suggest this because it doesn't convert an SqlDateTime instance to a DateTime, it converts parameters which are the internal representation used in TDS into a DateTime, the input and output are not what a user would expect without reading the documentation.GetFieldValue<DateTime>
orGetDateTime
SqlMoney.ToSqlInternalRepresentation
SqlMoney
already contains an internal implementation of this method so i propose only to change the existing access modifier. The internal representation is an unscaled value read from a TDS packet.SqlMoney.FromInternalRepresentation
SqlMoney
contains an internal ctorinternal SqlMoney(long value, int ignored)
which was used to perform an unscaled construction ofSqlMoney
from the internal representation. The ignored parameter is present because there is already a public ctorinternal SqlMoney(long value)
which performs a scaled creation. Since I've already proposed a method to get the unscaled value and contains the words InternalRepresentation it made sense to me to have a symmetric static method to do the creation from that representation.SqlDecimal.GetInternalDataRepresentation
This method name was chosen for close to be similarity to the
SqlMoney
andSqlDateTime
methods which provide access to internal data representations. This method does not give access to all of the internal data for theSqlDecimal
, it only provides access to 4 Data uints which contain the value, it does not provide access to the additional 4 bytes of data (Status, Length, Precision and Scale) and because of that I have added the word Data in the middle of the name. Access to the other byte fields is done through existing public accessor properties.This method could have been called
GetBits
for some symmetry withdecimal.GetBits
, i decided against this because it isn't an exactly analogous operation.SqlBinary.WrapBytes and SqlGuid.WrapBytes
These could be called
TakeBytes
but that felt clumsy or too low level a name. Their intention is to return an instance of their containing type which uses the reference to the array parameter passed in, currently byte[] ctor for each type would allocate a new appropriately sized array and copy the contents of the parameter into it.General
The existing behaviour of accessing internal members will be present in pre NET6 framework and SqlClient binaries for as long as both exist. It would cause compatibility problems discoverable only at runtime if we tried to change that. Adding these members will allow the same access to be done using a legitimate channel on NET6 and future SqlClient versions that compile against it.
Adding these new members simply makes the access to data used by SqlClient official and thus documented. They should have no performance impact.
Risks
The new members will need to be documented.
/cc area owners @ajcvickers, @roji and @David-Engel
The text was updated successfully, but these errors were encountered: