Make JSON type loading conditional upon server support #1111
Conversation
| @@ -311,6 +311,14 @@ public TimeZone get() { | |||
| types.addCoreType("xml", Oid.XML, Types.SQLXML, "java.sql.SQLXML", Oid.XML_ARRAY); | |||
| } | |||
|
|
|||
| if (haveMinimumServerVersion(ServerVersion.v9_2)) { | |||
| types.addCoreType("json", Oid.JSON, Types.JAVA_OBJECT, "org.postgresql.util.PGobject", Oid.JSON_ARRAY); | |||
There was a problem hiding this comment.
Why do you use Types.JAVA_OBJECT here?
It looks like it was declared as Types.OTHER.
There was a problem hiding this comment.
JAVA_OBJECT, while not entirely correct is more meaningful than OTHER, and it makes no difference either way. But I don't have old server instances running to be able to test the conditional adding. I simply repeated the pattern of the other checks with the appropriate version.
There was a problem hiding this comment.
The PR is tested via Travis, and it does use multiple PostgreSQL versions (see https://travis-ci.org/pgjdbc/pgjdbc/builds/340856695?utm_source=github_status&utm_medium=notification )
JAVA_OBJECT, while not entirely correct is more meaningful thanOTHER
This change breaks backward compatibility. Is there a reason to do that?
I simply repeated the pattern of the other checks with the appropriate version.
The changes should be tested otherwise they can be silently reverted.
The tests are useful as code samples for those who want to use json/jsonb
There was a problem hiding this comment.
This change breaks backward compatibility. Is there a reason to do that?
I believe this change is justified, because OTHER can literally mean anything, and there is no standardized usage in higher-level APIs that would be broken from it. They're all still "cooking".
If I am to add tests for the presence of the two types, should I change the SQL type back to OTHER?
vlsi
force-pushed
the
master
branch
2 times, most recently
from
c25d807
to
adcb194
Compare
vlsi
force-pushed
the
master
branch
2 times, most recently
from
4c39f96
to
866c6a9
Compare
It's partly feature, partly fix. Not sure how to tag it.
And adding the JAVA_OBJECT to the acceptable null types fixes a small issue in an experiment of mine. The rest of the time it works fine, when the values are not nulls.
Changing the SQL type integer constant from OTHER to JAVA_OBJECT (while having no positive or negative effect) makes it consistent with the Hibernate JPA provider's choice for type association. Even though they don't fully support it out-of-the-box. I've checked with EclipseLink and OpenJPA and they're even further behind on this, with no
Reference 1