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.

Sign up for GitHub

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

fix for getSchema when using "-" in name #718

Merged
merged 6 commits into from
Jun 20, 2018
Merged

fix for getSchema when using "-" in name #718

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a8dcc9e
fix for getSchema when using "-" in name
rene-ye Jun 6, 2018
b713507
test changes
rene-ye Jun 18, 2018
876a375
drop database before creating
rene-ye Jun 18, 2018
a968dd9
Merge pull request #44 from Microsoft/dev
rene-ye Jun 18, 2018
4c85049
replaced dropDBIfExists with Utils function
rene-ye Jun 18, 2018
fc78177
added try-with-resources nest
rene-ye Jun 18, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions src/main/java/com/microsoft/sqlserver/jdbc/SQLServerDatabaseMetaData.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1177,8 +1177,9 @@ private java.sql.ResultSet getSchemasInternal(String catalog,
String schema = "sys.schemas";
String schemaName = "sys.schemas.name";
if (null != catalog && catalog.length() != 0) {
schema = catalog + "." + schema;
schemaName = catalog + "." + schemaName;
final String catalogId = Util.escapeSQLId(catalog);
schema = catalogId + "." + schema;
schemaName = catalogId + "." + schemaName;
}

// The common schemas need to be under null catalog name however the schemas specific to the particular catalog has to have the current
Expand Down
104 changes: 104 additions & 0 deletions src/test/java/com/microsoft/sqlserver/jdbc/databasemetadata/DatabaseMetaDataTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,10 @@
*/
package com.microsoft.sqlserver.jdbc.databasemetadata;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertNull;
import static org.junit.jupiter.api.Assertions.assertSame;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.junit.jupiter.api.Assumptions.assumeTrue;
Expand All @@ -23,6 +25,8 @@
import java.sql.DriverManager;
import java.sql.ResultSet;
import java.sql.SQLException;
import java.sql.Statement;
import java.util.UUID;
import java.util.jar.Attributes;
import java.util.jar.Manifest;
import java.text.MessageFormat;
Expand Down Expand Up @@ -175,6 +179,106 @@ public void testDBSchema() throws SQLException {
assertTrue(!StringUtils.isEmpty(rs.getString(1)), form.format(msgArgs));
}
}

/**
* Tests that the catalog parameter containing - is escaped by
* {@link SQLServerDatabaseMetaData#getSchemas(String catalog, String schemaPattern)}.
* @throws SQLException
*/
@Test
public void testDBSchemasForDashedCatalogName() throws SQLException {
final UUID id = UUID.randomUUID();
final String testCatalog = "dash-catalog"+id;
final String testSchema = "some-schema"+id;
boolean dropDatabase = false;

try (final Connection dashConn = DriverManager.getConnection(connectionString);
final Statement dashStatement = dashConn.createStatement()) {

connection.createStatement().execute(String.format("CREATE DATABASE [%s]", testCatalog));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You probably want to drop the database if it exists first. You can call Utils.dropDatabaseIfExists().

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dropDatabaseIfExists is already on dev branch.

dropDatabase = true;
dashStatement.execute(String.format("USE [%s]", testCatalog));
dashStatement.execute(String.format("CREATE SCHEMA [%s]", testSchema));

final DatabaseMetaData databaseMetaData = connection.getMetaData();
final ResultSet rs = databaseMetaData.getSchemas(testCatalog, null);

final MessageFormat schemaEmptyFormat = new MessageFormat(TestResource.getResource("R_nameEmpty"));
final Object[] schemaMsgArgs = {"Schema"};

boolean hasResults = false;
boolean hasDashCatalogSchema = false;
while (rs.next()) {
hasResults = true;
final String schemaName = rs.getString(1);
assertTrue(!StringUtils.isEmpty(schemaName), schemaEmptyFormat.format(schemaMsgArgs));
final String catalogName = rs.getString(2);
if (schemaName.equals(testSchema)) {
hasDashCatalogSchema = true;
assertEquals(catalogName, testCatalog);
} else {
assertNull(catalogName);
}
}

final MessageFormat atLeastOneFoundFormat = new MessageFormat(TestResource.getResource("R_atLeastOneFound"));
assertTrue(hasResults, atLeastOneFoundFormat.format(schemaMsgArgs));

final MessageFormat dashCatalogFormat = new MessageFormat(TestResource.getResource("R_atLeastOneFound"));
assertTrue(hasDashCatalogSchema, dashCatalogFormat.format(new Object[] {testSchema}));
} finally {
if (dropDatabase) {
connection.createStatement().execute(String.format("DROP DATABASE [%s]", testCatalog));
}
}
}

/**
* Tests that the catalog parameter containing - is escaped by
* {@link SQLServerDatabaseMetaData#getSchemas(String catalog, String schemaPattern)}.
* @throws SQLException
*/
@Test
public void testDBSchemasForDashedCatalogNameWithPattern() throws SQLException {
final UUID id = UUID.randomUUID();
final String testCatalog = "dash-catalog"+id;
final String testSchema = "some-schema"+id;
boolean dropDatabase = false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This varible can be avoided if you'd use Utils drop functions, that check for existence of objects in sql.

Copy link
Member Author

@rene-ye rene-ye Jun 18, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


try (final Connection dashConn = DriverManager.getConnection(connectionString);
final Statement dashStatement = dashConn.createStatement()) {

connection.createStatement().execute(String.format("CREATE DATABASE [%s]", testCatalog));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unclosed Statement, either close this Statement, or use dashStatement instead.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created with try-with-resources, auto closed at the end of the blocks.

dropDatabase = true;
dashStatement.execute(String.format("USE [%s]", testCatalog));
dashStatement.execute(String.format("CREATE SCHEMA [%s]", testSchema));

final DatabaseMetaData databaseMetaData = connection.getMetaData();
final ResultSet rs = databaseMetaData.getSchemas(testCatalog, "some-%");

final MessageFormat schemaEmptyFormat = new MessageFormat(TestResource.getResource("R_nameEmpty"));
final Object[] schemaMsgArgs = {testSchema};
final Object[] catalogMsgArgs = {testCatalog};

boolean hasResults = false;
while (rs.next()) {
hasResults = true;
final String schemaName = rs.getString(1);
final String catalogName = rs.getString(2);
assertTrue(!StringUtils.isEmpty(schemaName), schemaEmptyFormat.format(schemaMsgArgs));
assertTrue(!StringUtils.isEmpty(catalogName), schemaEmptyFormat.format(catalogMsgArgs));
assertEquals(schemaName, schemaMsgArgs[0]);
assertEquals(catalogName, catalogMsgArgs[0]);
}

final MessageFormat atLeastOneFoundFormat = new MessageFormat(TestResource.getResource("R_atLeastOneFound"));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed all instances of final.

assertTrue(hasResults, atLeastOneFoundFormat.format(schemaMsgArgs));
} finally {
if (dropDatabase) {
connection.createStatement().execute(String.format("DROP DATABASE [%s]", testCatalog));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}
}

/**
* Get All Tables.
Expand Down