postgres-kit icon indicating copy to clipboard operation
postgres-kit copied to clipboard

Update use of `connect` and `authenticate` to fix warnings

Open 0xTim opened this issue 2 years ago • 3 comments

We use a couple of APIs in PostgresNIO(?) that are deprecated:

/Users/timc/Developer/Vapor/postgres-kit/Sources/PostgresKit/PostgresConnectionSource.swift:58:39: warning: 'connect(to:tlsConfiguration:serverHostname:logger:on:)' is deprecated: Use the new connect method that allows you to connect and authenticate in a single step
            return PostgresConnection.connect(
                                      ^
/Users/timc/Developer/Vapor/postgres-kit/Sources/PostgresKit/PostgresConnectionSource.swift:58:39: note: use 'connect(on:configuration:id:logger:)' instead
            return PostgresConnection.connect(
                                      ^~~~~~~
                                      connect
/Users/timc/Developer/Vapor/postgres-kit/Sources/PostgresKit/PostgresConnectionSource.swift:65:29: warning: 'authenticate(username:database:password:logger:)' is deprecated: Use the new connect method that allows you to connect and authenticate in a single step
                return conn.authenticate(
                            ^
/Users/timc/Developer/Vapor/postgres-kit/Sources/PostgresKit/PostgresConnectionSource.swift:65:29: note: use 'connect(on:configuration:id:logger:)' instead
                return conn.authenticate(
                            ^~~~~~~~~~~~
                            connect
[921/921] Compiling PostgresKit PostgresRow+SQL.swift
Build complete! (26.69s)

We should migrate to the new APIs to remove the warnings

0xTim avatar Jul 09 '22 08:07 0xTim

If I understand the issue correctly, PostgresNIO would also requires some changes, right now the only public connect function takes a PostgresConnection.Configuration where the Connection struct only accept host and port. Those warnings come from the condition where you don't have a hostname, just a Unix domain socket. Will need PostgresNIO support before address them.

fatto avatar Aug 18 '22 15:08 fatto

Depends on Adding uds connection support #302

fatto avatar Aug 19 '22 20:08 fatto

After studying this issue I have come to a conclusion that additional changes will need to be made to the PostgresNIO package to fix this issue.

Currently, there are four ways to initialize a PostgresConfiguration in the PostgresKit:

  • URL(String)
  • URL(URL)
  • Host:Port
  • UnixDomianSocketPath.

The URL initializers parse the URL into the PostgresKit/PostgresConfiguration properties including the internal host and port fields. The Host:Port initializer will also populate the internal host and port properties. The UnixDomainSocketpath initializer does not populate the internal host and port properties. It sets them to 'nil'.

The PostgresNIO/PostgresConfiguration.Connection structure requires a hostname and port. They cannot be nil. Because the 'UnixDomainSocketPath" initializer does not provide host and port properties the current implementation of the PostgresKit/PostgresConnectionSource uses the deprecated connect method that includes a socket address instead of hostname and port. This in essence is the issue.

The address property in the PostgresKit/PostgresConfiguration structure is a 'public' property. I am not sure why it is public since it is defaulted to a closure during 'UnixSomainSockerPath' initialization. I guess it could be set after the fact but it doesn't make much sense. Note that it is also set during URL and Host:Port initialization but is never used for connection purposes.

As mentioned, PostgresNIO will have to changed so that the host property on the PostgresConnection.Configuration structure will accept a UnixdomainSocketPath. This is what is done on the libpq-C library. The host property can then be used to accept both a UnixDomainSockerpath or a host name.

brucemontegani avatar Aug 21 '22 04:08 brucemontegani