MS SQL Server: handle columns with spaces

[mitchelldavis]

Added an override of the buildInsertCommand method on the
SqlServerEnvironment object. The method simply surrounds the column
names with brackets.

For Example:
INSERT INTO ([COLUMN1],[COLUMN TWO],[COLUMN3])...
instead of:
INSERT INTO (COLUMN1,COLUMN TWO, COLUMN3)... which causes an error.

[benilovj]

Hi @mitchelldavis, thanks for the patch!

To stop this case from regressing in the future:

• would you be able to add an acceptance test for this case, which fails before your proposed change and passes with it
• the new method should be nicely unit-testable, it would be very good to add that as well

[benilovj]

if you could get rid of these commented out code lines, that'd be great - no point in carrying around unused code.

[mitchelldavis]

I'm on it.

[mitchelldavis]

I've added some tests for the SqlEnvironment.buildInsertCommand() method. Also, added an AcceptanceTest in the FitNesseRoot called BulkInsertWithWeirdColumnNames

@javornikolov: I didn't implement your suggestion on pulling out the column name wrapping simply because JAVA is not my native language and I was stubbing my toes a lot to get what I've done in.

Any suggestions on how to quickly implement what your suggesting would be appreciated.

[mitchelldavis]

Darn, I hit the wrong button.... Sorry. :-)

[javornikolov]

Thanks @mitchelldavis! We'll review.

Another request/question - have you managed to run the tests locally (the new ones and the whole suite for MS SQL Server)? We're lacking MS SQL Server environments so it's a bit hard to test it properly but if you can give some feedback, that would be great.

[mitchelldavis]

I was able to run the integration tests locally: .\gradlew :dbfit-java:mysql:test (Notice: I'm using windows command line.) Which included the unit tests I wrote.

However, the acceptance tests I had to do a little rigging for and on that note, I only ran the one I wrote. Below is the actual test I ran.

!path ..\Dependencies\sqljdbc_4.0\enu\sqljdbc4.jar #Needed for SQLServer connectivity
!| dbfit.SqlServerTest|

|Connect|jdbc:sqlserver://localhost;databaseName=<someDatabase>;integratedSecurity=true|
|Execute|Create table Test_DBFit(name varchar(50), [lucky Number] int)|

|Insert|Test_DBFit|
|name|luckyNumber|
|pera|1|
|nuja|2|
|nnn|3|

|Query|Select * from Test_DBFit|
|name|lucky Number|
|pera|1|
|nuja|2|
|nnn|3|


|Execute|Drop table Test_DBFit|

I wasn't sure how to get all of the acceptance tests running on my windows machine. .\gradlew start was giving me fits.

C:\Git\dbfit\dist>cd /d C:\Git\dbfit\dist\

C:\Git\dbfit\dist>java -cp 'lib\dbfit-docs-2.2.0.jar:lib\fitnesse-standalone-20140122.jar' fitnesseMain.FitNesseMain
Error: Could not find or load main class fitnesseMain.FitNesseMain

C:\Git\dbfit\dist>pause
Press any key to continue . . .

BUILD SUCCESSFUL

So, other than connectivity (which it seemed is being handled outside of most of your acceptance tests) the test is self containing, so I made sure that I was able to successfully get the above test to pass and fail, then committed it minus the first three lines. The environment I used to get it to pass and fail was an in house one referencing the updated *.jar files.

In house, we use a combination of MSBuild, http://www.liquibase.org/, and DbFit for our database projects, With some neat build scripts it's working pretty well for us.

What specifically are you looking for feedback on? DbFit seems to be working great with SQLServer so far (other than the issue this pull request is for. ;-)). I'll try porting over all the tests into our environment, but maybe you could give me a hint on why .\gradlew start is giving me fits?

[javornikolov]

java -cp 'lib\dbfit-docs-2.2.0.jar:lib\fitnesse-standalone-20140122.jar' fitnesseMain.FitNesseMain
Error: Could not find or load main class fitnesseMain.FitNesseMain

For this one - seems we have a bug: path separator should be ; instead of :.

[mitchelldavis]

That last pull didn't really fix the issue. I can run java -cp 'lib\dbfit-docs-2.2.0.jar:lib\fitnesse-standalone-20140122.jar' fitnesseMain.FitNesseMain manually from the dist folder, but when I run .\gradlew start it still fails with the same error.

[mitchelldavis]

FYI, I'm finding issues with my acceptance test. I'll let you know when I get them resolved.

[javornikolov]

That last pull didn't really fix the issue. I can run java -cp 'lib\dbfit-docs-2.2.0.jar:lib\fitnesse-standalone-20140122.jar' fitnesseMain.FitNesseMain manually from the dist folder, but when I run .\gradlew start it still fails with the same error.

I fixed that in #236. Thanks a lot for catching the issues with startFitnesse.bat :-)

What specifically are you looking for feedback on? DbFit seems to be working great with SQLServer so far (other than the issue this pull request is for. ;-)).

I'm glad to hear DbFit works well for you :-) Since we don't build and test DbFit on MS SQL Server (nor on Windows) there is higher risk for glitches there. So I wondered: are the acceptance tests still passing OK for SQL Server.

[mitchelldavis]

Thanks for those two pulls, I was able to finally get .\gradlew start to work.

Here's the status of the...DotNetTests.SqlServerTests: (I don't have a copy of SQLServer 2000 or DB2)

• 17 right
• 3 wrong
• 2 ignored
• 19 exceptions

However, they're actually all failing. I noticed within the gradle build...

task bundleFitsharp(dependsOn: copyLocal) << {
    mkdir 'dist/fitsharp/'
    ant.get(src: 'http://cloud.github.com/downloads/jediwhale/fitsharp/release.2.2.net.40.zip',
            dest: 'dist/fitsharp/',
            verbose: true)
    copy {
        from zipTree('dist/fitsharp/release.2.2.net.40.zip')
        into 'dist/fitsharp/'
    }
    delete 'dist/fitsharp/release.2.2.net.40.zip'
    file('dist/fitsharp/Runner.exe.config') << '<configuration><runtime><loadFromRemoteSources enabled="true"/></runtime></configuration>'
}

... that the .\dist\fitsharp directory is not populated with current .net builds of the code on .\gradlew start. Meaning that none of the tests are actually testing current code (I.E. any of the changes I was making).

I made these changes in .\FitnesseRoot\DbFit\AcceptanceTests\DotNetTests\SqlServerTests\FlowMode\SetUp\content.txt:

!|dbfit.SqlServerTest|

#!|Connect|Server=localhost;Database=dbfit_Test;Trusted_Connection=True;|
!| Connect | jdbc:sqlserver://localhost;databaseName=dbfit_test;integratedSecurity=true |

and .\FitnessRoot\DbFit\AcceptanceTests\DotNetTests\content.txt:

#!define COMMAND_PATTERN {%m -r fitnesse.fitserver.FitServer %p}
#!define TEST_RUNNER {fitsharp\Runner.exe}
#!define PATH_SEPARATOR {;}
#!path fitsharp\*.dll
!path lib\*.jar
!path C:\...\sqljdbc4.jar #This is needed for SqlServer connectivity.

!contents

In order to get the tests to run with the default java runner and against the current code base. I also, removed the changes that I originally submitted for the Pull Request and re-ran the tests and got the following results:

• 12 right
• 3 wrong (one of them being the acceptance test for the code that was removed.)
• 1 ignored
• 25 exceptions

So, long story short: I can prove that my acceptance test passes, but I cannot prove that it doesn't cause any regressions in other code.

So, where do we go from here? I would really like this pull to get through because we would like to not have to rely on a local build of dbfit for our build server. However, I understand that we could be introducing some bad stuff.

[mitchelldavis]

Crap, saw an error... Here are the actual test numbers that should replace that second set of test numbers above:

• 16 right
• 4 wrong (one of them being the acceptance test for the code that was removed.)
• 2 ignored
• 19 exceptions

Which matches the original set of test numbers. This quasi-proves that the pull's changes won't cause any issues.

[benilovj]

@mitchelldavis thanks for investigating.

The reason why you had to hack .\FitnessRoot\DbFit\AcceptanceTests\DotNetTests\content.txt in the way you did is because that test suite is actually designed to test the .NET version of DbFit. The equivalent java version (that you've proposed changes to) is here.

However the versions share tests, so you ended up running the correct test suite in the end.

Now, the final tweak that I think is needed is to add a symbolic link to the new test into the SQLServer java testsuite - it's possible to either do that by hand, or through the properties of the DbFit.AcceptanceTests.JavaTests.SqlServerTests.FlowMode test suite.

Apart from that, obviously it's not ideal that the test suite doesn't pass entirely, but that's not due to this change, so I'm inclined to merge it after the above change has been made. @javornikolov what do you think?

[javornikolov]

Yes, I think the change is OK to be merged 👍

It seems to be a different topic what are these exceptions raising failures/exceptions: doesn't seem related to this change.

[javornikolov]

One question came to my mind: @mitchelldavis, could you confirm that enclosing a column name into '[]' doesn't make it case sensitive?

[benilovj]

One question came to my mind: @mitchelldavis, could you confirm that enclosing a column name into '[]' doesn't make it case sensitive?

this comment on stackoverflow seems to suggest that the answer is "not necessarily"

[javornikolov]

OK. As far as I understand, case sensitivity is determined by db configuration and is not dependent on surrounding names in square brackets. Good, so this change is not supposed to break anything. (My concern was that it may behave double quotes in Oracle where "MyColumn" would make name case sensitive.)

[mitchelldavis]

Thanks for all your patience and help with this @benilovj and @javornikolov. Let me know if I can be of any help in the future.

[benilovj]

@mitchelldavis thank you for your contribution!

I hope to push out a release including this change soon (within a week)

[benilovj]

@mitchelldavis I'd have normally sent you a link to a build that included your change, but unfortunately it seems that the package publishing functionality of our CI build is broken at the moment.

[mitchelldavis]

Sounds great. Thank you @benilovj.

[javornikolov]

I see that with a bit of delay but I think here it should be: lucky Number instead of luckyNumber.

The reason I got into that again - I was thinking about a similar issue in Oracle and I started wondering about an alternative solution - explicitly enclosing the column names in test itself.

|Insert|Test_DBFit|
|name|[lucky Number]|
...

@mitchelldavis, have you tried it that way?

[mitchelldavis]

@javornikolov see #238