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.

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

configure dedicated db user for database migrations: DML-only user fo… #29649

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
package io.quarkus.flyway.test;

import static org.junit.jupiter.api.Assertions.assertEquals;

import java.sql.Connection;
import java.sql.PreparedStatement;
import java.sql.ResultSet;
import java.sql.SQLException;

import javax.inject.Inject;
import javax.sql.DataSource;

import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.RegisterExtension;

import io.quarkus.test.QuarkusUnitTest;

public class FlywayExtensionDifferentUsernameTest {

@Inject
DataSource datasource;

@RegisterExtension
static final QuarkusUnitTest config = new QuarkusUnitTest()
.withApplicationRoot((jar) -> jar

.addAsResource("config-for-different-username.properties", "application.properties"));

@Test
@DisplayName("Check if connected with new username")
public void testFlywayInitSql() throws SQLException {

int var = 0;
try (Connection con = datasource.getConnection();
PreparedStatement ps = con.prepareStatement("SELECT 100");
ResultSet rs = ps.executeQuery()) {
if (rs.next()) {
var = rs.getInt(1);
}
}
assertEquals(100, var, "Different username and password was not executed");
Copy link
Contributor

Choose a reason for hiding this comment

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

This test doesn't really verify the feature you provided

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test if the connection is working with new configs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will change the message for "Database connected with new config", its ok?

Copy link
Member

Choose a reason for hiding this comment

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

I think you should rather get the datasource and check the user there. In your previous PR, I outlined how you could get the datasource URL. I'm pretty sure you can get the user from there too.


}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
# Flyway config properties
quarkus.flyway.username=sa
quarkus.flyway.password=sa

Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
package io.quarkus.flyway.runtime;

import java.io.PrintWriter;
import java.sql.Connection;
import java.sql.SQLException;
import java.sql.SQLFeatureNotSupportedException;
import java.util.Optional;
import java.util.logging.Logger;

import javax.sql.DataSource;

public class DataSourceDecorator implements DataSource {
Copy link
Contributor

Choose a reason for hiding this comment

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

The name doesn't really explain what this class does, but I am not very good at naming, so I'll let others decide about it


private DataSource dataSource;
private Optional<String> username;
private Optional<String> password;

public DataSourceDecorator(DataSource datasource, Optional<String> username, Optional<String> password) {

this.dataSource = datasource;
this.username = username;
this.password = password;

}

public PrintWriter getLogWriter() throws SQLException {
// TODO Auto-generated method stub
return dataSource.getLogWriter();
}

public int getLoginTimeout() throws SQLException {
// TODO Auto-generated method stub
return dataSource.getLoginTimeout();
}

public Logger getParentLogger() throws SQLFeatureNotSupportedException {
// TODO Auto-generated method stub
return dataSource.getParentLogger();
}

public void setLogWriter(PrintWriter arg0) throws SQLException {
// TODO Auto-generated method stub
dataSource.setLogWriter(arg0);
}

public void setLoginTimeout(int arg0) throws SQLException {
// TODO Auto-generated method stub
dataSource.setLoginTimeout(arg0);
}

public boolean isWrapperFor(Class<?> arg0) throws SQLException {
// TODO Auto-generated method stub
return dataSource.isWrapperFor(arg0);
}

public <T> T unwrap(Class<T> arg0) throws SQLException {
Copy link
Contributor

Choose a reason for hiding this comment

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

Replace arg0 parameters with the name used in the sources

// TODO Auto-generated method stub
return dataSource.unwrap(arg0);
}

public Connection getConnection() throws SQLException {
// TODO Auto-generated method stub
return dataSource.getConnection(username.get(), password.get());
}

public Connection getConnection(String username, String password)
throws SQLException {
// TODO Auto-generated method stub
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove these comments

return dataSource.getConnection(username, password);
}

}
8 changes: 7 additions & 1 deletion extensions/flyway/runtime/src/main/java/io/quarkus/flyway/runtime/FlywayCreator.java
100644 → 100755
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,13 @@ public FlywayCreator withCallbacks(Collection<Callback> callbacks) {

public Flyway createFlyway(DataSource dataSource) {
FluentConfiguration configure = Flyway.configure();
configure.dataSource(dataSource);

if (flywayRuntimeConfig.username.isPresent() && flywayRuntimeConfig.password.isPresent()) {
configure.dataSource(
new DataSourceDecorator(dataSource, flywayRuntimeConfig.username, flywayRuntimeConfig.password));
Copy link
Contributor

Choose a reason for hiding this comment

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

You can invoke get() here and avoid using Optional in the class constructor

Copy link
Member

Choose a reason for hiding this comment

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

I don't undertstand the usage of this pattern. I gave you good hints on how you can deal with this in your previous PR with some pseudo code that should allow you to do what you want and some advice on how to deal with the JDBC URL.
Not sure why you went another way? There might be a very good reason so don't hesitate to speak up.

Copy link
Contributor

Choose a reason for hiding this comment

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

@gsmet Before I read your comments on the previous PR, I suggested @mfpc to use this pattern. The idea was to reuse the DataSource configuration and call DataSource#getConnection(String user, String password) when DataSource#getConnection() is called in the Flyway codebase.

But now I see that your suggestion seems nicer, so 👍🏻 to that

} else {
configure.dataSource(dataSource);
}
if (flywayRuntimeConfig.initSql.isPresent()) {
configure.initSql(flywayRuntimeConfig.initSql.get());
}
Expand Down
20 changes: 20 additions & 0 deletions ...flyway/runtime/src/main/java/io/quarkus/flyway/runtime/FlywayDataSourceRuntimeConfig.java
100644 → 100755
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,26 @@ public static FlywayDataSourceRuntimeConfig defaultConfig() {
@ConfigItem
public Optional<String> defaultSchema = Optional.empty();

/**
* This is a safety measurement against theoretical situations where the application goes haywire for any reason.
* This includes e.g. accidentally setting quarkus.hibernate-orm.database.generation to drop-and-create.
Copy link
Contributor

Choose a reason for hiding this comment

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

This description looks misleading and confusing

* If the application runs with a restricted database user, damage in such situations can be mitigated.
* # Name of a custom database user for flyway to use. Must have DDL rights
* quarkus.flyway.username=my_schema_owner
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be removed

*
*/

@ConfigItem
public Optional<String> username = Optional.empty();

/**
* Set the password for connection;
* quarkus.flyway.password=my_secret_password
Copy link
Contributor

Choose a reason for hiding this comment

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

This line can be removed

*/

@ConfigItem
public Optional<String> password = Optional.empty();

/**
* Comma-separated case-sensitive list of schemas managed by Flyway.
* The first schema in the list will be automatically set as the default one during the migration.
Expand Down