quarkusio / quarkus

Quarkus: Supersonic Subatomic Java.
https://quarkus.io
Apache License 2.0
13.73k stars 2.67k forks source link

Force Zero Data Source Configuration through clearing URL #39053

Closed c-schmitz-tsystems closed 7 months ago

c-schmitz-tsystems commented 7 months ago

Describe the bug

We have a default configuration for the default data source in our solution like so:

  datasource:
    db-kind: h2
    username: quarkus
    password: quarkus
    jdbc:
       url: jdbc:h2:mem:unittest;DB_CLOSE_DELAY=-1;NON_KEYWORDS=row;DATABASE_TO_UPPER=FALSE;CASE_INSENSITIVE_IDENTIFIERS=TRUE;

We would like to local developers the opportunity (optinal) to use devservices instead. Therefore the developer defines to following .env file:

quarkus.devservices.enabled=true
quarkus.datasource.db-kind=postgresql
quarkus.datasource.jdbc.url=
quarkus.datasource.devservices.port=5432
testcontainers.reuse.enable=false

Resetting the url to empty does not trigger the automatic spin up of the appropriate devservice. It only works when we removing the url from the default settings. But thats not an option.

It seems there are a difference between empty and non existent. But to be flexible enough there should be a solution for removing the properties, so that the connected functionality like devservice can work correctly!

Expected behavior

Set the datasource url to empty triggers the devservices.

Actual behavior

Set the datasource url to empty does not trigger the devservices. Instead a default datasouce configuration error comes up.

How to Reproduce?

No response

Output of uname -a or ver

No response

Output of java -version

No response

Quarkus version or git rev

No response

Build tool (ie. output of mvnw --version or gradlew --version)

No response

Additional information

No response

quarkus-bot[bot] commented 7 months ago

/cc @DavideD (hibernate-reactive), @gavinking (hibernate-reactive), @gsmet (hibernate-orm), @mswatosh (db2), @yrodiere (hibernate-orm)

yrodiere commented 7 months ago

I think that's related to something I saw discussed recently... @radcortez @maxandersen rings any bell? I think the discussion invovled @dmlloyd as well.

I don't think there's a way to "clear" properties due to how Smallrye Config treats empty strings as "no value", but I may be wrong.

A solution I can suggest would to specify this JDBC URL in a configuration profile, to set that profile by default, and to ask that developer to select another profile. But I'm not sure that's not possible right now without also affecting prod.

@geoand Do we have something similar to quarkus.test.profile/quarkus.test.integration-test-profile, but for dev mode?

E.g.:


quarkus:
  datasource:
    db-kind: postgresql
    username: quarkus
    password: quarkus
  dev:
    profile: dev-no-services # does "quarkus.dev.profile" even exist though?

"%dev-no-services":
  quarkus:
    devservices:
      enabled: false
    datasource:
      db-kind: h2
      jdbc:
         url: jdbc:h2:mem:unittest;DB_CLOSE_DELAY=-1;NON_KEYWORDS=row;DATABASE_TO_UPPER=FALSE;CASE_INSENSITIVE_IDENTIFIERS=TRUE;

"%dev":
  quarkus:
    devservices:
      enabled: true # That's the default but let's be explicit
    datasource:
      devservices:
        port: 5432

.env:

quarkus.dev.profile=dev
testcontainers.reuse.enable=false # Not sure what this is doing here but I'll leave it
geoand commented 7 months ago

@geoand Do we have something similar to quarkus.test.profile/quarkus.test.integration-test-profile, but for dev mode?

No, but it's not needed is it? Isn't quarkus.profile sufficient?

yrodiere commented 7 months ago

Wouldn't putting quarkus.profile in .env affect tests?

EDIT: And wouldn't putting quarkus.profile in application.properties affect prod? :thinking:

geoand commented 7 months ago

Ah okay, yeah.

I guess the only way to specify it in that case is as a system property

dmlloyd commented 7 months ago

I think that's related to something I saw discussed recently... @radcortez @maxandersen rings any bell? I think the discussion invovled @dmlloyd as well.

I don't think there's a way to "clear" properties due to how Smallrye Config treats empty strings as "no value", but I may be wrong.

In fact this should work. I think there must be something strange about how the JDBC URL is being handled if clearing the property is behaving differently than leaving it unset. I can't seem to locate the responsible code though. Looking at io.quarkus.agroal.runtime.DataSourceJdbcRuntimeConfig#url and its use site in io.quarkus.agroal.runtime.DataSources#doCreateDataSource doesn't reveal anything.

Is there more you can tell me about how (or where, in case I missed it) these two cases are handled?

yrodiere commented 7 months ago

Ok, if clearing a property is supposed to work... I just realized the syntax in the given .env file is simply wrong, so that must be it.

Quoting https://quarkus.io/guides/config-reference#env-file:

The name QUARKUS_DATASOURCE_PASSWORD follows the same conversion rules used for Environment variables.

So @c-schmitz-tsystems, you should replace this content in .env:

quarkus.devservices.enabled=true
quarkus.datasource.db-kind=postgresql
quarkus.datasource.jdbc.url=
quarkus.datasource.devservices.port=5432
testcontainers.reuse.enable=false

With this:

QUARKUS_DEVSERVICES_ENABLED=true
QUARKUS_DATASOURCE_DB_KIND=postgresql
QUARKUS_DATASOURCE_JDBC_URL=
QUARKUS_DATASOURCE_DEVSERVICES_PORT=5432
# Not sure about this one -- again, that's not the right place for this config,
# see https://java.testcontainers.org/features/reuse/
TESTCONTAINERS_REUSE_ENABLE=false
yrodiere commented 7 months ago

Ok, dumb me wasn't aware that we also support non-uppercase environment variables... I'd ask "why in the world do we even support that other weird uppercase syntax then", but well. I guess asking won't change much.

So... @dmlloyd Detection of whether we enable devservices or not based on the JDBC URL is there:

https://github.com/quarkusio/quarkus/blob/5bbae6fc32ca65188b91d0f86db987fde727332f/extensions/datasource/deployment/src/main/java/io/quarkus/datasource/deployment/devservices/DevServicesDatasourceProcessor.java#L241-L247

https://github.com/quarkusio/quarkus/blob/52925f72e93d67cd0fa5f78ca5119ce1bd98f0a0/extensions/datasource/deployment-spi/src/main/java/io/quarkus/datasource/deployment/spi/DevServicesDatasourceConfigurationHandlerBuildItem.java#L70-L75

It's indeed quite specific, relying on config from the deployment classpath, but I think it ought to work?

yrodiere commented 7 months ago

@c-schmitz-tsystems We'll probably need a reproducer, or at least debug logs from that developer who doesn't manage to clear the JDBC URL.

c-schmitz-tsystems commented 7 months ago

at first it makes no difference how the config keys in the env file are defined. both cases (upper lower) worked. with the above .env file i get the following error (liquibase is the first user of the default datasource):

Caused by: io.quarkus.runtime.configuration.ConfigurationException: Datasource '' is not configured. To solve this, configure datasource ''. Refer to https://quarkus.io/guides/datasource for guidance. at io.quarkus.datasource.common.runtime.DataSourceUtil.dataSourceNotConfigured(DataSourceUtil.java:47) at io.quarkus.liquibase.runtime.LiquibaseRecorder$1.apply(LiquibaseRecorder.java:39) ... 29 more

i think setting the url to an empty string is different than never setting this url. the dev services seems to activate only for the latter case.

yrodiere commented 7 months ago

Thanks, but we'd really need debug logs, not just the error (which is itself quite weird btw; did you give us your full config?)

yrodiere commented 7 months ago

FWIW this error message would mean that somewhere, you refer to a named datasource whose name is the empty string. Which is a completely different problem than dev services not activating.

Having config such as quarkus.liquibase."".something in your application.properties would be one way to get this error, but there are probably others.

Scratch that, you didn't format your error and GitHub removed <default>. There is no empty string involved.

Still, the datasource not being configured is not a problem with dev services. You probably have missing config somewhere, or Quarkus is mistakenly disabling the default datasource.

Really, we'd need a reproducer.

c-schmitz-tsystems commented 7 months ago

sorry, but i can't give you the code because of security issues and i haven't time to extract the relevant parts and try it out. the only special thing in our setup is, that we have a maven module that depends on another maven modul that contains the configuration of the data source in its application.properties file in the following way:

quarkus:
  log:
    level: DEBUG
    console:
      format: "%d{dd.MM.yyyy HH:mm:ss,SS z(Z)} %p [%C{2.}] (%t:%X{TRACE_ID}) %m %n"
    category:
      "com.telekom":
        level: DEBUG
  devservices:
    enabled: false
  datasource:
    db-kind: h2
    username: sa
    password:
    jdbc:
      url: jdbc:h2:mem:unittest;DB_CLOSE_DELAY=-1;NON_KEYWORDS=row;DATABASE_TO_UPPER=FALSE;CASE_INSENSITIVE_IDENTIFIERS=TRUE;

When i run the top level maven module (microservice) everything works with the h2 database, means: the configuration is correctly used from the base module. When i put the .env file (as defined above) in the top level maven module, the execution breaks with the above error, means: overwriting the settings that way does not have the whished effects.

i think the bug in quarkus is: empty string values in the config are not handled as null

radcortez commented 7 months ago

Ok, dumb me wasn't aware that we also support non-uppercase environment variables... I'd ask "why in the world do we even support that other weird uppercase syntax then", but well. I guess asking won't change much.

Because Docker containers don't have any restrictions on ENV names :(

radcortez commented 7 months ago

Problem here is that https://github.com/quarkusio/quarkus/blob/52925f72e93d67cd0fa5f78ca5119ce1bd98f0a0/extensions/datasource/deployment-spi/src/main/java/io/quarkus/datasource/deployment/spi/DevServicesDatasourceConfigurationHandlerBuildItem.java#L70-L75

Only checks if the property name is available in the configuration, but does not check the actual value.

radcortez commented 7 months ago

Even if we fix the empty issue, this will still not work. This is not only about clearing the value; it is about clearing the value and returning to the dev-services default.

The way we have this implemented is we generate the devservices url and record it as a default configuration with the lowest priority. A configuration override will use the new value and discard any other values set in lower-priority sources. When we clear the value, it is the same thing. The configuration is effectively removed, and we don't return to its previous value (in this case, the expectation was that it could fall back to start the dev services again). While this sounds reasonable, it does open the door to different behaviors in how each extension could use the configuration, which we should avoid.

My recommendation is to use profiles.

c-schmitz-tsystems commented 7 months ago

Thanks for your explanations. We still use profiles. Our default (checked in) profiles for dev and test uses a configuration that runs for all developers locally and in the gitlab pipeline. For better developer experience (Postgre instead of H2, additional ressources like KeyCloak) we provide .env files for a local dockerrized environment (test containers). This wont work for now.

radcortez commented 7 months ago

I'm closing this. If something is still missing, please feel free to ask.