565720 - [MS-SQL] Support MS SQL Server: CLR Functions

Status: NEW

(Bugzilla :: Database, enhancement)

Description

[Michael Thomas (Mockodin)]

Attachment: v1

MSSQL CLR functions to create missing functionality such as RegEx support and simplify database operations such as drop column when default values are present.

The goal is to replicate commonly accepted norms from other database platforms such as MySQL within MSSQL.

Features to be added at this time
Function Name     | Emulated DB | Emulated Function
GroupConcat       | MySQL       | GROUP_CONCAT
RexExContains     | MySQL       | REGEXP
RexExContains     | MySQL       | NOT REGEXP
ToDays            | MySQL       | TO_DAYS
FromDays          | MySQL       | FROM_DAYS
DateFormat        | MySQL       | DATE_FORMAT

Helper Stored Procs
AlterDefault    - Drops and Creates New Default for a Column
AlterPrimaryKey - Drops and Creates New Primary Key for Table
DropColumn      - Drops any existing Default for Column, then drops Column

See bug 487437 for Originally Submitted version and review notes prior to this this bug

Comment 1

[Jasmin Sehic]

Attachment: v2

Added SortOrder to GroupConcat Accumulate function

Comment 2

[Jasmin Sehic]

Attachment: v3

Comment 3

[Max Kanat-Alexander]

Comment on attachment 562297 [details] [diff] [review]
v3

Review of attachment 562297 [details] [diff] [review]:
-----------------------------------------------------------------

::: contrib/mssql_clr_functions/Bugzilla.MSSQL/Bugzilla.Mssql.cs
@@ +1,1 @@
> +// Original Source Zplace.com <[e-mail]> March 2010

This needs an MPL header. I believe we can assume that mockodin intended Zplace to contribute this under the MPL, as the entire rest of his code was contributed under the MPL, and he posted it as a patch to Bugzilla, which only accepts MPL-licensed code.

@@ +5,5 @@
> +using Microsoft.SqlServer.Server;
> +using System.Text.RegularExpressions;
> +using System.Collections;
> +using System.Globalization;
> +using System.Data.SqlClient;

Probably best to do a "Remove and sort" on this.

@@ +6,5 @@
> +using System.Text.RegularExpressions;
> +using System.Collections;
> +using System.Globalization;
> +using System.Data.SqlClient;
> +

Why isn't there a namespace?

@@ +9,5 @@
> +using System.Data.SqlClient;
> +
> +public partial class UserDefinedFunctions
> +{
> +    [Microsoft.SqlServer.Server.SqlFunction(DataAccess=DataAccessKind.None, IsDeterministic=true, IsPrecise=true)]

Instead of having that all one one line, can it be broken into several?

Also, there's already a "using Microsoft.SqlServer.Server" at the top of the file, so we don't have to type that out fully every time.

@@ +10,5 @@
> +
> +public partial class UserDefinedFunctions
> +{
> +    [Microsoft.SqlServer.Server.SqlFunction(DataAccess=DataAccessKind.None, IsDeterministic=true, IsPrecise=true)]
> +    public static SqlBoolean RexExContains([SqlFacet(MaxSize = -1)]SqlString subject, [SqlFacet(MaxSize = -1)]SqlString pattern, SqlBoolean ignoreCase)

That looks like it should be called RegexContains.

Can we impose a line-length limit of something like 100 characters for all of these .cs files? Here's how I'd format this line:

public static SqlBoolean RegexContains(
    [SqlFacet(MaxSize = -1)]
    SqlString subject,
    [SqlFacet(MaxSize = -1)]
    SqlString pattern,
    SqlBoolean ignoreCase)
{


Also, one other comment--do you really need MaxSize = -1 explicitly there?

These comments in general apply to every function in this file.

@@ +12,5 @@
> +{
> +    [Microsoft.SqlServer.Server.SqlFunction(DataAccess=DataAccessKind.None, IsDeterministic=true, IsPrecise=true)]
> +    public static SqlBoolean RexExContains([SqlFacet(MaxSize = -1)]SqlString subject, [SqlFacet(MaxSize = -1)]SqlString pattern, SqlBoolean ignoreCase)
> +    {
> +        if (subject.IsNull || pattern.IsNull || string.IsNullOrEmpty(subject.Value) || string.IsNullOrEmpty(pattern.Value)) return SqlBoolean.False;

Don't use single-line if statements like this. At the worst, put the "return" there on the next line, indented four spaces. This comment applies to every single-line "if" statement in this file.

Also, it's entirely possible for an empty pattern to validly match a regex. (For example, empty strings match the regex ".*".)

@@ +16,5 @@
> +        if (subject.IsNull || pattern.IsNull || string.IsNullOrEmpty(subject.Value) || string.IsNullOrEmpty(pattern.Value)) return SqlBoolean.False;
> +
> +        string patternValue = pattern.Value.ToString().Replace("[[:cntrl:]]", "[\\x00-\\x1f]");
> +
> +        return (Regex.IsMatch(subject.Value, patternValue, (ignoreCase.IsTrue ? RegexOptions.IgnoreCase : RegexOptions.None)) ? SqlBoolean.True : SqlBoolean.False);

That's a hugely long line that does way too much. Can some of that be put into individual variables on individual lines?

@@ +22,5 @@
> +
> +    [Microsoft.SqlServer.Server.SqlFunction(DataAccess = DataAccessKind.None, IsDeterministic = true, IsPrecise = true)]
> +    public static SqlBoolean RexExNotContains([SqlFacet(MaxSize = -1)]SqlString subject, [SqlFacet(MaxSize = -1)]SqlString pattern, SqlBoolean ignoreCase)
> +    {
> +        if (subject.IsNull || pattern.IsNull || string.IsNullOrEmpty(subject.Value) || string.IsNullOrEmpty(pattern.Value)) return SqlBoolean.False;

Same comments here about empty subjects, line length, etc.

@@ +23,5 @@
> +    [Microsoft.SqlServer.Server.SqlFunction(DataAccess = DataAccessKind.None, IsDeterministic = true, IsPrecise = true)]
> +    public static SqlBoolean RexExNotContains([SqlFacet(MaxSize = -1)]SqlString subject, [SqlFacet(MaxSize = -1)]SqlString pattern, SqlBoolean ignoreCase)
> +    {
> +        if (subject.IsNull || pattern.IsNull || string.IsNullOrEmpty(subject.Value) || string.IsNullOrEmpty(pattern.Value)) return SqlBoolean.False;
> +        return (RexExContains(subject, pattern, ignoreCase).IsTrue ? SqlBoolean.False : SqlBoolean.True);

Those outer parentheses look unnecessary to me. Also, can SqlBoolean just do a cast, here?

@@ +33,5 @@
> +        if (string.IsNullOrEmpty(findValue.Value) || string.IsNullOrEmpty(inThis.Value)) return 0;
> +        // Indexof returns 0 if the string is found starting at the first position
> +        // and -1 not found since we are using this in a database environment, we will incriment by 1
> +        // meaning 0 equals not found.
> +        return ((string)inThis.Value).IndexOf(findValue.Value, (ignoreCase ? StringComparison.InvariantCultureIgnoreCase : StringComparison.InvariantCulture)) + 1;

Once again, this is an enormously long line that should be broken into several lines where variables get set.

@@ +42,5 @@
> +    {
> +        if (dateTime.IsNull) return SqlInt32.Null;
> +
> +        //Adjusted Epoch date to return of the same value MySql would.
> +        DateTime SubjectDate = dateTime.Value;

Better to use "var" there.

@@ +43,5 @@
> +        if (dateTime.IsNull) return SqlInt32.Null;
> +
> +        //Adjusted Epoch date to return of the same value MySql would.
> +        DateTime SubjectDate = dateTime.Value;
> +        DateTime Y1900 = new DateTime(1900, 1, 1);

This can be a static readonly constant at the top of the file.

@@ +44,5 @@
> +
> +        //Adjusted Epoch date to return of the same value MySql would.
> +        DateTime SubjectDate = dateTime.Value;
> +        DateTime Y1900 = new DateTime(1900, 1, 1);
> +        long elapsedTicks = SubjectDate.Ticks - Y1900.Ticks;

var

@@ +46,5 @@
> +        DateTime SubjectDate = dateTime.Value;
> +        DateTime Y1900 = new DateTime(1900, 1, 1);
> +        long elapsedTicks = SubjectDate.Ticks - Y1900.Ticks;
> +
> +        TimeSpan elapsedSpan = new TimeSpan(elapsedTicks);

var

(I'll stop making this comment, just use it when necessary.)

Also, doing math with DateTime objects automatically produces a TimeSpan. You don't have to do math on the Ticks and then create a new TimeSpan from the ticks.

@@ +102,5 @@
> +            .Replace(@"%X", YearOfWeek(Date, DayOfWeek.Sunday).ToString())
> +            .Replace(@"%x", YearOfWeek(Date, DayOfWeek.Monday).ToString())
> +            .Replace(@"%Y", Date.Year.ToString())
> +            .Replace(@"%y", Date.Year.ToString().Substring(2, 2))
> +            .Replace(@"%%", @"%"));

So, DateTime.ToString() already has a formatter. Instead of all of this, how about just translating the input format into the format that DateTime.ToString expects, and making sure it does it with CultureInfo.Invariant?

@@ +156,5 @@
> +    {
> +        if (dateTime.IsNull) return null;
> +
> +        // We only want the last two numbers to figure out the suffix, we also need this to be a string to use EndsWith and StartsWith
> +        string day = (((DateTime)dateTime).Day < 100 ? ((DateTime)dateTime).Day.ToString() : ((DateTime)dateTime).Day.ToString().Substring(((DateTime)dateTime).Day.ToString().Length - 2));

Line is too long and does too much.

@@ +180,5 @@
> +        return ((DateTime)date).Year;
> +    }
> +
> +    [Microsoft.SqlServer.Server.SqlFunction(DataAccess = DataAccessKind.None, IsDeterministic = true, IsPrecise = true)]
> +    public static string DllVersion (){

So, out of curiosity, why is every function in here static? Is that just how these things work with SQL Server?

@@ +11,5 @@
> +    <ProjectGuid>{CEB9E10F-FAC4-411E-ACE8-8AECE457330B}</ProjectGuid>
> +    <OutputType>Library</OutputType>
> +    <NoStandardLibraries>false</NoStandardLibraries>
> +    <AssemblyName>Bugzilla.Mssql</AssemblyName>
> +    <TargetFrameworkVersion>v2.0</TargetFrameworkVersion>

Wow, does SQL Server really only support .NET 2.0?

@@ +45,5 @@
> +  <Import Project="$(MSBuildBinPath)\SqlServer.targets" />
> +  <ItemGroup>
> +    <Reference Include="System" />
> +    <Reference Include="System.Data" />
> +    <Reference Include="System.XML" />

Do you actually need to reference System.XML?

@@ +4,5 @@
> +using System.Data.SqlTypes;
> +using Microsoft.SqlServer.Server;
> +using System.Text.RegularExpressions;
> +using System.Collections;
> +using System.Collections.Generic;

Remove and sort.

@@ +8,5 @@
> +using System.Collections.Generic;
> +
> +
> +[Serializable]
> +[Microsoft.SqlServer.Server.SqlUserDefinedAggregate(Microsoft.SqlServer.Server.Format.UserDefined, MaxByteSize = -1)]

Don't need to prefix anything here with Microsoft.SqlServer.Server. (Same comment goes for the entire file.)

@@ +11,5 @@
> +[Serializable]
> +[Microsoft.SqlServer.Server.SqlUserDefinedAggregate(Microsoft.SqlServer.Server.Format.UserDefined, MaxByteSize = -1)]
> +public struct GroupConcat : Microsoft.SqlServer.Server.IBinarySerialize
> +{
> +    private ArrayList Remembered;

Why use an ArrayList instead of some type from System.Collections.Generic?

@@ +12,5 @@
> +[Microsoft.SqlServer.Server.SqlUserDefinedAggregate(Microsoft.SqlServer.Server.Format.UserDefined, MaxByteSize = -1)]
> +public struct GroupConcat : Microsoft.SqlServer.Server.IBinarySerialize
> +{
> +    private ArrayList Remembered;
> +    private string Sep;

Nit: Empty line between fields and methods.

@@ +16,5 @@
> +    private string Sep;
> +    public void Init()
> +    {
> +        this.Remembered = new ArrayList();
> +        this.Sep = null;

Instead of doing this here, why not just specify them as defaults above when the fields are specified?

@@ +17,5 @@
> +    public void Init()
> +    {
> +        this.Remembered = new ArrayList();
> +        this.Sep = null;
> +    }

Nit: empty line between each method.

@@ +23,5 @@
> +    {
> +        if (Value.IsNull) return;
> +
> +        // Add the Seperator as the first array element, default empty string if null
> +        if (this.Remembered.Count == 0) this.Remembered.Add((Seperator.IsNull ? "" : Seperator.Value.ToString()));

Why add the separator as an item to the array? Oh, I see, it's for Write. Just manually write out the separator during Write, instead.

@@ +35,5 @@
> +    }
> +
> +    public SqlString Terminate()
> +    {
> +        return new SqlString(string.Join(this.Sep, (string[])this.Remembered.ToArray(typeof(string))));

Doesn't the Remembered array itself have a Join method or something? At the very least I'm sure there's a Linq method to do this.

@@ +38,5 @@
> +    {
> +        return new SqlString(string.Join(this.Sep, (string[])this.Remembered.ToArray(typeof(string))));
> +    }
> +
> +    public void Read(System.IO.BinaryReader r)

Just use System.IO, and don't namespace this whole thing out. Also, let's call it "reader" instead of r.

Also, let's be consistent--local variables and argument names start with a lowercase letter. Everything else can start with an uppercase letter.

@@ +43,5 @@
> +    {
> +        int itemCount = r.ReadInt32();
> +
> +        this.Remembered = new ArrayList();
> +        for (int i = 0; i <= itemCount - 1; i++)

I think you just want i < itemCount, there.

@@ +49,5 @@
> +            // Retrieve the Seperator for use by Terminate()
> +            if (this.Sep == null)
> +            {
> +                this.Sep = r.ReadString();
> +            }

Instead of this, just do another ReadString at the start, before the loop.

@@ +6,5 @@
> +// General Information about an assembly is controlled through the following
> +// set of attributes. Change these attribute values to modify the information
> +// associated with an assembly.
> +[assembly: AssemblyTitle("Bugzilla.Mssql")]
> +[assembly: AssemblyDescription("MSSQL CLR Functions and Procesdures for Bugzilla")]

remove "and Procedures"

@@ +1,1 @@
> +using System;

Needs an MPL header.

@@ +4,5 @@
> +using System.Data.SqlTypes;
> +using Microsoft.SqlServer.Server;
> +
> +
> +public partial class StoredProcedures

So, question, out of curiosity--I'm using to partial classes being filled out by XAML. What's filling out the class here?

@@ +12,5 @@
> +    {
> +        if (string.IsNullOrEmpty(sqlTable.Value) || string.IsNullOrEmpty(sqlColumn.Value))
> +        {
> +            return;
> +        }

I'd imagine that would be an exception, not just a return.

@@ +24,5 @@
> +            SELECT df.name from
> +            sys.tables t
> +            INNER JOIN sys.default_constraints df ON df.parent_object_id = t.object_id
> +            INNER JOIN sys.columns col ON parent_column_id = column_id
> +            WHERE col.name = @column AND t.name = @table";

That doesn't need to be inside of the using block. In fact, it can be a constant at the top of the class.

Also, this should follow the standard Bugzilla SQL formatting, as described in the Developers Guide.

@@ +29,5 @@
> +            using (SqlCommand sqlCommand = new SqlCommand(Query, sqlConn))
> +            {
> +                sqlCommand.Parameters.AddWithValue("@column", sqlColumn.Value);
> +                sqlCommand.Parameters.AddWithValue("@table", sqlTable.Value);
> +                SqlDataReader sqlReader = sqlCommand.ExecuteReader();

You could just use ExecuteScalar instead, no?

@@ +33,5 @@
> +                SqlDataReader sqlReader = sqlCommand.ExecuteReader();
> +                if (sqlReader.HasRows)
> +                {
> +                    sqlReader.Read();
> +                    oldSqlDefaultName = sqlReader[0].ToString();

If this block stays, wouldn't it be simpler to use one of the Get functions available on SqlDataReader?

@@ +51,5 @@
> +
> +            if (string.IsNullOrEmpty(sqlNewDefault.Value))
> +            {
> +                // Assumes that we were removing the existing default and not replacing it, so we are done and can return here.
> +                return;

Ah, but an empty string is a valid new default. (It's not NULL, after all.)

@@ +54,5 @@
> +                // Assumes that we were removing the existing default and not replacing it, so we are done and can return here.
> +                return;
> +            }
> +
> +            string AddQuery = @"ALTER TABLE " + sqlTable.Value + " ADD DEFAULT('" + sqlNewDefault.Value+ "') FOR [" + sqlColumn.Value + "]";

Ah, that's a SQL injection waiting to happen--sqlNewDefault definitely needs to be in a placeholder.

@@ +93,5 @@
> +                    sqlReader.Read();
> +                    oldSqlDefaultName = sqlReader[0].ToString();
> +                }
> +                sqlReader.Close();
> +            }

Is all of this code a cut-and-paste from above? If so, it should be factored out into a separate method.

@@ +102,5 @@
> +                using (SqlCommand sqlCommand = new SqlCommand(DropQuery, sqlConn))
> +                {
> +                    sqlCommand.ExecuteNonQuery();
> +                }
> +            }

In fact, it looks like all through here, the code is identical to the code above, so it should be factored out.

@@ +118,5 @@
> +    public static void AlterPrimaryKey(SqlString sqlTable, SqlString sqlNewPrimaryKeyColumn)
> +    {
> +        if (string.IsNullOrEmpty(sqlTable.Value) || string.IsNullOrEmpty(sqlNewPrimaryKeyColumn.Value))
> +        {
> +            return;

Probably should be an exception instead of returning.

@@ +130,5 @@
> +            string Query = @"
> +            SELECT k.name FROM
> +                sys.tables t
> +                INNER JOIN sys.key_constraints k ON k.parent_object_id = t.object_id
> +            WHERE t.name = @table AND  k.type = 'PK'";

Should go into a constant, and be re-formatted per our SQL guidelines.

@@ +140,5 @@
> +                {
> +                    sqlReader.Read();
> +                    oldSqlPrimaryKeyName = sqlReader[0].ToString();
> +                }
> +                sqlReader.Close();

Another possible case for ExecuteScalar?

Comment 4

[Jasmin Sehic]

Thanks for the review. I should have some time later next week to go through this.

Comment 5

[Michael Thomas (Mockodin)]

(In reply to Max Kanat-Alexander from comment #3)
I didn't comment on everything just a few the popped out to me.

>Why isn't there a namespace?
Visual Studio for MSSQL functions defaults to not using one.

>So, question, out of curiosity--I'm using to partial classes being filled out by
>XAML. What's filling out the class here?
Again default for Visual Studio

>do you really need MaxSize = -1 explicitly there?
yes, else it cut off at 8000 characters. To me its a bug but until MS decides to add direct specification of or a keyword for 'MAX' it must be. Unless someone knows how to globally set that.. Or MS released something in a service pack or hotfix, as of VS 2010 sp1 it was still an issue.

>So, out of curiosity, why is every function in here static? Is that just how >these things work with SQL Server?

As I recall it's required.

>definitely needs to be in a placeholder.
Keep in mind MSSQL doesn't allow for placeholders for somethings, so unless the driver is handling the entire process, which I don't believe it is.. that might not be possible.. If it is then agreed.