Prevent leading or multiple underscores in Migration class names from causing problems

[rogersillito]

A migration class must both inherit from Migration, and be decorated with MigrationAttribute to be included in a migration run. Without the second check, a Migration base class would be included even if it wasn't decorated as a migration. This fixes #18.

To address #20, the GetMigrationIdFromName method of RavenMigrationHelpers now ensures that the ids of each MigrationDocument created cannot contain adjacent IdentityPartsSeparator chars.

[dportzline83]

The bit about replacing multiple underscores with a single forward-slash could be a breaking change for a system that already has migrations logged in the database that were named the "old" way. I'm not sure it's a good idea to change how the id is determined at this point. Thoughts?

[brunomlopes]

An option would be to have a default function that transforms a migration (or migration properties) into an id, and back again. That way you could maintain backwards compatibility for people with old migrations, but still move forward with new features that need different ids.

[rogersillito]

If there is an existing migration named in this way then it will have been continually re-applied at each migrate anyway (the underlying issue). As such I suspect anyone having this issue will either have problems resulting from re-running the same migration, or have renamed their migration as workaround (as I chose to initially).

I take your point about breaking changes though - I'll take a look at the option idea and see how it could be worked in. My reservation here might be that while it's a good choice technically, it could end up introducing complexity for what is probably an edge case?

[dportzline83]

Ok, maybe I don't quite understand the underlying issue. Is it that Raven strips multiple and trailing forward-slashes when creating the id, so the Raven id never matches the migration id?

[rogersillito]

Yes I think that's my understanding - it didn't seem to get on with ids having adjacent IdentityPartsSeparator chars when the runner checked the store to see if a migration had already been applied.

I dug out the code again to take a look: I was expecting that renaming First_Migration with some extra underscores would expose the issue, but Calling_run_twice_runs_migrations_only_once still passes. I ran into this though when migrating against a real doc store (not Embeddable) - I'll try and reproduce the scenario in the wild and report back...

[rogersillito]

There's some inconsistency between the RavenDb builds I've tested against: I've reproduced the issue against builds 2947 and 3660. However against builds 2700 (the same version this project currently uses) and 2750, the migrations with multiple or leading underscores are only applied once. Possibly the behaviour changed and it will affect versions after a certain point? I've contacted the Raven guys to hopefully get some insight.

[dportzline83]

Hey, if you want to put the bit about filtering out the migrations that don't have the attribute into a separate pull request, I think we can merge that in soon. We'll wait until we have more information as to what's going on with the other change.

[rogersillito]

Will get onto it tomorrow.

[rogersillito]

Response from @ayende on the underscores..

IIRC, the issue here is that we are narrowing // to /, and I think that we don't allow // in document names any longer in general.

So the behaviour changed at a certain Raven build... However I think this confirms that moving forwards we shouldn't allow migrations to have document ids named like this.

If I have the right idea around your suggestion @brunomlopes, we would introduce a new runner option to trigger the non-default id conversion function? This would work fine for people who are only migrating against versions either above or below the point where the behaviour change happened, but as soon as you upgrade past the point of change your old migrations with dodgy ids would start to re-migrate each time.

Also, once the newer Raven behaviour applies, there doesn't seem to be a way to retrieve an existing document with the invalid naming (you get null on Load), so we would never get as far as needing to convert back to a Migration class name.

This brings me back to thinking we just accept a breaking changes for anyone who might have done this and not already dealt with the consequences by renaming their migrations - it seems better than the alternative?

[dportzline83]

It sounds like the breaking change was made in RavenDb first, so we'll need a breaking change to compensate. I suppose it may still require a major version bump in this project, but oh well.

[brunomlopes]

I think that with a new runner option to override the id generator for migrations a user could "lock down" how they're generating the ids for migrations. For me this is relevant because our versions are x.y.w.z, and we generate ids a bit differently to make them easier to parse when using the studio.

For people caught in the middle, yeah, they need to first migrate their docs. They might get away with some scripted patches over Raven/DocumentsByTag before running the migrations proper, at least the first time.

[brunomlopes]

Also, @dportzline83 , it looks like there are a couple of large changes in the PRs, so it looks like "a major version bump" might be warranted :)

[rogersillito]

I can see the value in this, although it's probably separate from fixing the current default behaviour. I've submitted a separate PR https://github.com/migrating-ravens/RavenMigrations/pull/30 - is this what you were after?

[dportzline83]

So, my main concern with the rest of this PR is that I'm not sure we really want the migration library to "guess" for you and sanitize your migration id automatically. What do you think about just warning the user if they use a malformed name for the migration? The warning could mention the version of RavenDb which introduces that problem and how to solve it.

[rogersillito]

I don't see that is a big issue personally - the library already has responsibility for transforming the migration class name into the migration id. Also the migration documents are the domain of the framework, so it seems right it should ensure they work correctly in all situations.

It took me some time to figure out what was going on when I first came across this - avoiding that situation seems like the better choice. Having said that, if a warning were to appear (possibly using the logging functionality in #22?) that would still be better than the present situation if you feel strongly about it?

I had some follow-up chat with Oren about the issue - it appears that matching the server and client builds avoids the problem: e.g. a build 2947 client connecting to a 2947 server successfully returns a document when doing session.Load<MigrationDocument>("ravenmigrations//a/b//c"). I don't think this helps us though - we can't rely on the framework version matching whatever instance it is migrating against so we're back with either avoiding (or warning against) the situation.

[dportzline83]

Yeah, I can see your point about the migration documents being the domain of the framework. That's also interesting about the client and server versions matching. Ideally they would always match. But I guess we can't guarantee that. I'm fine with bringing this in along with some of the other breaking changes that are coming.

[dportzline83]

So, #30 is an alternative approach to the remaining changes proposed here, correct? Can you resolve the merge conflicts here so that the proper diff is displayed? Or you could open a new PR for the proposed document id conversion changes here, and we can close this one.

[rogersillito]

So, #30 is an alternative approach to the remaining changes proposed here, correct?

Not quite - it gives a way of providing an alternative way of generating a migration id (so could be used to avoid the problem assuming you are aware of it up-front!), but the default migration id conversion will still lead to issues given migration classes with problem naming. I believe #30 will work in conjunction with 1f7c5887aa5b638c5311a762f8b374f093de53f7 - if I understand that right an alternative migration id conversion could be provided to work with the new versioning approach.

...you could open a new PR for the proposed document id conversion changes here, and we can close this one.

I'll do that - it will be simpler.

[rogersillito]

Actually, once i'd merged in the latest upstream changes this PR now only contains the document id conversion changes, so it's good to go: I've renamed it to make it more descriptive.

[dportzline83]

Perfect. I think I'll bring this in as part of the next major release.