Signature of org.postgresql.jdbc.TimestampUtils.toOffsetDateTime(java.lang.String) changed

[busches]

Describe the issue
Upgrading from Postgres Driver 42.3.3 to 42.3.4 results in:

java.lang.NoSuchMethodError: 'java.time.OffsetDateTime org.postgresql.jdbc.TimestampUtils.toOffsetDateTime(java.lang.String)'
	at io.debezium.connector.postgresql.connection.PostgresDefaultValueConverter.lambda$createDefaultValueMappers$22(PostgresDefaultValueConverter.java:168)
	at io.debezium.connector.postgresql.connection.PostgresDefaultValueConverter.parseDefaultValue(PostgresDefaultValueConverter.java:77)
	at io.debezium.relational.TableSchemaBuilder.lambda$addField$9(TableSchemaBuilder.java:391)
	at java.base/java.util.Optional.flatMap(Unknown Source)

Driver Version?
42.3.4

Java Version?
17

OS Version?
Ubunutu Focal (20.04)

PostgreSQL Version?
11.3

To Reproduce
Use 'java.time.OffsetDateTime org.postgresql.jdbc.TimestampUtils.toOffsetDateTime(java.lang.String)' in your code or a library (in this case Debezium) with 42.3.3, upgrade to 42.3.4.

Expected behaviour
I would not expect the public API to change in a bug fix version and if it was, to be called out as a breaking change in the notes. The Java Doc still shows the old version: https://jdbc.postgresql.org/documentation/publicapi/org/postgresql/jdbc/TimestampUtils.html#toLocalDateTime-java.lang.String-

This change was introduced here: #2467

[vlsi]

Here's the change: https://github.com/pgjdbc/pgjdbc/pull/2467/files#diff-21e3023c84d9d5fb4f5f04678a094fce6ab5af36a5e73fdfaf495646f3b720d5R523

[davecramer]

You are quite correct. I will fix this ASAP

…

On Fri, Apr 22, 2022, 1:13 PM Vladimir Sitnikov ***@***.***> wrote: Here's the change: https://github.com/pgjdbc/pgjdbc/pull/2467/files#diff-21e3023c84d9d5fb4f5f04678a094fce6ab5af36a5e73fdfaf495646f3b720d5R523 — Reply to this email directly, view it on GitHub <#2497 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADDH5TAQELJ25FIDW4WF4LVGLM4VANCNFSM5UC3APZQ> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[davecramer]

So while this function is declared public. This is not part of the public API. We don't publish this API. I'm not sure we can be responsible for making sure this API doesn't change in minor versions.
Comments ?

[vlsi]

The sad thing is that TimestampUtils was not in "internal" package, and its javadoc does not suggest the class is not public.
So it looks like the way to go here is to revert all the previous signatures, and mark them as deprecated.

[vlsi]

Here's the corresponding change in debezium: debezium/debezium#2339

[vlsi]

We don't publish this API

Unfortunately, we have no clear way to declare the published API, so it looks like we should be careful of keeping all "looking as public" APIs in both minor and major releases.

[davecramer]

@vlsi agree with everything above. However we have to be free to change our internal code. I honestly doubt if documenting that it is not public will help.

[vlsi]

Moving classes to .internal. package might help to surface the issues during the code review.
For instance, if Debezium PR added import org.postgresql.internal..., then it might raise @gunnarmorling's eyebrow.

I honestly doubt if documenting that it is not public will help.

I hope documenting BaseConnection helps:

pgjdbc/pgjdbc/src/main/java/org/postgresql/core/BaseConnection.java

Lines 25 to 27 in ffda18b

	* Driver-internal connection interface. Application code should not use this interface.
	*/
	public interface BaseConnection extends PGConnection, Connection {

However, it looks like .internal. package is the most visible marker for the users.

[busches]

What you're discussing is solved by using Java modules. With that said you publish a Java doc for this method, seems pretty public to an outsider looking in.

[vlsi]

I'm not sure we are ready to drop Java 8 support yet, so module-info.java does not seem to be a solution yet.

[davecramer]

Ya I think moving everything into .internal would be my vote.

Documentation is more for us than anything. The fact that there are javadocs is really an artifact of the build process.
As driver writers we really need the freedom to change anything below the JDBC spec.

[busches]

So do I need to get debezium to change their usage and only allow, 42.3.4+ or can we restore the old method and deprecate it?

[gunnarmorling]

Hey all, did a quik grep in the Debezium code base, and these are the Postgres driver classes we import:

org.postgresql.Driver
org.postgresql.PGStatement
org.postgresql.core.BaseConnection
org.postgresql.core.Oid
org.postgresql.core.ServerVersion
org.postgresql.core.TypeInfo
org.postgresql.geometric.PGbox
org.postgresql.geometric.PGcircle
org.postgresql.geometric.PGline
org.postgresql.geometric.PGpath
org.postgresql.geometric.PGpoint
org.postgresql.geometric.PGpolygon
org.postgresql.jdbc.PgArray
org.postgresql.jdbc.PgConnection
org.postgresql.jdbc.PgDatabaseMetaData
org.postgresql.jdbc.PgStatement
org.postgresql.jdbc.TimestampUtils
org.postgresql.replication.LogSequenceNumber
org.postgresql.replication.PGReplicationStream
org.postgresql.replication.fluent.logical.ChainedLogicalStreamBuilder
org.postgresql.util.HStoreConverter
org.postgresql.util.PGInterval
org.postgresql.util.PGmoney
org.postgresql.util.PGobject
org.postgresql.util.PSQLException
org.postgresql.util.PSQLState

Which one of those would you consider as implementation details of the driver? And what would be the public API alternatives to them?

[davecramer]

Off the top of my head the easy ones are:

org.postgresql.replication.LogSequenceNumber
org.postgresql.replication.PGReplicationStream
org.postgresql.replication.fluent.logical.ChainedLogicalStreamBuilder
org.postgresql.util.HStoreConverter
org.postgresql.util.PGInterval
org.postgresql.util.PGmoney
org.postgresql.util.PGobject
org.postgresql.geometric.PGbox
org.postgresql.geometric.PGcircle
org.postgresql.geometric.PGline
org.postgresql.geometric.PGpath
org.postgresql.geometric.PGpoint
org.postgresql.geometric.PGpolygon

But let me turn this question around and ask you the same?

I think the solution to this is to create external classes for what you or others may need that implement what you are using and then have internal classes that we are free to modify as we see fit.

Thought ?

[uschindler]

I don't fully understand why one would like to use TimestampUtils. This class is also on the list of classes to be removed once the rewrite to java.time is done.

I have no problem to restore an old deprecated signature back, but please don't restore the buggy code from before.

In Apache Lucene we have a javadocs annotation "@lucene.internal". If anybody complains we refer to it an close issue as won't fix.

[uschindler]

I just checked, the additional paramter "adoptToUTC" may default to true in the deprecated replicate. I see no issue to add it back. The change was needed for OffsetTime support, because there the adoption of timezone is wrong.

Nevertheless, TimestampUtils is to be going away soon, so we should mark it as internal. See my comment above.

[uschindler]

I would add this deprecated change:

@Deprecated
public @PolyNull OffsetDateTime toOffsetDateTime(@PolyNull String s) throws SQLException {
  return toOffsetDateTime(s, true);
}

[uschindler]

I'm not sure we are ready to drop Java 8 support yet, so module-info.java does not seem to be a solution yet.

And this would not help with classpath users. Most projects out there still use Java 17 with classpath compilation. So adding module info helps almost nothing.

[uschindler]

Ya I think moving everything into .internal would be my vote.

Documentation is more for us than anything. The fact that there are javadocs is really an artifact of the build process. As driver writers we really need the freedom to change anything below the JDBC spec.

I would do this two-side: If the class is only used from one package, move it to that package and make it pkg-protected. Otherwise, yes ".internal." package is fine. We do the same in Lucene after we modularized all of lucene and ran into package name conflicts: https://lucene.apache.org/core/9_1_0/core/org/apache/lucene/internal/tests/package-summary.html (of course, it is not listed in module-info.java)

[davecramer]

closed with #2501

[uschindler]

Thanks and sorry for the trouble! 😵‍💫

[gunnarmorling]

I don't fully understand why one would like to use TimestampUtils

Sorry, just getting back to this, as I was travelling for Kafka Summit last week. So we use this class for converting temporal values we receive from PG in change events into the properly typed types. We could do this ourselves, but then we'd re-implement the logic of the driver, e.g. including infinity handling. So I'd argue there's reasonable cause for having those methods as a supported driver API?

Taking a step back, I've lost track what exactly has changed now and what we need to do when upgrading to the new driver version. I suppose a number of compile errors will tell us?

[davecramer]

@gunnarmorling
Agreed on providing some standard conversion classes. I guess we weren't really aware anyone was relying on these.
Now that we are aware we will take some care in maintaining them as a public API. That said things may change, but we will be more transparent in the future.

As for what you will have to do. 42.3.5 will revert these changes and your code will work as is.

[gunnarmorling]

Ok, perfect thanks. Note I'm very sympathetic towards the concern of separating API and implementation parts of a library such as the driver; so if we currently are referencing things you actually consider internal, we'll happily try and make the required adjustments, assuming there's some deprecation period and ideally a public replacement for that functionality.