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

Reactive datasource should not have a default for quarkus.datasource.reactive.url #43517

Closed
yrodiere opened this issue Sep 26, 2024 · 3 comments · Fixed by #41929
Closed

Reactive datasource should not have a default for quarkus.datasource.reactive.url #43517

yrodiere opened this issue Sep 26, 2024 · 3 comments · Fixed by #41929
Milestone

Comments

@yrodiere
Copy link
Member

yrodiere commented Sep 26, 2024

Describe the bug

I noticed while working on #41929 that:

  • For JDBC datasources, we don't even try to connect to a database if the JDBC URL is not set
  • For Reactive datasources, we default to localhost and a DB-specific port if the Reactive URL is not set

This is inconsistent, and IMO the reactive behavior is at best unnecessary, at worst dangerous:

  1. In dev mode, people should generally use dev services, so the URL will always be set, so this is irrelevant.
  2. In prod mode, people will most likely need dedicated config -- specific DB name at the very least, and most likely authentication.
  3. In either dev mode or prod, people might not want to connect to some arbitrary, default DB that just happens to be there, and potentially start messing with the database schema (e.g. with Hibernate).

What's more, this default is not documented at all:

image

I would suggest we align reactive datasources on the behavior of JDBC datasources.

Expected behavior

Reactive datasources should behave similarly to JDBC datasources when the reactive URL is unset. Or, to be precise, they should behave similarly to JDBC datasources after #41929 gets fixed:

  1. Runtime startup should fail if such a datasource is statically injected.
  2. Dynamic retrieval of such such a datasource through Arc should throw an exception.
  3. Such a datasource should not appear in health checks.

Actual behavior

  1. Runtime startup does not fail if such a datasource is statically injected.
  2. Dynamic retrieval of such such a datasource through Arc does not throw an exception.
  3. Such a datasource appears in health checks, with a DOWN status.

How to Reproduce?

I will add tests in #41929; I may send a separate PR with just the tests.

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

@yrodiere yrodiere added kind/bug Something isn't working area/reactive-sql-clients labels Sep 26, 2024
@yrodiere
Copy link
Member Author

I would suggest we align reactive datasources on the behavior of JDBC datasources.

Hey @tsegismont, would you agree with ^ ?

If so I'll try to incorporate the change into my PR (#41929)

@tsegismont
Copy link
Contributor

Yes, fine with me. This default is inherited from bare Vert.x SQL Clients, but it makes sense to align behavior of JDBC/Reactive datasources in Quarkus

@gsmet
Copy link
Member

gsmet commented Sep 26, 2024

FWIW, I'm also for consistency. I wouldn't qualify it as a bug though as it's the intended behavior. Changing the kind label.

@gsmet gsmet added kind/enhancement New feature or request and removed kind/bug Something isn't working labels Sep 26, 2024
@yrodiere yrodiere changed the title Reactive datasource defaults to localhost and DB-specific port for quarkus.datasource.reactive.url Reactive datasource should not have a default for quarkus.datasource.reactive.url Sep 26, 2024
@quarkus-bot quarkus-bot bot added this to the 3.17 - main milestone Oct 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants