URI parsing adds additional properties to configuration rather than properties map.

[blafond]

I'm working in HR #1059 and trying to leverage the DB-specific SqlConnectOptions parsing of a URL string to allow additional properties on the URL but it looks like the parse logic isn't capturing these properties except for a few in the PgConnectionUriParser.

The expectation would be that additional properties would be returned with a call to SqlConnectOptions.getProperties()

By using this URL String url = "jdbc:postgresql://testuser:testpassword@localhost:11111/hreact?prop1=value1&prop2=value2"; and calling PgConnectOptions.fromUri(....) prop1 and prop2 are dropped because any extra parsed props are put into the configuration rather than the properties.put

I've changed configuration.put(key, value); to properties.put(key, value); locally and the properties are added to the options however, because the SqlConnectOptions properties are stored in a HashMap() and the conversion to JSON may end up failing in the fail assertEquals(expectedConfiguration, actualConfiguration); because the properties aren't necessarily ordered in a HashMap().

The following test added to PgConnectOptionsTest should work, but it fails.

  @Test
  public void testValidUriShouldSucceed() {
    connectionUri = "postgresql://?fallback_application_name=myapp&search_path=myschema&prop1=value1&prop2=value2&prop3=value3&loggerLevel=OFF";
    actualConfiguration = PgConnectOptions.fromUri(connectionUri);

    Map<String, String> expectedProperties = new HashMap<>();
    expectedProperties.put("fallback_application_name", "myapp");
    expectedProperties.put("search_path", "myschema");
    expectedProperties.put("prop1", "value1");
    expectedProperties.put("prop2", "value2");
    expectedProperties.put("prop3", "value3");
    expectedProperties.put("loggerLevel", "OFF");

    expectedConfiguration = new PgConnectOptions()
      .setProperties(expectedProperties);

    assertEquals(expectedConfiguration, actualConfiguration);
  }

Is there a reason why the assertEquals() check compares the JSON string only? I would think that the order of general JSON property elements shouldn't matter.

Maybe the assertEquals() could be modified to do a more specific check for equality and in particular specific additional properties?

[vietj]

I think equals/hashcode should not be implemented by this class.

[vietj]

so we should fix this bug and likely fix the existing equals/hashCode

[vietj]

the implementation of equals is actually wrong because it calls super.equals that is not implemented.

[vietj]

also the uri parser does a

    String key = parameterPair.substring(0, indexOfDelimiter).toLowerCase();

that I cannot explain

[vietj]

I think the idea is that some extra parameters of the connect options can be used from the JSON and apply to the vertx connect options, e.g specifying the idleTimeOut with an URI having a parameter like : &idleTimeout=10 that would be like options.setIdleTimeout(10)

[vietj]

so we could have unrecognised options as properties ? e.g do something like

1. parse to JSON
2. create connect options from JSON
3. transform connect options to JSON
4. take the initial JSON and remove all keys in the connect options JSON
5. take the remaining key/value and store them as properties

[vietj]

e.g

  public static PgConnectOptions fromUri(String connectionUri) throws IllegalArgumentException {
    JsonObject parsedConfiguration = PgConnectionUriParser.parse(connectionUri);
    PgConnectOptions options = new PgConnectOptions(parsedConfiguration);
    JsonObject foobar = options.toJson();
    for (Map.Entry<String, Object> entry : parsedConfiguration) {
      if (!foobar.containsKey(entry.getKey())) {
        options.getProperties().put(entry.getKey(), "" + entry.getValue());
      }
    }
    return options;
  }

that being said I'm not sure that settings those parameters as config for stuff like idleTimeout would work currently because it would only work for string parameters and currently the toLowerCase() conversion make it unusable.

[blafond]

@vietj Not sure why the conversion lower case other than to maybe simplify the switch(key) logic.

Seems to me that each XXConnectOptions implementation should adhere to the case of each of the DB's specific property key. Since JSON doesn't enforce lower case keys it should be doable, unless there's some other requirement in vertx/DB connection setup.

Been looking at where SqlConnectOptions.getProperties() is actually called.. to track down where vertx-sql-client is passing them on to the connection. I see PgDriver.newPoolImpl() method where baseConnectOptions is set, but I can't find where the options properties would be propagated to the PosgreSQL.

Is there some current example of a general property being set on connection?

[vietj]

what are the common connection properties that are not yet supported from URI parsing, e.g for PG.

[blafond]

From a hibernate-reactive standpoint, we're intending to allow a user to pass along any syntax/valid property on the URL. Problem with properties across DB types is there are few "common" properties.

[blafond]

Looks like the Driver classes consume the SqlConnectOptions which are used to create the PoolImpl.

So JsonObject maintains a "configuration". Is that object sent outside the Vertx clients? Just wondering if there's a reason why utilizing the properties map would be problematic.

[vietj]

well actually it seems that there is a well defined list of parameters for PG :

https://www.postgresql.org/docs/9.6/libpq-connect.html

Some are transmitted directly to the database like application_name, some are interpreted by the client like port or host or sslmode, the one currently transmitted and supported by the client are:

• application_name
• fallback_application_name
• search_path

Some are not supported but could be like keepalives.

Other values are not, so it is not clear for PostgreSQL what is the purpose of promoting any URI parameter as a database property.

[gavinking]

@vietj well of course the list is finite, but it's long, and it's unclear to me why you (or we) would want to have to keep an up-to-date list of them all in our clients.

Just pass through whatever the user specifies, and let Postgres sort it out. I don't see why we need to protect Postgres from receiving parameters it doesn't understand.

But perhaps I'm misunderstanding something here.

[DavideD]

Just pass through whatever the user specifies, and let Postgres sort it out. I don't see why we need to protect Postgres from receiving parameters it doesn't understand.

+1

[vietj]

@gavinking I don't disagree with you, I'm saying that some documented parameters are for the client and either the pg client supports them or it does not. So we need for the client side parameters to log at least that it will not work as expected.

[gavinking]

Ah, OK, fine, then I just misunderstood.

[vietj]

need to check also what happens with PG when it receives a parameter it does not support

[vietj]

I'm looking at how PGJDBC supports this, and it seems it only supports a fixed size of parameters to be sent at startup:

https://github.com/pgjdbc/pgjdbc/blob/473091ad3d60e1cd223b25727fe76d18f3b47f4e/pgjdbc/src/main/java/org/postgresql/core/v3/ConnectionFactoryImpl.java#L352

can you tell which parameters you need to send that you are missing ?

[DavideD]

I don't think Hibernate Reactive needs any specific property supported at the moment. But a user can add anything on the URL and we would like to be able to pass the additional query parameters to Vert.x without having to filter them first because it could generate an exception. A warning log would be nice though.

If the exception is the only way, we can always filter the properties before passing them to Vert.x

[blafond]

Agreed.

The commits (above) to my fork capture the extra properties during parsing and adds them to the configuration properties map.

However, when applied via an Hibernate Reactive connection URL, they create a PgExceptions like: unrecognized configuration parameter "defaultRowFetchSize" during the connect process.

So as @DavideD mentioned, we could either filter the properties on our end or capture those properties in Vertx parsers and log a WARNING message instead of throwing an exception.

[blafond]

@vietj I made some changes locally to just log a warning. Here's an example of what a warning log entry might look like:

INFO DefaultSqlClientPool [vert.x-worker-thread-0] HR000011: SQL Client URL [jdbc:postgresql://localhost:49219/hreact?loggerLevel=OFF&user=hreact&password=hreact]
[2022-01-11 13:15:38] WARNING io.vertx.pgclient.impl.PgConnectionUriParser parseParam  connection URL property: [ loggerLevel=OFF ] is not currently supported

[vietj]

yes we can log them of course

[vietj]

I will try to support more of them like TCP time out, etc... some seem not hard to implement.