Proposed change to GetColumnDefinition in OrmLiteDialectProviderBase.cs

ServiceStack.OrmLite
1

I’m currently working on adding some new features to the ServiceStack.OrmLite.SqlServer project. Unfortunately, I have run into an architectural hurdle and would like to propose a change to the GetColumnDefition method in OrmLiteDialectProviderBase.

The current definition for this method is:

public virtual string GetColumnDefinition(string fieldName, Type fieldType,
            bool isPrimaryKey, bool autoIncrement, bool isNullable, bool isRowVersion,
            int? fieldLength, int? scale, string defaultValue, string customFieldDefinition)

and just about every use of this method looks like this:

var columnDefinition = GetColumnDefinition(
                            fieldDef.FieldName,
                            fieldDef.ColumnType,
                            fieldDef.IsPrimaryKey,
                            fieldDef.AutoIncrement,
                            fieldDef.IsNullable,
                            fieldDef.IsRowVersion,
                            fieldDef.FieldLength,
                            fieldDef.Scale,
                            GetDefaultValue(fieldDef),
                            fieldDef.CustomFieldDefinition);

As you can see, every method parameter is derived from the field definition. My proposal is to change this to the following:

var columnDefinition = GetColumnDefinition(FieldDefinition fieldDef);

This change will allow the downstream dialect providers to have more information around the field definition, so that when it does have to be overwritten, there is access to properties like PropertyInfo, IsClustered, IsIndexed, IsUnique, etc.

I am posting this as a forum topic because I would like to know if there was a reason why this approach wasn’t initially taken? Plus, would there be any adverse effects from this change? I’m only familiar with MySql and SqlServer, so I’m not sure what advantages or disadvantages, if any, this would have on other providers.

I believe the below change could be immediately made without having to update everything plus backwards compatibility can be maintained:

public virtual string GetColumnDefinition(FieldDefinition fieldDef)
{
    var fieldName = fieldDef.FieldName;
    var isPrimaryKey = fieldDef.IsPrimaryKey;
    var fieldType = fieldDef.FieldType;
    var autoIncrement = fieldDef.AutoIncrement;
    var isNullable = fieldDef.IsNullable;
    var isRowVersion = fieldDef.IsRowVersion;
    var fieldLength = fieldDef.FieldLength;
    var scale = fieldDef.Scale;
    var defaultValue = GetDefaultValue(fieldDef);
    var customFieldDefinition = fieldDef.CustomFieldDefinition;

// Existing code .... 
}

public virtual string GetColumnDefinition(string fieldName, Type fieldType,
    bool isPrimaryKey, bool autoIncrement, bool isNullable, bool isRowVersion,
    int? fieldLength, int? scale, string defaultValue, string customFieldDefinition)
{
    var fieldDef = new FieldDefinition()
    {
        Name = fieldName,
        FieldType = fieldType,
        IsPrimaryKey = isPrimaryKey,
        AutoIncrement = autoIncrement,
        IsNullable = isNullable,
        IsRowVersion = isRowVersion, 
        FieldLength = fieldLength, 
        Scale = scale,
        DefaultValue = defaultValue, 
        CustomFieldDefinition = customFieldDefinition
    };

    return GetColumnDefinition(fieldDef);
}
2

It’s to allow RDBMS providers to customize what Column Definition they want which doesn’t always map 1:1 to a Field Definition, e.g in VistaDbDialectProvider:

    var columnDefinition = this.GetColumnDefinition(
        fieldDef.FieldName,
        fieldDef.ColumnType,
        false, // isPrimaryKey
        fieldDef.AutoIncrement,
        fieldDef.IsNullable,
        fieldDef.IsRowVersion,
        fieldDef.FieldLength,
        fieldDef.Scale,
        GetDefaultValue(fieldDef),
        fieldDef.CustomFieldDefinition);

    columns.Append(columnDefinition);

    if (fieldDef.IsPrimaryKey)
    {
        constraints.AppendFormat("ALTER TABLE {0} ADD CONSTRAINT {1} PRIMARY KEY ({2});\n",
            quotedTableName,
            this.GetQuotedName("PK_" + modelDefinition.ModelName),
            this.GetQuotedColumnName(fieldDef.FieldName));
    }

But we could probably add fieldDef as an optional argument to give RDBMS providers the opportunity to access additional metadata, it just shouldn’t be relied upon in the base Dialect Provider. Would the additional fieldDef argument work for you?

3

Since GetColumnDefinition is almost always a 1:1 relationship with the Field Definition, I would make it a rule and work through the exceptions. This would simplify most calls to the GetColumnDefinition as there seem to be very few exceptions.

So, to handle your provider example, I add the following private method to the VistaDbDialectProvider:

private string GetColumnDefinitionWithoutPK(FieldDefinition fieldDef) 
{
    fieldDef.IsPrimaryKey = false;
    return base.GetColumnDefinition(fieldDef);
}

and then change the existing code like so:

....

var columnDefinition = GetColumnDefinitionWithoutPK(fieldDef);

columns.Append(columnDefinition);

if (fieldDef.IsPrimaryKey)
{
    constraints.AppendFormat("ALTER TABLE {0} ADD CONSTRAINT {1} PRIMARY KEY ({2});\n",
        quotedTableName,
        this.GetQuotedName("PK_" + modelDefinition.ModelName),
        this.GetQuotedColumnName(fieldDef.FieldName));
}

....

This change will provide the same final result, while simplifying a critical method that has 10 parameters down to 1 parameter, plus make it more extensible - just my two cents.

However, as you recommended, adding an optional FieldDefinition parameter would fit my needs.

4

Also, I wanted to also point out, if the GetColumnDefinitionWithoutPK is only used once within the provider, we can simplify it even further and just do this:

....

var fieldDefWithoutPK = fieldDef;
fieldDefWithoutPK.IsPrimaryKey = false;

var columnDefinition = GetColumnDefinition(fieldDefWithoutPK);

columns.Append(columnDefinition);

if (fieldDef.IsPrimaryKey)
{
    constraints.AppendFormat("ALTER TABLE {0} ADD CONSTRAINT {1} PRIMARY KEY ({2});\n",
        quotedTableName,
        this.GetQuotedName("PK_" + modelDefinition.ModelName),
        this.GetQuotedColumnName(fieldDef.FieldName));
}

....
5

Mutating a FieldDefinition instance isn’t ThreadSafe and the calls still aren’t the same because of the GetDefaultValue() custom function. It’s still doable by cloning the FieldDefinition, but it should also needs to backwards-compatible as some people have custom dialect providers and others may be using the existing method, which we can also maintain with a cloned instance.

6

I’ve changed everything over to use the new GetColumnDefinition(fieldDef) in this commit.

This change is available from v4.5.5 that’s now available on MyGet.

7

Thanks Demis! Elegant approach.

1 Like