Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow setting the schema for an entire migration class #290

Closed
dennisdoomen opened this issue Aug 15, 2012 · 17 comments
Closed

Allow setting the schema for an entire migration class #290

dennisdoomen opened this issue Aug 15, 2012 · 17 comments
Labels
improvement Improvement of behavior or code quality

Comments

@dennisdoomen
Copy link

Currently, you have to repeatingly call InSchema() in every statement. It would be nice to be able to associate the migration class with a particular schema. E.g. by overriding the Schema property from the MigrationBase.

@daniellee
Copy link
Contributor

Thinking of doing this as a convention instead. See this discussion: https://groups.google.com/forum/?fromgroups=#!searchin/fluentmigrator-google-group/schema/fluentmigrator-google-group/RtoPUMWSoVY/9k9Mt2EnicIJ

Although, I'll have to create an extension point so that this can be used by the console, MsBuild and Nant runners as well.

@dennisdoomen
Copy link
Author

That would be exactly what I need. We created our own .exe that invokes the FluentMigrator test runner for different schemas (using dynamic loading of DLLs with marker interfaces). It’s quite annoying to have to specify the schema for each and every command.

From: Daniel Lee [mailto:notifications@github.com]
Sent: donderdag 4 oktober 2012 14:55
To: schambers/fluentmigrator
Cc: Dennis Doomen
Subject: Re: [fluentmigrator] Allow setting the schema for an entire migration class (#290)

Thinking of doing this as a convention instead. See this discussion: https://groups.google.com/forum/?fromgroups=#!searchin/fluentmigrator-google-group/schema/fluentmigrator-google-group/RtoPUMWSoVY/9k9Mt2EnicIJ

Although, I'll have to create an extension point so that this can be used by the console, MsBuild and Nant runners as well.


Reply to this email directly or view it on GitHubhttps://github.com//issues/290#issuecomment-9140167.

@ashes999
Copy link

ashes999 commented May 2, 2013

+1

@daniellee
Copy link
Contributor

I'm working on this and will submit a PR soon (see my branch). Just trying to figure out the best way to override/set properties on the MigrationConventions class. I was thinking I would do it in the same way as for the VersionTableMetaData. But that won't be possible unfortunately as every convention is a func. So maybe setting them in the constructor is the easiest way.

Then I just have to figure out how to test this properly.

@daniellee
Copy link
Contributor

Think I'm finished. Just going to test it a bit more and think about any potential bad side effects. For example, setting the default schema before you execute the first migration is an interesting edge case. But that will have to be tomorrow.

@ashes999
Copy link

ashes999 commented May 9, 2013

@daniellee from a usability perspective, that would be exactly the best case. Setting the schema once per migration is (for me, at least) a workable interm solution.

Looking forward to trying this out. I actually switched from migratordotnet just because my production server used a different schema name, and my migrations broke.

@dennisdoomen
Copy link
Author

+1

@daniellee
Copy link
Contributor

The problem with setting the default schema before you execute the first migration is that you end up in a chicken and egg situation. The first migration to be run will create the VersionInfo table (the table that keeps track of which migrations have been applied) and it will try and create it under the default schema. Which doesn't exist yet. And will therefore crash.

I'm thinking that the VersionInfo table is a special case and should not have the default schema convention applied to it. Instead if the user wants to change the schema for the VersionInfo table then they should create custom metadata for it.

You can also potentially mess things up by changing the Foreign Key naming conventions or Index naming conventions but there's not much I can do about that.

@dennisdoomen
Copy link
Author

That's how we do it now. And I would assume that all migrations within the same assembly would use that same schema (unless overridden in the specific migration).

@ashes999
Copy link

@daniellee I had exactly that problem on my production hosting. The hosting company already had a VersionInfo table under their schema. I couldn't create it. If the VersionInfo table can't use the default schema, that'll cause a problem for others in the same situation.

Maybe I'm oversimplifying, but to look at T-SQL as an example, isn't applying the default schema as simple as a USE SchemaName statement that you execute before running any migrations?

Also, it seems that specifying the schema in the first migration also specifies it for the VersionInfo table. If that behaviour doesn't go away, it's pretty simple to set the default schema for everything (use this in conjunction with the new default schema specifier).

@daniellee
Copy link
Contributor

Got a bit stuck this with change. There are already some pull requests that partially solve the problem of exposing the MigrationConventions. #287 does it by creating a few factories that are extensible. #386 has its own MigrationConventionsBase that wraps the MigrationConventions class. If I take this approach then I would inherit from the class and override the appropriate method much like you need to do for the VersionInfo table.

The other possible approach is what I have right now on my branch. It keeps the existing structure of the MigrationConventions class so if I want to change some behavior in it then I have to inherit the class and set a property in the constructor.

I'm not too keen on the factory approach so I'll show examples of the other two approaches. These assume that you want to keep most of the default conventions and just change the DefaultSchema so both examples inherit from MigrationConventions.

First, via the constructor and keeping delegates:

public class MyConventions: MigrationConventions
{
    public MyConventions()
    {
        GetDefaultSchema = () => "test";
    }
}

Second, removing delegates and changing properties to be methods and making them overridable:

public class MyConventions: MigrationConventions
{
    public override string GetDefaultSchema()
    {
        return "test";
    }
}

This is also not as black and white is it might seem. The second approach seems simpler for the end-user but does complicate some parts of the code as you can't switch out conventions at runtime. I've been looking at Nancy and at how they do this and they use a lot of delegates and actions but also sometimes just simple overrides of virtual methods. Their Bootstrapper class approach is maybe something we should be copying if we want to break FluentMigrator up into smaller parts.

So what do you all think? I have two branches and both approaches work but once this is released then it will be hard to change without breaking stuff.

P.S. I am thinking about adding ApplicationContext as a readonly property to the MigrationConventions class as that adds some more flexibility that you get with the factory approach in #287. Anything else that would be handy to have as a readonly property?

@ashes999
Copy link

ashes999 commented Jun 6, 2013

I know you only want one of the two approaches, but I have a third to offer up.

First, I solved this problem by creating a constant called CurrentSchema in a helper class; I simply refer to it and use .InSchema(CurrentSchema) in all my migrations. Error-prone (which migration did I forget it in?), but does the job.

What about just exposing a simple string Schema property in the Migration class, and allowing users to set the value? It would be trivial to create a subclass (MySchemaMigration), set it once in the constructor (or a static constructor), and derive all my migrations from the MySchemaMigration class. Schema name is in one location, and I get it for free in all my migrations.

Would that be simpler? It seems like there isn't a schema attribute currently in the migration class.

@tommarien
Copy link
Contributor

@daniellee any update on the pull you where working on ?

@tommarien
Copy link
Contributor

@daniellee Recently i was thinking about the ability to change conventions

  • It should be possible to alter fluentmigrator's default extensions: maybe with a class simular to how versionmetadata is working

[MigrationSetup]
public class MyMigrationSetup : IMigrationSetup
{
      public void Setup(IMigrationSomething context)
     {
            context.Conventions.AddOrReplace<PrimaryKeyNamingConvention>(new MyCustomerPrimaryKeyNamingConvention());
     }
}

  • It should be possible to override conventions per migrations
    Every migration gets a copy of the original migration conventions collection, we just need to add a virtual method so users can override them per migration

protected override SetupConventions(Imigrationconventions conventions)
{
   conventions.AddOrReplace<DefaultSchemaConvention>(new DefaultSchemaConvention("SchemaName");
}

What do you think ?

@monnster
Copy link

As I see there's still no solution, so for those who's looking for a workaround on this issue:
You will need to use some AOP library, I used LightInject.Interception.

First create a base class for your migrations and replace Create, Insert and Delete properties so that they return a proxy with our interceptor:

public abstract class DatabaseMigrationBase: Migration
{
    public static readonly string DefaultSchema;
    private IServiceContainer _container;

    static DatabaseMigrationBase()
    {
        DefaultSchema = "your_schema"; // load from config
    }

    protected DatabaseMigrationBase()
    {
        _container = new ServiceContainer(new ContainerOptions { EnableVariance = false });

        _container.Register<ICreateExpressionRoot>(_ => BaseCreate(), new PerContainerLifetime());
        _container.Register<IInsertExpressionRoot>(_ => BaseInsert(), new PerContainerLifetime());
        _container.Register<IDeleteExpressionRoot>(_ => BaseDelete(), new PerContainerLifetime());

        _container.Intercept(
            sr => sr.ServiceType == typeof(ICreateExpressionRoot), 
            sf => new SetDefaultSchemaInterceptor(DefaultSchema));

        _container.Intercept(
            sr => sr.ServiceType == typeof(IInsertExpressionRoot),
            sf => new SetDefaultSchemaInterceptor(DefaultSchema));

        _container.Intercept(
            sr => sr.ServiceType == typeof(IDeleteExpressionRoot),
            sf => new SetDefaultSchemaInterceptor(DefaultSchema));
    }

    /// <summary>
    /// Replace base methods so that proxy with our interceptor is returned.
    /// </summary>
    public new ICreateExpressionRoot Create
    {
        get { return _container.GetInstance<ICreateExpressionRoot>(); }
    }

    public new IInsertExpressionRoot Insert
    {
        get { return _container.GetInstance<IInsertExpressionRoot>(); }
    }

    public new IDeleteExpressionRoot Delete
    {
        get { return _container.GetInstance<IDeleteExpressionRoot>(); }
    }

    private ICreateExpressionRoot BaseCreate()
    {
        return base.Create;
    }

    private IInsertExpressionRoot BaseInsert()
    {
        return base.Insert;
    }

    private IDeleteExpressionRoot BaseDelete()
    {
        return base.Delete;
    }
}

Now implement interceptor which checks every call of IXxxCompositionExpressionRoot members and searches for InSchema methods in its results:

public class SetDefaultSchemaInterceptor: IInterceptor
{
    private string _defaultSchema;

    public SetDefaultSchemaInterceptor(string defaultSchema)
    {
        _defaultSchema = defaultSchema;
    }

    public object Invoke(IInvocationInfo invocationInfo)
    {
        // execute any method of ICreateExpressionRoot and get it's result
        var result = invocationInfo.Proceed();

        // check if return type is not void
        if (null == result || string.IsNullOrEmpty(_defaultSchema))
            return result;

        // get all methods named InSchema
        var inSchemaMethods = result.GetType()
                .GetMethods()
                // add interface members implemented by type explicitly
                .Union(GetExplicitMethods(result.GetType()))
                .Where(m => m.Name == "WithSchema" || m.Name == "InSchema" || m.Name.EndsWith(".InSchema"));

        foreach (var method in inSchemaMethods)
        {
            // check signature - one string parameter
            var parameters = method.GetParameters();
            if (parameters.Count() == 1 && parameters.First().ParameterType == typeof (string))
            {
                // if ok - set schema
                method.Invoke(result, new object[] { _defaultSchema });
            }
        }

        return result;
    }

    private MethodInfo[] GetExplicitMethods(Type type)
    {
        return type
            .GetInterfaces()
            .SelectMany(t => type.GetInterfaceMap(t).InterfaceMethods)
            .ToArray();
    }
}

Now you may derive your migrations from DatabaseMigrationBase and use methods Create/Insert/Delete without specifying schema.

@SlyNet
Copy link

SlyNet commented Jul 29, 2016

Why does migrator is ignoring the Search Path option from the connection string? Is it an intended behavior?

@fubar-coder
Copy link
Member

Fixed by #772

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improvement of behavior or code quality
Projects
None yet
Development

No branches or pull requests

7 participants