Added support for a default collation for all tables.

[magnusbakken]

Added support for setting a default collation that applies by default to all string fields without a specific collation. CollationAttribute takes precedence.

This PR is a proof of concept. The API for the default collation should probably look different, and the implementation could no doubt be stronger as well.

[msallin]

Thank you for your contribution.
It LGTM. No breaking change and the code is clean.

However, you write "The API for the default collation should probably look different, and the implementation could no doubt be stronger as well." Is there anything specific which bothers you and should be discussed?

[magnusbakken]

I think it's probably okay, but I threw it together pretty quickly, without considering many alternatives.

Perhaps the biggest issue would be a lack of discoverability. Most people will probably use this library by doing

Database.SetInitializer(new SqliteCreateDatabaseIfNotExists(modelBuilder));

Now you have to know to do

Database.SetInitializer(new SqliteCreateDatabaseIfNotExists(modelBuilder) { DefaultCollation = ... });

Maybe it would be better if the collation data came as an optional parameter to the SqliteCreateDatabaseIfNotExists constructor? Or both?

The other thing is the ICollationData interface. I added it so CollationAttribute and CollationData could be handled with the same code internally, but there's no reason to pass a CollationAttribute object around manually, so maybe it would be better to just use CollationData directly? The interface could still be there, but it would be more obvious how to pass the input data.

Also, maybe CollationOptions instead of CollationData? Maybe the classes should be DefaultCollationOptions and CollationAttribute, and the interface is ICollationData?

Or maybe all of this is just splitting hairs, but you're probably in a better position to say what's right for your library. 😃

[magnusbakken]

Or simply DefaultCollation and ICollation?

[msallin]

Thank you for your explanations I had a more granular look at it.

Or maybe all of this is just splitting hairs, but you're probably in a better position to say what's right for your library.

Well, I didn't use this library for years. Chances are high that others known it better than I do ;)

I suggest doing the following:

• Rename ICollationData to ICollation (and also the class)
• Move ICollation and Collation out of the Attributes namespace to the root of Public
• Make the property SqliteInitializerBase get only and pass Collation as optional parameter (maybe not expect the interface but the concrete class because there is no benefit of using the interface but it is less obvious for the user what to pass)
• The code in the attribute does ensure that a user does correctly use the API. Hence, this code should also be in the Collation class. However, code duplication is bad. What about moving this all to the Collation class and instantiate this class in the attribute ctors. Then either just forward the Collation properties or just expose the Collation instance as a property. In the second case, the interface isn't necessary anymore.

Let me know what you think.

[magnusbakken]

Sounds good. 👍 I'll make the changes soon.

[magnusbakken]

A few notes:

• You can't have a property with the same name as the containing class, so after renaming the class to just Collation I had to change the property name to CollationFunction. Only on the new class, of course. On the attribute class it still has the same name.
• I moved the CollationFunction enum out of attributes as well, since it feels like it belongs next to the Collation class.
• I believe it's currently possible to add CollateAttribute to columns that aren't string types, and the library will emit the given collation function for that column. I don't think this would ever make sense to do with SQLite, so in my current code I've modified the behavior so such attributes are silently ignored:

if (property.PrimitiveType.PrimitiveTypeKind != PrimitiveTypeKind.String)
{
    return;
}

var collateAttribute = property.GetCustomAnnotation<CollateAttribute>();
var value = collateAttribute == null ? defaultCollation : collateAttribute.CollationData;

if (value != null)
{
    columnStatement.ColumnConstraints.Add(new CollateConstraint { CollationFunction = value.CollationFunction, CustomCollationFunction = value.Function });
}

If it's necessary to maintain the existing behavior, the function could be written like this instead:

Collation value = null;

var collateAttribute = property.GetCustomAnnotation<CollateAttribute>();
if (collateAttribute != null)
{
    value = collateAttribute.CollationData;
}
else if (property.PrimitiveType.PrimitiveTypeKind == PrimitiveTypeKind.String)
{
    value = defaultCollation;
}

if (value != null)
{
    columnStatement.ColumnConstraints.Add(new CollateConstraint { CollationFunction = value.CollationFunction, CustomCollationFunction = value.Function });
}

The first version would be a breaking change, if adding CollateAttribute to non-strings is ever valid. But if that's the case I may also have to reconsider how the default collation works.

A third option could be to explicitly throw an exception if CollateAttribute has been added to a field where it doesn't make sense.

[msallin]

Wouldn't it make sense to name this CustomFunction and the property CollationFunction -> Function as this is in a class with the name Collation?

[magnusbakken]

Yes, agreed.

[msallin]

I suggest to move all code from the attribute to the collation class. Because currently one only gets the checks when using the attribute and not when using the class. Also remove all setters but only use the ctor, to guide the usage.

Like this (I also renamed it according to my comment in the PR):

    [AttributeUsage(AttributeTargets.Property)]
    public sealed class CollateAttribute : Attribute
    {
        public CollateAttribute()
        {
            Collation = new Collation();
        }

        public CollateAttribute(CollationFunction function)
        {
            Collation = new Collation(function);
        }

        public CollateAttribute(CollationFunction function, string customFunction)
        {
            Collation = new Collation(function, customFunction);
        }

        public Collation Collation { get; }
    }

    public class Collation
    {
        public Collation()
            : this(CollationFunction.None)
        {
        }

        public Collation(CollationFunction function)
            : this(function, null)
        {
        }

        public Collation(CollationFunction function, string customFunction)
        {
            if (function != CollationFunction.Custom && !string.IsNullOrEmpty(customFunction))
            {
                throw new ArgumentException("If the collation is not set to CollationFunction.Custom a function must not be specified.", nameof(function));
            }

            if (function == CollationFunction.Custom && string.IsNullOrEmpty(customFunction))
            {
                throw new ArgumentException("If the collation is set to CollationFunction.Custom a function must be specified.", nameof(function));
            }

            Function = customFunction;
            CollationFunction = function;
        }

        public CollationFunction CollationFunction { get; set; }

        /// <summary>
        /// The name of the custom collating function to use (CollationFunction.Custom).
        /// </summary>
        public string Function { get; set; }
    }

To your question:
Only apply the default collation to string column seems reasonable. I would also suggest to throw an exception if one tries to add the collation attribute to a string column rather than just ignoring it.

[msallin]

You might add also an additional bullet point to the readme smth like:

Default collation (pass an instance of Collation as constructor parameter for an initializer to specify a default collation).

[magnusbakken]

I made the changes you recommended. Minor points:

• CollateAttribute now no longer has public properties called Collation and Function, which is a very minor breaking change for anybody who was manually accessing those properties in their code (probably nobody).
• I'm now throwing an InvalidOperationException when there's a CollateAttribute but the underlying property type isn't String. I wanted to write a test for this code path, but it would require a separate example model, which is a little cumbersome. Should I add one anyway?

[msallin]

CollateAttribute now no longer has public properties called Collation and Function, which is a very minor breaking change for anybody who was manually accessing those properties in their code (probably nobody).

Thank you for the hint. I'm aware of it. In theory this should result in incrementing the major version. However, I don't think this is used from someone, hence I will increment the minor.

I'm now throwing an InvalidOperationException when there's a CollateAttribute but the underlying property type isn't String. I wanted to write a test for this code path, but it would require a separate example model, which is a little cumbersome. Should I add one anyway?

Fine for me if you don't.