SqlClient shrink SqlParameter and fix 34414

[Wraith2]

fixes https://github.com/dotnet/corefx/issues/34414

When using SqlCommandBuilder.DeriveParameters to identify the parameters of a stored procedure any table valued parameters will have their type name incorrectly constructed including the database name part.

This bug has been outstanding in SqlClient for quite some time so the direct fix would break user code where they rely on the incorrect parameter format to identify and fix the problem themselves. Rather than produce a correct name the name is still incorrectly generated but parameter is flagged as having a known incorrect name. If the user assigns to the typename the flag is cleared and we assume they've fixed it themselves. Later in the execution process when the user can no longer alter the parameter before execution the flag is checked and if still set the type name is auto corrected. Discussion of this method can be found on the original PR by @ssa3512

I have added a new test UDTParams_DeriveParameters which verifies that the auto correction behaviour takes place. This test requires a new table values type and sproc in the UDT test database so the test database script has been updated. The functional and manual tests all pass.

While investigating and implementing these changes I have compressed the SqlParameter field definitions because it was being quite wasteful about storing multiple parameters. Multiple boolean fields have been combined into flags, the rarely used xml fields have been moved into the same lazy initialized object as used in SqlCommand. I realize that this adds noise to the fix diffs but it seemed wasteful not to take the opportunity while I was working on the type. The patterns used are already used in SqlCommand so they're low risk imo.

/cc all the usual people @afsanehr @tarikulsabbir, @David-Engel and @ssa3512 @saurabh500 from the original PR

[TrayanZapryanov]

Isn't the result of AppendQuotedString same value as resultString.ToString()?

[Wraith2]

The output is the same, The new path doesn't allocate a second stringbuilder or a string for that segment it just writes everything to the stringbuilder, That also allows the replace call to avoid a string allocation because it can be done in the internal buffer.

[TrayanZapryanov]

My idea was that BuildQuotedString method can look like this:
`internal static string BuildQuotedString(string quotePrefix, string quoteSuffix, string unQuotedString)
{
var resultString = new StringBuilder(unQuotedString.Length + quoteSuffix.Length + quoteSuffix.Length);

        return AppendQuotedString(resultString, quotePrefix, quoteSuffix, unQuotedString);
    }`

But maybe I don't see something.

[Wraith2]

It could but that would change AppendQuotedString to be allocating when there's no need to. If you want to produce a multipart name you could call AppendQuotedString up to 4 times with . separators in between them. I chose to provide flexibility by having one method responsible for the lifetime of the builder and the other simply using it.

[tarikulsabbir]

The comments are mismatched here I think, part[0] should be server name, part[1] is the database/catalog name.

[Wraith2]

Note that they're length-x so the first reference is to the last element, so they're in the right order because it's server.database.schema.name.
The whole multipart identifier parse is a bit awkward. I have a replacement written up that works in terms of spans but I'm still iterating on the api surface and making sure I have full coverage.

[tarikulsabbir]

Yeah you're right. What I meant is in this line parts[length - 3] != null && // server should not be null or we don't need to remove it, parts[length - 3] will be parts[1] which is the database name, but the comment says server -- // server should not be null or we don't need to remove it,

[Wraith2]

I was overthinking it and adding complexity where it wasn't required. The overload used will always return an int[4] array and the results will be right justified in the array. I've simplified the checks which fixes this and the point raised below.

[tarikulsabbir]

The length will be 4 here always, right? As the ParseMultipartIdentifier is returning 4 parts ( each part can be null). In that case the first check length >=3 and last check length==3 is redundant.

[Wraith2]

Currently it will be 4, it isn't required to be though because there is an overload to ParseMultiPartIdentifier which takes a limit parameter and uses that to initialize the array. Since i'm also working on a replacement for this api as mentioned above i chose to be cautious and correct rather than assume the specific implementation detail and trip someone up later.

In purely correctness terms this should be a 3 or 4 part name and cannot be less, so that's what i went with. Practically i don't see how it can end up being a 4 part name but that isn't a reason to exclude the possibility when i can handle it.

[tarikulsabbir]

Well, at this moment we are not quite handling a 4 part name in this if block as we are expecting first part to be null, if length is 4, if not then it exits the if. However, as you said in future you will be working on this API, I am willing to keep this to extend the possibility of a better handling of a 4 part name if the need arises.

[tarikulsabbir]

why not 1 << 8 ?

[Wraith2]

Hmmm. I see two options.

• there was an 8 and i removed it, though i can't think what it might have been.
• i have an irrational and undiscovered fear of the number 8 that is causing me to avoid using it.

I think option 1 is more likely 😀 so i should switch it back. I'll do that tomorrow.

[jnm2]

Would have been funny if you had skipped 9 😊

[Wraith2]

I've worked out why. When I started writing the fix I thought I'd need two flags and had added them in. Later I realised it was simpler than I'd thought and removed the additional flag but didn't clean up the hole. It's cleaned now.

[tarikulsabbir]

The change looks good to me. I will run the CI tests one more time and will merge this PR.