fix for getSchema when using "-" in name #718
Conversation
|
Apologies for not opening the PR myself, was busy at work :) |
Codecov Report
@@ Coverage Diff @@
## dev #718 +/- ##
============================================
+ Coverage 48.29% 48.33% +0.04%
- Complexity 2614 2622 +8
============================================
Files 113 113
Lines 26625 26626 +1
Branches 4480 4480
============================================
+ Hits 12858 12871 +13
+ Misses 11613 11598 -15
- Partials 2154 2157 +3
Continue to review full report at Codecov.
|
| assertEquals(catalogName, catalogMsgArgs[0]); | ||
| } | ||
|
|
||
| final MessageFormat atLeastOneFoundFormat = new MessageFormat(TestResource.getResource("R_atLeastOneFound")); |
There was a problem hiding this comment.
Everything else looks okay, there's just too many final objects here in both new methods, which don't really have a purpose. Let's keep it consistent with other tests.
There was a problem hiding this comment.
Removed all instances of final.
| assertTrue(hasResults, atLeastOneFoundFormat.format(schemaMsgArgs)); | ||
| } finally { | ||
| if (dropDatabase) { | ||
| connection.createStatement().execute(String.format("DROP DATABASE [%s]", testCatalog)); |
There was a problem hiding this comment.
You're using the generic connection and creating statement, which is not closed (same for other method), you could use Utils functions to drop database for consistency and that clear up objects as well.
| try (final Connection dashConn = DriverManager.getConnection(connectionString); | ||
| final Statement dashStatement = dashConn.createStatement()) { | ||
|
|
||
| connection.createStatement().execute(String.format("CREATE DATABASE [%s]", testCatalog)); |
There was a problem hiding this comment.
Unclosed Statement, either close this Statement, or use dashStatement instead.
There was a problem hiding this comment.
Created with try-with-resources, auto closed at the end of the blocks.
| final UUID id = UUID.randomUUID(); | ||
| final String testCatalog = "dash-catalog"+id; | ||
| final String testSchema = "some-schema"+id; | ||
| boolean dropDatabase = false; |
There was a problem hiding this comment.
This varible can be avoided if you'd use Utils drop functions, that check for existence of objects in sql.
There was a problem hiding this comment.
Utils is able to drop (if exists) stored procedures and tables. There doesn't seem to be one to handle databases.
Edit 1: I have gone ahead and removed the database creation tracking on the Java side, and replaced it with a 'DROP IF EXISTS' on the SQL side.
removed finals removed database creation tracking
| @Test | ||
| public void testDBSchemasForDashedCatalogName() throws SQLException { | ||
| UUID id = UUID.randomUUID(); | ||
| String testCatalog = "dash-catalog"+id; |
There was a problem hiding this comment.
Both testCatalog and testSchema should not be hard-coded. You can at least append a random number.
There was a problem hiding this comment.
Catalog name and Schema name are appended with an ID, and the ID is generated from our UUID class.
| try (Connection dashConn = DriverManager.getConnection(connectionString); | ||
| Statement dashStatement = dashConn.createStatement()) { | ||
|
|
||
| connection.createStatement().execute(String.format("CREATE DATABASE [%s]", testCatalog)); |
There was a problem hiding this comment.
You probably want to drop the database if it exists first. You can call Utils.dropDatabaseIfExists().
There was a problem hiding this comment.
The unique id makes this unlikely, and Utils does not have a dropDatabaseIfExists functionality (only stored procedure and table). However, I will adjust the code to check for this as it's a good habit.
There was a problem hiding this comment.
dropDatabaseIfExists is already on dev branch.
| MessageFormat dashCatalogFormat = new MessageFormat(TestResource.getResource("R_atLeastOneFound")); | ||
| assertTrue(hasDashCatalogSchema, dashCatalogFormat.format(new Object[] {testSchema})); | ||
| } finally { | ||
| connection.createStatement().execute("IF EXISTS (SELECT name FROM sys.databases WHERE name = N'" + testCatalog + "') " + |
There was a problem hiding this comment.
Replace this with the method from Utils class too.
Request Boundary methods - beginRequest()/endRequest() implementation…
| UUID id = UUID.randomUUID(); | ||
| String testCatalog = "dash-catalog" + id; | ||
| String testSchema = "some-schema" + id; | ||
| Statement stmt = connection.createStatement(); |
There was a problem hiding this comment.
You can move this into try with resources.
There was a problem hiding this comment.
Cannot resolve variables declared in try-with-resources in a finally block, and a finally block is used to drop the created db.
avoid manually closing statements, and safetly handles resources.
Fix for issue #715 by @fabiocorneti