-
Notifications
You must be signed in to change notification settings - Fork 618
[jdbc] Adds CREATE USER statement to parser grammar
#2421
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.
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
Changes from all commits
942a2c8
af9e682
0b55563
a532b23
d5e9c9d
0e1310d
5ca334b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -129,7 +129,7 @@ protected ResultSetImpl executeQueryImpl(String sql, QuerySettings settings) thr | |
|
|
||
| try { | ||
| lastStatementSql = parseJdbcEscapeSyntax(sql); | ||
| LOG.debug("SQL Query: {}", lastStatementSql); | ||
| LOG.trace("SQL Query: {}", lastStatementSql); // this is not secure for create statements because of passwords | ||
chernser marked this conversation as resolved.
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Logging full SQL queries, even at TRACE level, may expose sensitive information such as passwords in CREATE USER statements. Consider redacting sensitive data from the SQL before logging, or ensure users are aware of this risk in documentation or configuration. |
||
| QueryResponse response; | ||
| if (queryTimeout == 0) { | ||
| response = connection.client.query(lastStatementSql, mergedSettings).get(); | ||
|
|
@@ -179,7 +179,7 @@ protected int executeUpdateImpl(String sql, QuerySettings settings) throws SQLEx | |
| } | ||
|
|
||
| lastStatementSql = parseJdbcEscapeSyntax(sql); | ||
| LOG.debug("SQL Query: {}", lastStatementSql); | ||
| LOG.trace("SQL Query: {}", lastStatementSql); | ||
| int updateCount = 0; | ||
| try (QueryResponse response = queryTimeout == 0 ? connection.client.query(lastStatementSql, mergedSettings).get() | ||
| : connection.client.query(lastStatementSql, mergedSettings).get(queryTimeout, TimeUnit.SECONDS)) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -103,4 +103,6 @@ public void enterSetRoleStmt(ClickHouseParser.SetRoleStmtContext ctx) { | |
| setRoles(roles); | ||
| } | ||
| } | ||
|
|
||
|
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -342,7 +342,7 @@ public void testExecuteQueryTimeout() throws Exception { | |
|
|
||
|
|
||
| @Test(groups = { "integration" }) | ||
| private void testSettingRole() throws SQLException { | ||
| public void testSettingRole() throws SQLException { | ||
| if (earlierThan(24, 4)) {//Min version is 24.4 | ||
| return; | ||
| } | ||
|
|
@@ -551,8 +551,8 @@ public void testSwitchDatabase() throws Exception { | |
| } | ||
| } | ||
| } | ||
|
|
||
|
|
||
| @Test(groups = { "integration" }) | ||
| public void testNewLineSQLParsing() throws Exception { | ||
| try (Connection conn = getJdbcConnection()) { | ||
|
|
@@ -617,7 +617,7 @@ public void testNewLineSQLParsing() throws Exception { | |
| } | ||
| } | ||
|
|
||
|
|
||
| @Test(groups = { "integration" }) | ||
| public void testNullableFixedStringType() throws Exception { | ||
| try (Connection conn = getJdbcConnection()) { | ||
|
|
@@ -708,4 +708,25 @@ public void testExecuteWithMaxRows() throws Exception { | |
| } | ||
| } | ||
|
|
||
| @Test(groups = {"integration"}) | ||
| public void testDDLStatements() throws Exception { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we can expand testing with a few more examples (to see that the parser works correctly)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, will do this. |
||
| if (isCloud()) { | ||
| return; // skip because we do not want to create extra on cloud instance | ||
| } | ||
| try (Connection conn = getJdbcConnection()) { | ||
| try (Statement stmt = conn.createStatement()){ | ||
| Assert.assertFalse(stmt.execute("CREATE USER IF NOT EXISTS 'user011' IDENTIFIED BY 'password'")); | ||
|
|
||
| try (ResultSet rs = stmt.executeQuery("SHOW USERS")) { | ||
| boolean found = false; | ||
| while (rs.next()) { | ||
| if (rs.getString("name").equals("user011")) { | ||
| found = true; | ||
| } | ||
| } | ||
| Assert.assertTrue(found); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests are required for the new CREATE USER statement grammar. Please add unit and/or integration tests to ensure the parser correctly handles valid and invalid CREATE USER statements.