Modified One Time Script check false positive

[cabbruzzese]

I encountered what is essentially a false positive on the one time script check that took a while to track down. The USE keyword in SQL causes a string replace before running the script. So running a script on a database named "live" will produce a text_hash with the token "live" replaced all over the file. Running the same script on a backup copy named "live_backup" will replace the same places with the string "live_backup" instead. Even though both databases are the exact same database spun up with the same scripts.

Future migrations made against "live_backup" will fail forever because all onetime scripts with a USE statement will cause hash or checksum differences.

The one time script modified check HAS to be done before the string replace happens. In fact running on the contents of the actual file, not the string being used for the migrartion, is the right answer. This will prevent the issue from taking place because the USE statement will still just be a variable name at that point.

A simple work around to this issue no longer works due to issue #174

[BiggerNoise]

I am having a bit of trouble understanding your use case. I am not sure why you would USE in a migration. Seems very limiting.

Are you able to share part of your script or at least elaborate on what you are trying to accomplish?

Thanks

[cabbruzzese]

Sorry, I was incorrect about this being an issue with a use statement. This entirely a RoundhousE bug. Scripts containing the keyword {{DatabaseName}} will be modified by RoundhouseE before making the check to see if the contents are the same.

The line in question is in TokenReplacerSpecs.cs lines 34-80

Here's an example of one of the functions:

[Observation]
            public void if_given_bracket_bracket_DatabaseName_bracket_bracket_should_replace_with_the_DatabaseName_from_the_configuration()
            {
                TokenReplacer.replace_tokens(configuration, "ALTER DATABASE {{DatabaseName}}").should_be_equal_to("ALTER DATABASE " + database_name);
            }

This string replace is causing a false positive.

On a related note, I'm pretty sure that all scripts generated from SQL Management Studio contain USE statement. It's probably safe to say that all DB compare scripts in MS Visual Studio, and all import/export scripts made by SQL Server will contain USE statements as well. It's part of their typical naming conventions. Although it's unrelated here as all it does is move the use of this variable to one place at the top instead of repeating the variable's use with every statement.

[cabbruzzese]

Sorry to be spammy, but I found the line where it does the token replace before doing the check:

in RoundhouseMigrationRunner line 287 runs a tokenizer but the compare happens much later in database_migrator.run_sql() function on line 289.

Snippit:

public void traverse_files_and_run_sql(string directory, long version_id, MigrationsFolder migration_folder, Environment migrating_environment,
                                               string repository_version, ConnectionType connection_type)
        {
            if (!file_system.directory_exists(directory)) return;

            var fileNames = configuration.SearchAllSubdirectoriesInsteadOfTraverse
                                ? file_system.get_all_file_name_strings_recurevly_in(directory, SQL_EXTENSION)
                                : file_system.get_all_file_name_strings_in(directory, SQL_EXTENSION);
            foreach (string sql_file in fileNames)
            {
                string sql_file_text = replace_tokens(get_file_text(sql_file));
                Log.bound_to(this).log_a_debug_event_containing(" Found and running {0}.", sql_file);
                bool the_sql_ran = database_migrator.run_sql(sql_file_text, file_system.get_file_name_from(sql_file),
                                                             migration_folder.should_run_items_in_folder_once,
                                                             migration_folder.should_run_items_in_folder_every_time,
                                                             version_id, migrating_environment, repository_version, repository_path, connection_type);
                if (the_sql_ran)
                {
                    try
                    {
                        copy_to_change_drop_folder(sql_file, migration_folder);
                    }
                    catch (Exception ex)
                    {
                        Log.bound_to(this).log_a_warning_event_containing("Unable to copy {0} to {1}. {2}{3}", sql_file, migration_folder.folder_full_path,
                                                                          System.Environment.NewLine, ex.to_string());
                    }
                }
            }

            if (configuration.SearchAllSubdirectoriesInsteadOfTraverse) return;
            foreach (string child_directory in file_system.get_all_directory_name_strings_in(directory))
            {
                traverse_files_and_run_sql(child_directory, version_id, migration_folder, migrating_environment, repository_version, connection_type);
            }
        }

[BiggerNoise]

I don't think that I would call this a bug in RoundhousE.

If I have an up script that uses token replacement to create a table of {{table_name}} and I run it with two different values of table_name, then that is two entirely different things being done to the database. It is no different from someone opening up a script that creates a table (without a token replacement) and changing the table name.

The second situation would clearly be a modified script, but the effect is exactly the same as changing the token in the first situation. I don't see a good reason for roundhouse to treat these situations differently.

I think you have a very narrow use case where altering the tokens doesn't have any effect on what's being done within the database. I don't think that it's possible for RH to satisfy your situation and not break for the majority of use cases where the current behavior is correct.

[BiggerNoise]

Regarding auto generated scripts from Microsoft tools.

They are often a great place to start, but I would not use them as is. My roundhouse scripts have these two characteristics:

• They do one thing. Create a table, sproc, index etc. Sometimes, I'll create multiple indexes in a single script, or some indexes when I create a table, but I never put more than one table create in a single script.
• They never "USE" a database. Pointing the scripts at the database is RH's job.

I think that some of your grief is coming from trying to use RH in a way that really isn't intended. If you want to elaborate on where your scripts are coming from and what you're doing with them, I think that we may be able to steer you towards a less painful implementation.

Some ideas to start.

If you have a MS tool that migrates one schema to another, then I am not sure what value RH has in your environment. I know that some of the advanced versions of Visual Studio can do this, and it is my understanding that their functionality would replace what RH does. I have never worked with one of those, so that's about the limit of what I can say there.

If you're looking to create a baseline set of scripts from an exiting database, I think you can put them in the "baseline" folder (I'm working from memory here). These scripts will only be run after a database is created. If you took your dump from SQL studio, removed all of the using statements, and put that here, I think you would be OK. Subsequent alterations go in the up folder.

FWIW, what I did (long ago, when dinosaurs ruled the earth) was take the dump script and spend a morning cutting it up into proper single purpose scripts and put those into the proper places.

[chadcampling]

We have just encountered this same issue with the use of the EnvironmentName token in some scripts. We have restored a production database into our UAT environment are are unable to kick that database with a different Environment Name.

In the way we use Roundhouse we consider that this script hasn't changed and therefore the hash should have been based on the script prior to token replacement however I can see you point of view in regards to what actually ran.

Is there anyway an option could be added to support both usage scenarios?

[cabbruzzese]

There are some work-arounds. What worked for me was to use the same values for all variables and make my changes within the setup. You cannot fix the script to no longer use variables or else it will be considered a new script. (There is absolutely no way to override this without manually mucking with the history tables or starting from scratch)

I would suggest trying to preserve the original variable values. I don't know your setup, but if you can use the same environment name value and possibly run the job in a different path (so that the relative path resolves differently) it will work.

I really want to be nice and not stir up old hard feelings, but I lost A LOT of dev hours to this non-bug and I'm sure you did too. This "feature" needs to be advertised very loudly in the documentation because reaching this point typically means ending use of this product and moving to a different one. (as suggested to me by the devs of this project) There is no actual valid recovery state from this situation. Only bandaids and workarounds.

[BiggerNoise]

@cabbruzzese I hope you don't think that there were any hard feelings on the part of the RH team. Given my limited understanding of what you were trying to accomplish, I was attempting to explain how I thought that the current behavior of RoundhousE was correct.

I'm sorry that you took my invitation to share more details of what you were trying to accomplish so that we could suggest a way to use RH in a less painful way as a dismissal, it certainly wasn't intended as such.

[BiggerNoise]

@chadcampling I'm assuming that the snippet that you posted is from an UP script. If that's the case, you should be able to run RH with the --warnononetimescriptchanges (or something like that). This will re-run the script and compute a new hash for the environment.

I assume that since this is a restoration of a production environment, you actually need this script to run so that the down stream effects of setting the reporting path are put in place for the restored database.

Again, please feel free to elaborate on that script and what it does in your world. There may be a less painful way to use the tool.

[chadcampling]

@cabbruzzese We ended up going with preserving the variable in the script based on the token used in the production database. We had to modify around 15 scripts so our use of the token replacement has been limited thankfully.

@BiggerNoise Thanks for the tip on --warnononetimescriptchanges, I was aware of the switch but for some reason didn't consider that it will re-run the script and update the hash. I thought it would ignore the script as it had already been run with a different hash. Must have missed this in the documentation a few years back when we started with RoundHouse. I will see if this is of any use since all of our scripts in the up folder are written defensively to check what they are changing prior to changing it. We have separate scripts that are run to transfer a DB between environments and this picks up any use of the the Environment token replacement based on our internal conventions.