-
Notifications
You must be signed in to change notification settings - Fork 332
Add options to the bootstrap command to specify a schema file #1942
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
4e1766b
2fe7cdc
3be2225
0a8c6d0
9969903
6e9546b
db19939
6fee80f
183cfbd
1b53b8e
6a929e2
06e2a51
b80f6ae
e99d70e
49cce5b
e4a2f2a
3559234
28617e0
b7e2e5a
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 |
|---|---|---|
|
|
@@ -24,6 +24,7 @@ | |
| import jakarta.annotation.Nonnull; | ||
| import java.io.BufferedReader; | ||
| import java.io.IOException; | ||
| import java.io.InputStream; | ||
| import java.io.InputStreamReader; | ||
| import java.sql.Connection; | ||
| import java.sql.PreparedStatement; | ||
|
|
@@ -75,46 +76,51 @@ DatabaseType getDatabaseType() { | |
| } | ||
|
|
||
| /** | ||
| * Execute SQL script. | ||
| * Execute SQL script and close the associated input stream | ||
| * | ||
| * @param scriptFilePath : Path of SQL script. | ||
| * @param scriptInputStream : Input stream containing the SQL script. | ||
| * @throws SQLException : Exception while executing the script. | ||
| */ | ||
| public void executeScript(String scriptFilePath) throws SQLException { | ||
| ClassLoader classLoader = DatasourceOperations.class.getClassLoader(); | ||
| runWithinTransaction( | ||
| connection -> { | ||
| try (Statement statement = connection.createStatement()) { | ||
| BufferedReader reader = | ||
| new BufferedReader( | ||
| new InputStreamReader( | ||
| Objects.requireNonNull(classLoader.getResourceAsStream(scriptFilePath)), | ||
| UTF_8)); | ||
| StringBuilder sqlBuffer = new StringBuilder(); | ||
| String line; | ||
| while ((line = reader.readLine()) != null) { | ||
| line = line.trim(); | ||
| if (!line.isEmpty() && !line.startsWith("--")) { // Ignore empty lines and comments | ||
| sqlBuffer.append(line).append("\n"); | ||
| if (line.endsWith(";")) { // Execute statement when semicolon is found | ||
| String sql = sqlBuffer.toString().trim(); | ||
| try { | ||
| // since SQL is directly read from the file, there is close to 0 possibility | ||
| // of this being injected plus this run via an Admin tool, if attacker can | ||
| // fiddle with this that means lot of other things are already compromised. | ||
| statement.execute(sql); | ||
| } catch (SQLException e) { | ||
| throw new RuntimeException(e); | ||
| public void executeScript(InputStream scriptInputStream) throws SQLException { | ||
| try { | ||
| runWithinTransaction( | ||
| connection -> { | ||
| try (Statement statement = connection.createStatement(); | ||
| BufferedReader reader = | ||
| new BufferedReader( | ||
| new InputStreamReader(Objects.requireNonNull(scriptInputStream), UTF_8))) { | ||
| StringBuilder sqlBuffer = new StringBuilder(); | ||
| String line; | ||
| while ((line = reader.readLine()) != null) { | ||
| line = line.trim(); | ||
| if (!line.isEmpty() && !line.startsWith("--")) { // Ignore empty lines and comments | ||
| sqlBuffer.append(line).append("\n"); | ||
| if (line.endsWith(";")) { // Execute statement when semicolon is found | ||
| String sql = sqlBuffer.toString().trim(); | ||
| try { | ||
| // since SQL is directly read from the file, there is close to 0 possibility | ||
| // of this being injected plus this run via an Admin tool, if attacker can | ||
| // fiddle with this that means lot of other things are already compromised. | ||
| statement.execute(sql); | ||
| } catch (SQLException e) { | ||
| throw new RuntimeException(e); | ||
| } | ||
| sqlBuffer.setLength(0); // Clear the buffer for the next statement | ||
| } | ||
| sqlBuffer.setLength(0); // Clear the buffer for the next statement | ||
| } | ||
| } | ||
| return true; | ||
| } catch (IOException e) { | ||
| throw new RuntimeException(e); | ||
| } | ||
| return true; | ||
| } catch (IOException e) { | ||
| throw new RuntimeException(e); | ||
| } | ||
| }); | ||
| }); | ||
| } finally { | ||
| try { | ||
| scriptInputStream.close(); | ||
|
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'm pretty sure the try on line 88 closes this stream too... Could you double check?
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. Currently no, the only resource is the Statement: But if we added the BufferedReader there too, it might close the stream. Do you want to do that instead?
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. Yeah, if possible... I think try-with-resources is more intuitive and we get better exception propagation OOTB.
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. Do we still need this call to |
||
| } catch (IOException e) { | ||
| LOGGER.error("Failed to close input stream: {}", e.getMessage()); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /** | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,6 +20,7 @@ | |
|
|
||
| import static org.apache.polaris.core.persistence.PrincipalSecretsGenerator.RANDOM_SECRETS; | ||
|
|
||
| import java.io.InputStream; | ||
| import java.sql.SQLException; | ||
| import java.time.ZoneId; | ||
| import java.util.Optional; | ||
|
|
@@ -49,8 +50,11 @@ protected PolarisTestMetaStoreManager createPolarisTestMetaStoreManager() { | |
| try { | ||
| datasourceOperations = | ||
| new DatasourceOperations(createH2DataSource(), new H2JdbcConfiguration()); | ||
| datasourceOperations.executeScript( | ||
| String.format("%s/schema-v2.sql", DatabaseType.H2.getDisplayName())); | ||
| ClassLoader classLoader = DatasourceOperations.class.getClassLoader(); | ||
| InputStream scriptStream = | ||
| classLoader.getResourceAsStream( | ||
|
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. nit: this stream normally needs to be closed, AFAIK.
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. ditto the above -- I think we just weren't closing it before, but added an explicit
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. Why not a try-with-resources block?
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. Per the javadoc, I think it's better to have this method take on the responsibility for closing the stream after it's used rather than scatter try-with-resources amongst the callers. The loan pattern would be better, but we're already getting pretty far away from the intent of this change. |
||
| String.format("%s/schema-v2.sql", DatabaseType.H2.getDisplayName())); | ||
| datasourceOperations.executeScript(scriptStream); | ||
| } catch (SQLException e) { | ||
| throw new RuntimeException( | ||
| String.format( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one | ||
| * or more contributor license agreements. See the NOTICE file | ||
| * distributed with this work for additional information | ||
| * regarding copyright ownership. The ASF licenses this file | ||
| * to you under the Apache License, Version 2.0 (the | ||
| * "License"); you may not use this file except in compliance | ||
| * with the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, | ||
| * software distributed under the License is distributed on an | ||
| * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
| * KIND, either express or implied. See the License for the | ||
| * specific language governing permissions and limitations | ||
| * under the License. | ||
| */ | ||
|
|
||
| package org.apache.polaris.core.persistence.bootstrap; | ||
|
|
||
| import org.apache.polaris.immutables.PolarisImmutable; | ||
|
|
||
| @PolarisImmutable | ||
| public interface BootstrapOptions { | ||
| Iterable<String> realms(); | ||
|
|
||
| RootCredentialsSet rootCredentialsSet(); | ||
|
|
||
| SchemaOptions schemaOptions(); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one | ||
| * or more contributor license agreements. See the NOTICE file | ||
| * distributed with this work for additional information | ||
| * regarding copyright ownership. The ASF licenses this file | ||
| * to you under the Apache License, Version 2.0 (the | ||
| * "License"); you may not use this file except in compliance | ||
| * with the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, | ||
| * software distributed under the License is distributed on an | ||
| * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
| * KIND, either express or implied. See the License for the | ||
| * specific language governing permissions and limitations | ||
| * under the License. | ||
| */ | ||
|
|
||
| package org.apache.polaris.core.persistence.bootstrap; | ||
|
|
||
| import jakarta.annotation.Nullable; | ||
| import org.apache.polaris.immutables.PolarisImmutable; | ||
| import org.immutables.value.Value; | ||
|
|
||
| @PolarisImmutable | ||
| public interface SchemaOptions { | ||
| @Nullable | ||
| Integer schemaVersion(); | ||
|
|
||
| @Nullable | ||
| String schemaFile(); | ||
|
|
||
| @Value.Check | ||
| default void validate() { | ||
| if (schemaVersion() != null && schemaFile() != null) { | ||
| throw new IllegalStateException("Only one of schemaVersion or schemaFile can be set."); | ||
| } | ||
| } | ||
| } |
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.
WDYT about adding
@WillCloseto the stream parameter?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.
That sounds like a good idea... is this the one from jsr305? It doesn't seem that we have that dependency currently, and we're not using it anywhere else in the project.
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.
yes, that jar 👍 TBH, I was surprised we did not have any existing use cases 😅
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.
I noticed this is in the
banned-dependencies.txt?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.
TIL... well, I'm fine with not using it in this case.
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.
Me too! I learned this when I went to add the dep 😅
If there is another similar annotation we start using in the future that would be nice as this does sound pretty helpful.