Migration.Insert.Row where a value is Enum

[MihailsKuzmins]

I haven't found a similar issue, but please redirect me if it actually exists. (I apologise if I've created a duplicate by accident)
I have a problem with this code

public enum myEnum
{
    Value1 = 12334
} 

var values = new Dictionary<string, object>
{
    { "intColumn", myEnum.Value1 }
};

Insert.IntoTable("myTable")
    .Row(values);

The error I get: !!! Incorrect integer value: 'Value' for column 'intColumn' at row 1
So it means that the runner transforms enums into their string values instead of their underlying numeric type which is not the right behaviour.
Would it be possible to take a look at this case? I can possibly try to do it as well

[jzabroski]

Would it work if you cast the value?

public enum myEnum
{
    Value1 = 12334
} 

var values = new Dictionary<string, object>
{
    { "intColumn", (int)myEnum.Value1 }
};

Insert.IntoTable("myTable")
    .Row(values);

[MihailsKuzmins]

@jzabroski, thank for the reply.
Of course it does, but it makes my code a bit clumsy and unnatural.
Do you think that there might be problems with enum handling?

[jzabroski]

@MihailsKuzmins There is no enum handling . The code does not do any coercion.

If you find it unnatural, I would suggest creating a wrapper syntax, where you traverse the dictionary and update object values from enum values to integral storage values. However, you might want to also handle the underlying storage of the enum type, when you perform your cast.

public enum myEnum : short
{
    Value1 = 12
} 

I would think of this transform as similar to using Linq and calling Cast on an Array.

[jzabroski]

If you want to submit a PR for handling enums, you can, but you need to handle all possible integral storage types in your unit tests. Thank you,

[MihailsKuzmins]

@jzabroski
I'm thinking about creating a PR for enum handling. There is one question I would like to ask about a possible implementation.
Of course the easiest way is to do it in InsertDataExpressionBuilder, but it will result for an unnecessary mapping from Dictionary to Dictionary for Row(IDictionary<string, object> data) if this dictionary is ImmutableDictionary. Or if a dictionary is mutable than it is possible to map only values dictionary["propertyName"] = value.
Another option is to avoid it, so enum handling could be implemented where rows are enumerated over, but it requires changes in more than one classes (of course handling will be an extracted extension method).

So, due to the fact that I don't see a good solution, could you please advise, what could be a better approach? One of the aforecited or a completely different one?

Thanks

[jzabroski]

These are always hard guidance to give. Sometimes, I make mistakes, even with 20 years c# experience. Sometimes, you just have to play with it to see what feels right. I did have an idea for creating a "FluentMigrator.Opinions" nuget package (see #1206) which would allow people to export syntax that matches my aesthetic tastes. Since FluentMigrator is over 12 years old, it has the accumulation of many people's tastes over time.

As an example of tastes, some people might want to store enums in the database by their

That said, I would break the problem down into the following, testable pieces:

1. Get underlying storage for an enum type.
2. Write a "pickling method" that can convert an enum value into a either string or integral value
3. Write a pickling method that can convert from a specific built-in data type to sql text

[MihailsKuzmins]

When you mentioned the "pickling method" (serialisation, right?) it gave me an idea to look at GenericQuoter which does look like a perfect place to handle the type.

But looking at the bullet points 2 and 3, unfortunately, again I'm a bit lost.

into a either string or integral value

So you suggest that there should be something like options which determine how to handle enum values? For example in RunnerOptions?

Write a pickling method that can convert from a specific built-in data type to sql text

The Quoter class takes care of it, doesn't it? So the method FormatEnum needs to be implemented accordingly to handle both integral and string values

[jzabroski]

@MihailsKuzmins Sorry I didn't reply back - the day of your last reply, I was sick with COVID-19. Are you still interested in working on this?

[MihailsKuzmins]

@jzabroski oh, I'm sorry. I hope you are doing well now.
I just thought that you didn't have enough time for my never-ending cycle of questions and decided to leave everything as it is 😅.
Yes, I am interested in this issue, so if you could guide me a bit more further and tell me whether or not my approach is good for the FLuentMigrator, I could dive in 😃

[jzabroski]

Cool - let's re-open this. Yeah, I have time (I am just now getting close to 100% strength even though I have been covid negative for like a month). Hoping to take a look at the backlog this weekend and catch up. I think everything you brought up makes sense, but I would need to dive into the code to give exact guidance. You brought up a good point about the pickler needing to handle either numbers or strings.

[MihailsKuzmins]

@jzabroski, I've decided to stop talking and actually write some code, so that we have something to discuss.
I have prepared a PR #1475 were I added QuoterOptions. Since DI is used in the FluentMigrator it seemed like a good idea to inject QuoterOptions into quoters and use options there (hopefully I haven't messed up with constructors 😅).
I would be grateful if someone could review the PR, but there are already 30 open PRs at the moment, so I am patient

[jzabroski]

Most of those PRs are dependabot or more discussion-based PRs... only two or three are real, and one is waiting on someone to refactor it...

I will review your PR, either today or tomorrow. @fubar-coder if you would like to review as well.

[fubar-coder]

@jzabroski The PR is a good first start. This is what came to my mind:

• This change is a breaking change (source-level)
  • A parameterless constructor should be implemented for all direct IQuoter implementations like GenericQuoter
  • The parameterless constructor should be marked as obsolete with a hint that it'll be removed in a future version
  • The first version that would be able to remove the parameterless constructor would be 4.0 if we merge this PR for 3.x
• Maybe now (or in v4.0) is the opportunity to make the quoters injectable
• Maybe we can somehow merge FirebirdOptions and QuoterOptions
  • .NET Core 3.0 contains an options builder API that would (maybe) allow us to access IOptions<QuoterOptions> in the configuration of FirebirdOptions.
• I'm not sure about CreateFixture. Maybe it'd be better to use a separate unit test with a separate quoter configuration that just tests the quoter with a given configuration.

[MihailsKuzmins]

@fubar-coder, thank you for the review

1. I see. I will add default constructors
2. Quoters are already registered in the ServiceProvider (e.g.

   fluentmigrator/src/FluentMigrator.Runner.Firebird/FirebirdRunnerBuilderExtensions.cs

   Line 59 in 0efa70c

   .AddScoped<FirebirdQuoter>()

   ). Or maybe I didn't understand the idea correctly?
3. Since FirebirdOptions are also registered in the ServiceProvider, do you think it's better to do with inheritance or composition?
4. 😄, since [SetUp] does not allow any parameters, I haven't figured out a better way to keep all tests in the same file (I didn't want to create one instance in the setup method, and override it in my test). So I guess you're right. In order to keep the coding style as close to the current one as possible new test classes could be created

[fubar-coder]

First, I'd like to thank you for your work and the effort to improve FM.

Regarding 2: IIRC the injection of quoters, type maps, etc... has some quirks when it comes to using custom implementations and support for multiple databases in the same configuration. @jzabroski Maybe we should drop support for multiple databases in the same service collection and should just add the FM services required for a specific database, based on the current configuration?

Regarding 3: I think that inheritance might be the better choice here, but we can only implement this change if we're dropping all .NET Core versions < 3.0. This is for @jzabroski to decide.

Regarding 4: I understand your point, but this should be decided by @jzabroski too.

[jzabroski]

I reviewed this - I am commenting on 4 as a xUnit.net person who dislikes NUnit. Personally, I think [SetUp] is over-used and adds extra indirection to the code. The one use case for SetUp I quite like, is ability to have a CancellationToken that can time out a set of tests concurrently based on some abstract criteria (wait 30 seconds before timing out the task chain in the test). We don't need that here, though. I can also appreciate the idea that SetUp = Fixture Builder. In my experience, I'd rather have a abstract base class with an abstract fixture, and ideally use a factory to get a concrete fixture per concrete test class. The drawback is that sometimes you end up having to create extra classes that override the Fixture Builder (e.g., for negative test cases).

There's really no right or wrong way to do this - as long as we're consistent. Most of the drawbacks come from the fact that when you build a set of "facts" about your code you're effectively arranging a lattice of judgments about whether some set of truths is accurate/truly true. Introducing any negation is going to break that lattice and cause refactoring.

[jzabroski]

• Maybe we can somehow merge FirebirdOptions and QuoterOptions
  • .NET Core 3.0 contains an options builder API that would (maybe) allow us to access IOptions in the configuration of FirebirdOptions.
    -- @fubar-coder

3. Since FirebirdOptions are also registered in the ServiceProvider, do you think it's better to do with inheritance or composition?
-- @MihailsKuzmins

Regarding 3: I think that inheritance might be the better choice here, but we can only implement this change if we're dropping all .NET Core versions < 3.0. This is for @jzabroski to decide.
-- @fubar-coder

Separately, I pointed out it seems like we're breaking the ABI for FirebirdGenerator by not leaving the old constructor around and marking it as obsolete.

I think that we should not allow three-valued logic for the QuoterOptions - it should be not nullable.

Regarding Microsoft DI FirebirdQuoter - the way Microsoft DI works, you can get in trouble if it's not deterministic how to construct the object, and so you end up using a lambda to register it - but when you do that, I recall the "messy" part is things can "fall through" unexpectedly - I can't remember the exact scenario but lambdas can cause problems based on the order in which you ask for things, I think. Is that what you're referring to? In other words, it would be possible to do a Lambda that registers based on some key passed into the DI extension method. But then people forget to call the extension method to get the right type, so you end up with a lot of support requests with people not reading the docs/code well. Often, it's because there is some blog post or gist out there with a snippet of "how to do things" that isn't kept up to date.

[fubar-coder]

Ok, it seems my explanation wasn't clear enough, so I hacked together this sample code:

var services = new ServiceCollection();

// In user code:
services
	.Configure<QuoterOptions>(s => s.EnumAsUnderlyingType = true)
	.Configure<FirebirdOptions>(s => s.TruncateLongNames = true);

// Inside the AddFirebird extension method:
services
	.AddOptions<FirebirdOptions>()
	.Configure<IOptions<QuoterOptions>>((fo, qo) =>
	{
		// Maybe using AutoMapper or something would be useful here to
		// be sure that the values of all properties are copied from QuoterOptions
		// to FirebirdOptions.
		fo.EnumAsUnderlyingType ??= qo.Value.EnumAsUnderlyingType;
	});

var serviceProvider = services.BuildServiceProvider();
var firebirdOptions = serviceProvider.GetRequiredService<IOptions<FirebirdOptions>>();

// Both EnumAsUnderlyingType and TruncateLongNames are set to "true" here.
firebirdOptions.Dump();

class QuoterOptions
{
	// "null" will be interpreted as "false"
	public bool? EnumAsUnderlyingType { get; set; }
}

class FirebirdOptions : QuoterOptions
{
	public bool TruncateLongNames { get; set; }
	/* ... and more of those */
}

This code change allows to set EnumAsUnderlyingType to be set using the QuoterOptions and/or FirebirdOptions.

We could also keep the EnumAsUnderlyingType as bool if we use the InternalsVisibleToAttribute and the code would change to:

services
	.AddOptions<FirebirdOptions>()
	.Configure<IOptions<QuoterOptions>>((fo, qo) =>
	{
		// Maybe using AutoMapper or something would be useful here to
		// be sure that the values of all properties are copied from QuoterOptions
		// to FirebirdOptions.
		fo._enumAsUnderlyingType ??= qo.Value._enumAsUnderlyingType;
	});

class QuoterOptions
{
	internal bool? _enumAsUnderlyingType;

	public bool EnumAsUnderlyingType
	{
		get => _enumAsUnderlyingType ?? false;
		set => _enumAsUnderlyingType = value;
	}
}

With an InternalsVisibleToAttribute in the FM assembly that allows the Firebird FM assembly to access the _enumAsUnderlyingType.

This would allow us to configure "global" quoter options in QuoterOptions while also using FirebirdOptions for all global and firebird-related options.

However, I'm not entirely sure that this would be the right choice here, as it would also limit us to the Microsoft.Extensions.* libraries of .NET Core 3 and it might be overly complicated and error-prone.

[MihailsKuzmins]

@jzabroski, @fubar-coder thank you for the review!
I have pushed some changes to my code:

• added [Obsolete] constructors for quoters and some other classes, so that the ABI is not broken.
• added a [Theory] method for CanEscapeAString
• marked current constructors of the FirebirdQuoter as [Oboslete], added my new constructors which accept quoter options

For know, I guess, there are two open questions:

1. What to do with the CreateFixture method in tests;
2. Should be try to merge FirebirdOptions (maybe PostgeOptions) as Mark described in the previous comment