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

Allow passing of parameters to reactive sql client connect options #25931

Merged
merged 1 commit into from
Jun 6, 2022

Conversation

edeandrea
Copy link
Contributor

Allow passing of parameters to reactive sql client connect options

Fixes #25914

@edeandrea
Copy link
Contributor Author

edeandrea commented Jun 2, 2022

@geoand / @Sanne I took a stab at this. I implemented it using a generic properties container that gets passed along to the underlying SqlConnectOptions.addProperty. This way, configuration like this works:

quarkus:
  datasource:
    username: myusername
    password: mypassword
    reactive:
      url: postgresql://free-tier14.aws-us-east-1.cockroachlabs.cloud:26257/defaultdb
      properties:
        options: --cluster=heroes-db-2463
      postgresql:
        ssl-mode: require

or

quarkus.datasource.username=myusername
quarkus.datasource.password=mypassword
quarkus.datasource.reactive.url=postgresql://free-tier14.aws-us-east-1.cockroachlabs.cloud:26257/defaultdb
quarkus.datasource.reactive.properties.options=--cluster=heroes-db-2463
quarkus.datasource.reactive.postgresql.ssl-mode=require

If you'd rather just have a specific config property for options rather than a Map for properties, I can change things around.

Copy link
Contributor

@tsegismont tsegismont left a comment

Choose a reason for hiding this comment

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

Thank you @edeandrea

Just one suggestion, not mandatory.

* Additional properties for the client, which will be sent to server at the connection start.
*/
@ConfigItem
public Map<String, String> properties;
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps, for consistency, we could call this additionalProperties:

/**
* Other unspecified properties to be passed to the JDBC driver when creating new connections.
*/
@ConfigItem
public Map<String, String> additionalJdbcProperties;

Copy link
Contributor Author

@edeandrea edeandrea Jun 2, 2022

Choose a reason for hiding this comment

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

Sure I can do that. I was staying consistent with the getProperties/setProperties/addProperty naming conventions on io.vertx.sqlclient.SqlConnectOptions rather than trying to match what the JDBC stuff calls it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you

@edeandrea edeandrea marked this pull request as ready for review June 2, 2022 14:08
@quarkus-bot

This comment has been minimized.

@kdubb
Copy link
Contributor

kdubb commented Jun 2, 2022

@edeandrea Just by reading, the naming is a little confusing. Does cockroach support a PGConnectOptions option named options?

We need to set the search_path, would I do that as follows?

quarkus.datasource.username=myusername
quarkus.datasource.password=mypassword
quarkus.datasource.reactive.url=postgresql://host/db
quarkus.datasource.reactive.additional-properties.search_path=my_schema,public

@kdubb
Copy link
Contributor

kdubb commented Jun 2, 2022

BTW the naming issues start with vertx. As you know the name of the class is PGConnectOptions but it has addProperty and addProperties. So, naming it additionalProperties also seems inline as long as we ignore the "options" outlier.

@edeandrea
Copy link
Contributor Author

edeandrea commented Jun 3, 2022

@kdubb CockroachDB serverless DBaaS requires a connection parameter named options which has a value of --cluster=<cluster_name>. See their documentation (https://www.cockroachlabs.com/docs/v22.1/connection-parameters#connect-using-a-url).

With this change, quarkus.datasource.reactive.additional-properties.search_path=my_schema,public would set

search_path=my_schema,public as a property on the SqlConnectOptions (the parent class of PgConnectOptions). This change flows on all the reactive drivers, not just the postgres one.

@kdubb
Copy link
Contributor

kdubb commented Jun 3, 2022

@edeandrea Perfect. Thanks this is exactly what we need.

@gsmet This seems ready to go. Who do we ping to get this in for 2.10? I don't think I'm allowed to exercise my labeling powers to add this to the milestone lol

@geoand
Copy link
Contributor

geoand commented Jun 3, 2022

Who do we ping to get this in for 2.10? I don't think I'm allowed to exercise my labeling powers to add this to the milestone lol

As long as it's merged in the next few days, it will be part of 2.10.

@Sanne can you take a look at it? It looks good to me, but this is not my wheelhouse.

@@ -145,4 +146,10 @@ public class DataSourceReactiveRuntimeConfig {
*/
@ConfigItem
public Optional<String> name;

/**
* Other unspecified properties to be passed to the Reactive SQL Client when creating new connections.
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if this should be clarified: technically these options are passed directly to the PG database server each time a new connection is initiated.

Copy link
Contributor Author

@edeandrea edeandrea Jun 6, 2022

Choose a reason for hiding this comment

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

@Sanne What if we said something like this?

Other unspecified properties to be passed through the Reactive SQL Client directly to the database when new connections are initiated.

Not that this change is for all the reactive sql clients - not just the PG one.

Copy link
Member

Choose a reason for hiding this comment

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

yes sounds good! thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Just pushed an update.

@quarkus-bot

This comment has been minimized.

Copy link
Member

@Sanne Sanne left a comment

Choose a reason for hiding this comment

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

Great, thanks!

@Sanne Sanne added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jun 6, 2022
@Sanne Sanne merged commit 3ad7cd0 into quarkusio:main Jun 6, 2022
@quarkus-bot quarkus-bot bot added this to the 2.10 - main milestone Jun 6, 2022
@quarkus-bot quarkus-bot bot added kind/enhancement New feature or request and removed triage/waiting-for-ci Ready to merge when CI successfully finishes labels Jun 6, 2022
@edeandrea edeandrea deleted the reactive-sql-props branch June 6, 2022 21:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Postgres reactive configuration doesn't allow for arbitrary parameters on connect string
5 participants