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

Add support for postgis container images to postgresql DevService #15919

Closed
matjazs opened this issue Mar 22, 2021 · 11 comments · Fixed by #15939 or #15999
Closed

Add support for postgis container images to postgresql DevService #15919

matjazs opened this issue Mar 22, 2021 · 11 comments · Fixed by #15939 or #15999
Labels
Milestone

Comments

@matjazs
Copy link

matjazs commented Mar 22, 2021

Description

DevServices support for PostgreSQL database does not work for postgis images. If you specify a custom image, e.g.:

%test.quarkus.datasource.devservices.image-name=postgis/postgis:latest

image name verification fails, since postgis is not recognised as a postgresql compatible substitute.

I have stumbled upon this issue since our previously working TestContainers configuration stopped working with Quarkus 1.12. Specifying db-kind other and TestContainers JDBC driver and URL somehow works for the default datasource but fails for the named datasource:

[error]: Build step io.quarkus.agroal.deployment.AgroalProcessor#build threw an exception: io.quarkus.runtime.configuration.ConfigurationException: Unable to find a JDBC driver corresponding to the database kind 'other' for the datasource 'qgis_import'. Either provide a suitable JDBC driver extension, define the driver manually, or disable the JDBC datasource by adding 'quarkus.datasource.qgis_import.jdbc=false' to your configuration if you don't need it.

Implementation ideas

I made a patch to allow for specifying postgis images with postgresql db-kind, .e.g.:

%test.quarkus.datasource.qgis_import.db-kind=postgresql
%test.quarkus.datasource.qgis_import.devservices.image-name=postgis/postgis:latest
Index: extensions/devservices/postgresql/src/main/java/io/quarkus/devservices/postgresql/deployment/PostgresqlDevServicesProcessor.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/extensions/devservices/postgresql/src/main/java/io/quarkus/devservices/postgresql/deployment/PostgresqlDevServicesProcessor.java b/extensions/devservices/postgresql/src/main/java/io/quarkus/devservices/postgresql/deployment/PostgresqlDevServicesProcessor.java
--- a/extensions/devservices/postgresql/src/main/java/io/quarkus/devservices/postgresql/deployment/PostgresqlDevServicesProcessor.java	(revision fd713d371b10320dc8a6f9cb85fb9846255d64ac)
+++ b/extensions/devservices/postgresql/src/main/java/io/quarkus/devservices/postgresql/deployment/PostgresqlDevServicesProcessor.java	(date 1616172989135)
@@ -6,6 +6,7 @@
 import java.util.Optional;
 
 import org.testcontainers.containers.PostgreSQLContainer;
+import org.testcontainers.utility.DockerImageName;
 
 import io.quarkus.datasource.common.runtime.DatabaseKind;
 import io.quarkus.datasource.deployment.spi.DevServicesDatasourceProvider;
@@ -21,7 +22,7 @@
             public RunningDevServicesDatasource startDatabase(Optional<String> username, Optional<String> password,
                     Optional<String> datasourceName, Optional<String> imageName, Map<String, String> additionalProperties) {
                 PostgreSQLContainer container = new PostgreSQLContainer(
-                        imageName.orElse(PostgreSQLContainer.IMAGE + ":" + PostgreSQLContainer.DEFAULT_TAG))
+                        getDockerImageName(imageName.orElse(PostgreSQLContainer.IMAGE + ":" + PostgreSQLContainer.DEFAULT_TAG)))
                                 .withPassword(password.orElse("quarkus"))
                                 .withUsername(username.orElse("quarkus"))
                                 .withDatabaseName(datasourceName.orElse("default"));
@@ -36,6 +37,15 @@
                             }
                         });
             }
+
+            private DockerImageName getDockerImageName(String fullImageName) {
+                DockerImageName dockerImageName = DockerImageName.parse(fullImageName);
+                // handle postgis as postgres compatible substitute
+                if (dockerImageName.getRepository().contains("postgis")) {
+                    dockerImageName = dockerImageName.asCompatibleSubstituteFor(PostgreSQLContainer.IMAGE);
+                }
+                return dockerImageName;
+            }
         });
     }
 
@matjazs matjazs added the kind/enhancement New feature or request label Mar 22, 2021
@gsmet
Copy link
Member

gsmet commented Mar 22, 2021

Hmmm, could you give me more details about this:

Specifying db-kind other and TestContainers JDBC driver and URL somehow works for the default datasource but fails for the named datasource

Especially, could you give me the configuration you're trying and that doesn't work?

@gsmet
Copy link
Member

gsmet commented Mar 22, 2021

@stuartwdouglas as for the compatible image issue, I wonder if we should trust the user and make all provided images compatible? Because I could see how people would try to use non-standard images.

@matjazs
Copy link
Author

matjazs commented Mar 22, 2021

Hmmm, could you give me more details about this:

Specifying db-kind other and TestContainers JDBC driver and URL somehow works for the default datasource but fails for the named datasource

Especially, could you give me the configuration you're trying and that doesn't work?

Related to the separate issue about failing named-datasource configuration:

The following actually works with 1.12.2, but fails with the latest main branch.

Create a starter project:

mvn io.quarkus:quarkus-maven-plugin:1.12.2.Final:create \
    -DprojectGroupId=org.acme \
    -DprojectArtifactId=getting-started \
    -DclassName="org.acme.getting.started.GreetingResource" \
    -Dpath="/hello"

Apply the below patch and run the tests.

The DataSourceTest will fail if the secondary datasource JDBC driver is not configured for the unspecified profile in application.properties and will pass if configured (it can be a bogus value), however it will never complain about the missing driver configuration for the default datasource.

Configuration from patch:

quarkus.datasource.db-kind=postgresql
quarkus.datasource.username=user1

quarkus.datasource.secondary.db-kind=postgresql
quarkus.datasource.secondary.username=user1
#quarkus.datasource.secondary.jdbc.driver=x

# test datasources

%test.quarkus.datasource.db-kind=other
%test.quarkus.datasource.jdbc.driver=org.testcontainers.jdbc.ContainerDatabaseDriver
%test.quarkus.datasource.jdbc.url=jdbc:tc:postgis:latest:///db1?currentSchema=public,schema1

%test.quarkus.datasource.secondary.db-kind=other
%test.quarkus.datasource.secondary.jdbc.driver=org.testcontainers.jdbc.ContainerDatabaseDriver
%test.quarkus.datasource.secondary.jdbc.url=jdbc:tc:postgis:latest:///db1

The reason we don't have the JDBC URLs specified in application.properties for profiles other then test is because we specify them either in developer's local config folder or by using environment variables in Kubernetes deployments.

The patch:

Index: pom.xml
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/pom.xml b/pom.xml
--- a/pom.xml	(revision e54195ed1a16d75554fc93aa608b22202c7271ea)
+++ b/pom.xml	(date 1616413534371)
@@ -12,10 +12,13 @@
     <maven.compiler.target>11</maven.compiler.target>
     <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
     <project.reporting.outputEncoding>UTF-8</project.reporting.outputEncoding>
-    <quarkus-plugin.version>1.12.2.Final</quarkus-plugin.version>
-    <quarkus.platform.artifact-id>quarkus-universe-bom</quarkus.platform.artifact-id>
+    <!--    <quarkus-plugin.version>1.12.2.Final</quarkus-plugin.version>-->
+    <quarkus-plugin.version>999-SNAPSHOT</quarkus-plugin.version>
+    <!--    <quarkus.platform.artifact-id>quarkus-universe-bom</quarkus.platform.artifact-id>-->
+    <quarkus.platform.artifact-id>quarkus-bom</quarkus.platform.artifact-id>
     <quarkus.platform.group-id>io.quarkus</quarkus.platform.group-id>
-    <quarkus.platform.version>1.12.2.Final</quarkus.platform.version>
+    <!--    <quarkus.platform.version>1.12.2.Final</quarkus.platform.version>-->
+    <quarkus.platform.version>999-SNAPSHOT</quarkus.platform.version>
     <surefire-plugin.version>3.0.0-M5</surefire-plugin.version>
   </properties>
   <dependencyManagement>
@@ -46,6 +49,22 @@
     <dependency>
       <groupId>io.rest-assured</groupId>
       <artifactId>rest-assured</artifactId>
+      <scope>test</scope>
+    </dependency>
+    <!-- extra dependencies  -->
+    <dependency>
+      <groupId>io.quarkus</groupId>
+      <artifactId>quarkus-agroal</artifactId>
+    </dependency>
+    <dependency>
+      <groupId>org.postgresql</groupId>
+      <artifactId>postgresql</artifactId>
+      <version>42.2.19</version>
+    </dependency>
+    <dependency>
+      <groupId>org.testcontainers</groupId>
+      <artifactId>postgresql</artifactId>
+      <version>1.15.2</version>
       <scope>test</scope>
     </dependency>
   </dependencies>
Index: src/main/resources/application.properties
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/main/resources/application.properties b/src/main/resources/application.properties
--- a/src/main/resources/application.properties	(revision e54195ed1a16d75554fc93aa608b22202c7271ea)
+++ b/src/main/resources/application.properties	(date 1616413573306)
@@ -1,0 +1,16 @@
+quarkus.datasource.db-kind=postgresql
+quarkus.datasource.username=user1
+
+quarkus.datasource.secondary.db-kind=postgresql
+quarkus.datasource.secondary.username=user1
+#quarkus.datasource.secondary.jdbc.driver=x
+
+# test datasources
+
+%test.quarkus.datasource.db-kind=other
+%test.quarkus.datasource.jdbc.driver=org.testcontainers.jdbc.ContainerDatabaseDriver
+%test.quarkus.datasource.jdbc.url=jdbc:tc:postgis:latest:///db1?currentSchema=public,schema1
+
+%test.quarkus.datasource.secondary.db-kind=other
+%test.quarkus.datasource.secondary.jdbc.driver=org.testcontainers.jdbc.ContainerDatabaseDriver
+%test.quarkus.datasource.secondary.jdbc.url=jdbc:tc:postgis:latest:///db1
Index: src/test/java/org/acme/getting/started/DatasourceTest.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/test/java/org/acme/getting/started/DatasourceTest.java b/src/test/java/org/acme/getting/started/DatasourceTest.java
new file mode 100644
--- /dev/null	(date 1616412296194)
+++ b/src/test/java/org/acme/getting/started/DatasourceTest.java	(date 1616412296194)
@@ -0,0 +1,23 @@
+package org.acme.getting.started;
+
+import io.agroal.api.AgroalDataSource;
+import io.quarkus.test.junit.QuarkusTest;
+import org.junit.jupiter.api.Test;
+
+import javax.inject.Inject;
+import java.sql.Connection;
+import java.sql.SQLException;
+
+import static org.wildfly.common.Assert.assertNotNull;
+
+@QuarkusTest
+public class DatasourceTest {
+    @Inject
+    AgroalDataSource dataSource;
+
+    @Test
+    public void testGetConnection() throws SQLException {
+        Connection connection = dataSource.getConnection();
+        assertNotNull(connection);
+    }
+}
\ No newline at end of file

stuartwdouglas added a commit to stuartwdouglas/quarkus that referenced this issue Mar 22, 2021
We trust the user that they have supplied an appropriate image.

Fixes quarkusio#15919
@stuartwdouglas
Copy link
Member

The driver issue is because you have not included our postgresql extension, just the driver directly.

I am not sure how it worked previously.

gsmet pushed a commit to stuartwdouglas/quarkus that referenced this issue Mar 23, 2021
We trust the user that they have supplied an appropriate image.

Fixes quarkusio#15919
@matjazs
Copy link
Author

matjazs commented Mar 23, 2021

I have updated the reproducer for the driver configuration issue and included the postgresql extension: https://github.com/matjazs/quarkus-test-named-vs-default-datasource-config

We actually do have a postgresql extension included in our code, but I have excluded it in reproducer initially to present the minimal setup causing the error we get in our code. Adding the extension doesn't really change the outcome.

@stuartwdouglas
Copy link
Member

I added the extension to the diff based reproducer above and it worked.

@quarkus-bot quarkus-bot bot added this to the 1.14 - main milestone Mar 23, 2021
@matjazs
Copy link
Author

matjazs commented Mar 23, 2021

That's strange, I have tried with locally built Quarkus snapshot using the latest main branch and also the one from Sonatype. The extension I have added is:

    <dependency>
      <groupId>io.quarkus</groupId>
      <artifactId>quarkus-jdbc-postgresql</artifactId>
    </dependency>

Maven and Java version

Apache Maven 3.6.3 (cecedd343002696d0abb50b32b541b8a6ba2883f)
Maven home: /Users/matjaz/.m2/wrapper/dists/apache-maven-3.6.3-bin/1iopthnavndlasol9gbrbg6bf2/apache-maven-3.6.3
Java version: 11.0.10, vendor: AdoptOpenJDK, runtime: /Library/Java/JavaVirtualMachines/adoptopenjdk-11.jdk/Contents/Home
Default locale: en_SI, platform encoding: UTF-8
OS name: "mac os x", version: "10.16", arch: "x86_64", family: "mac"

The test is still failing, am I missing something here?

@gsmet gsmet modified the milestones: 1.14 - main, 1.13.0.Final Mar 23, 2021
gsmet pushed a commit to gsmet/quarkus that referenced this issue Mar 23, 2021
We trust the user that they have supplied an appropriate image.

Fixes quarkusio#15919

(cherry picked from commit ad9be98)
@stuartwdouglas
Copy link
Member

Ah, I just added the extension and dev mode started working. This appears to be a config issue with profiles rather than something data source specific, I am investigating.

@stuartwdouglas
Copy link
Member

@radcortez this is caused by 8d85f22

calling ConfigSource.getPropertyNames() won't take profiles into account, so if the name is only defined in a property it will be ignored and the value won't be set.

What issue is this commit fixing? I think we need to revert it, and add a test for this scenario in our test-extension.

@radcortez
Copy link
Member

The commit was to fix the issue reported in #15389.

Since the defaults were registered in main properties, if you change the runtime profile, these may not be valid anymore, so I ended up registering all profiles in the default to be handled in the same way.

@radcortez
Copy link
Member

Let me have a look and I'll fix this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants