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

Error upgrading from 1209 to 1210 #639

Closed
mbryzek opened this Issue Sep 13, 2016 · 21 comments

Comments

Projects
None yet
4 participants
@mbryzek

mbryzek commented Sep 13, 2016

Today we attempted to upgrade from 1209 to 1210 (no other changes except the psql driver). We are using com.typesafe.play.anorm version 2.5.2

It appears something has changed in the psql driver with respect to extracting type information.

Any chance you all may be able to give us a pointer on how to upgrade? Included info below on error, table structure, and anorm parser.

Much appreciated!

anorm.AnormException: TypeDoesNotMatch(Column[ColumnName(applications.ports,Some(ports))] error converting [{"service":{"id":"svc02bac17cd6624e749087fd596211e988"},"external":12772,"internal":8080}]: class java.lang.String to Json)
registrydb=# \d applications
                      Table "public.applications"
       Column       |           Type           |       Modifiers
--------------------+--------------------------+------------------------
 id                 | text                     | not null
 ports              | json                     | not null
 dependencies       | json                     | not null
 created_at         | timestamp with time zone | not null default now()
 updated_by_user_id | text                     | not null
    select applications.id,
           applications.ports,
           applications.dependencies
      from applications
  private[this] def parser(
    id: String = "id",
    ports: String = "ports",
    dependencies: String = "dependencies"
  ): RowParser[io.flow.registry.v0.models.Application] = {
    SqlParser.str(id) ~
    SqlParser.get[Seq[JsObject]](ports).? ~
    SqlParser.get[Seq[JsObject]](dependencies).? map {
      case id ~ ports ~ dependencies => {
        io.flow.registry.v0.models.Application(
          id = id,
          ports = ports.getOrElse(Nil).map { _.as[Port] },
          dependencies = dependencies.getOrElse(Nil).map { _.as[String] }
        )
      }
    }
  }
@davecramer

This comment has been minimized.

Member

davecramer commented Sep 13, 2016

What does the Port object look like ?

Dave Cramer

On 13 September 2016 at 07:01, Michael Bryzek notifications@github.com
wrote:

Today we attempted to upgrade from 1209 to 1210 (no other changes except
the psql driver). We are using com.typesafe.play.anorm version 2.5.2

It appears something has changed in the psql driver with respect to
extracting type information.

Any chance you all may be able to give us a pointer on how to upgrade?
Included info below on error, table structure, and anorm parser.

Much appreciated!

anorm.AnormException: TypeDoesNotMatch(Column[ColumnName(applications.ports,Some(ports))] error converting [{"service":{"id":"svc02bac17cd6624e749087fd596211e988"},"external":12772,"internal":8080}]: class java.lang.String to Json)

registrydb=# \d applications
Table "public.applications"
Column | Type | Modifiers
--------------------+--------------------------+------------------------
id | text | not null
ports | json | not null
dependencies | json | not null
created_at | timestamp with time zone | not null default now()
updated_by_user_id | text | not null

select applications.id,
       applications.ports,
       applications.dependencies
  from applications

private[this] def parser(
id: String = "id",
ports: String = "ports",
dependencies: String = "dependencies"
): RowParser[io.flow.registry.v0.models.Application] = {
SqlParser.str(id) ~
SqlParser.getSeq[JsObject].? ~
SqlParser.getSeq[JsObject].? map {
case id ~ ports ~ dependencies => {
io.flow.registry.v0.models.Application(
id = id,
ports = ports.getOrElse(Nil).map { _.as[Port] },
dependencies = dependencies.getOrElse(Nil).map { _.as[String] }
)
}
}
}


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#639, or mute the thread
https://github.com/notifications/unsubscribe-auth/AAYz9sMysbskg0981A6-OQbsnJT0TGbIks5qppCvgaJpZM4J7l3p
.

@vlsi

This comment has been minimized.

Member

vlsi commented Sep 13, 2016

I think this change is relevant: #610

I wonder if you could provide a standalone testcase.

I'm inclined to suppose that that is pgjdbc's issue as it uses Types.VARCHAR for json column. I would expect something like Types.OTHER there: https://github.com/pgjdbc/pgjdbc/pull/610/files#diff-49fcaf90c4908b482951125628e65c3bR95

@mbryzek

This comment has been minimized.

mbryzek commented Sep 13, 2016

example record:

select * from applications limit 1;
-[ RECORD 1 ]------+------------------------------------------------------------------------------------------------------------------------
id                 | app502ec7ba4e254e7aa6c5072098260812
ports              | [{"service":{"id":"nodejs"},"external":6010,"internal":7050},{"service":{"id":"play"},"external":6011,"internal":9000}]
dependencies       | []
created_at         | 2016-06-08 16:00:02.540745-04
updated_by_user_id | tst-20160608-726793331

Yes - change 610 looks relevant. Will try to put together a standalone test case - thanks for the quick reply!

@mbryzek

This comment has been minimized.

mbryzek commented Sep 14, 2016

FYI - so far have not been able to isolate the test in the pgsql libraries. I'm not too familiar with anorm; bug is preventing us from upgrading (but struggling to provide an accurate test in java in this repo).

If we were to provide a test case in scala w/ anorm dependency - would that be helpful?

@vlsi

This comment has been minimized.

Member

vlsi commented Sep 14, 2016

If we were to provide a test case in scala w/ anorm dependency - would that be helpful?

Of course it would.
It is good to increase test coverage, even via "obscure" combinations used in the wild.

By the way: I meant anorm kind of test case that we can put into pgjdbc repository and execute at Travis

@mbryzek

This comment has been minimized.

mbryzek commented Sep 15, 2016

A test to show the regression from 1209 to 1210 is at:

https://github.com/flowcommerce/pgbug

hopefully the readme is self explanatory. I am not sure how to put this into pgjdbc repo.

The main change is that the type for json columns has changed from

org.postgresql.util.PGobject

to

java.lang.String

now that I understand the change - we can work around it - but still hopefully useful for you as I think this was an intended breaking change in the driver.

@rymndhng

This comment has been minimized.

rymndhng commented Sep 17, 2016

I've also noticed this as an issue in Clojure when trying to parse the data into JSON with https://github.com/clojure/java.jdbc.

@vlsi

This comment has been minimized.

Member

vlsi commented Sep 17, 2016

@rymndhng , thanks for the java.jdbc case. I've tried to run it against my local PostgreSQL 9.5 and run into some test errors.

Can you probably make some sense of the test error messages? (see clojure/java.jdbc#44)

@rymndhng

This comment has been minimized.

rymndhng commented Sep 17, 2016

Right, so typically users add automatic JSON deserialization by openly extending PGobject like so:

(extend-protocol jdbc/IResultSetReadColumn
  PGobject.  ;; <-- this is org.postgresql.util.PGObject
  (result-set-read-column [pgobj metadata idx]
    (let [type  (.getType pgobj)
          value (.getValue pgobj)]
      (case type
        "json" (json/read-str value :key-fn keyword)
        :else value))))

This will deserialize JSON columns automatically. However, now that the type of a JSON column is a regular String, we have no way of checking if the contents came from a JSON column or not.

@vlsi

This comment has been minimized.

Member

vlsi commented Sep 17, 2016

This must be something else.

I did try testing against 9.4.1209, and the error message is the same:
TEST_DBS=postgres TEST_POSTGRES_USER=test TEST_POSTGRES_DBNAME=test mvn test -Djava.jdbc.test.pgjdbc.version=9.4.1209

Testing clojure.java.test-jdbc

FAIL in (test-metadata-managed) (test_jdbc.clj:770)
expected: (= "fruit" (-> table-info first :table_name clojure.string/lower-case))
  actual: (not (= "fruit" "evil.table"))

FAIL in (test-metadata-managed-computed) (test_jdbc.clj:778)
expected: (= "fruit" (sql/with-db-metadata [metadata db] (sql/metadata-query (.getTables metadata nil nil nil (into-array ["TABLE" "VIEW"])) {:row-fn (comp clojure.string/lower-case str :table_name), :result-set-fn first})))
  actual: (not (= "fruit" "evil.table"))

FAIL in (test-metadata) (test_jdbc.clj:796)
expected: (= "fruit" (-> table-info first :table_name clojure.string/lower-case))
  actual: (not (= "fruit" "evil.table"))

FAIL in (test-raw-metadata) (test_jdbc.clj:757)
expected: (= "fruit" (-> table-info first :table_name clojure.string/lower-case))
  actual: (not (= "fruit" "evil.table"))

Ran 70 tests containing 168 assertions.
4 failures, 0 errors.
@rymndhng

This comment has been minimized.

rymndhng commented Sep 17, 2016

Let me create you a sample repo to show what has changed.

@vlsi

This comment has been minimized.

Member

vlsi commented Sep 17, 2016

Just in case: I've pushed a fix for json, and scala test works fine: https://travis-ci.org/pgjdbc/pgjdbc/jobs/160642403#L2844

Here's Scala test: https://github.com/pgjdbc/pgjdbc/tree/master/test-anorm-sbt

PS. I'm fine with adding "checkout java.jdbc; mvn test" to regular pgjdbc test suite (see here: https://github.com/pgjdbc/pgjdbc/blob/master/.travis/travis_build.sh#L73-L76 )

@vlsi

This comment has been minimized.

Member

vlsi commented Sep 17, 2016

@mbryzek, if you wish, we could externalize "test-anorm-sbt" (I mean we could use checkout & run tests approach for Travis tesing), however I just thought it makes sense to copy the source right into pgjdbc tree as the repository is not that big

@vlsi vlsi closed this in beaec3a Sep 17, 2016

@rymndhng

This comment has been minimized.

rymndhng commented Sep 17, 2016

Thanks for the speedy response. �

@vlsi

This comment has been minimized.

Member

vlsi commented Sep 17, 2016

PS. I'm still interested in un-commenting https://github.com/pgjdbc/pgjdbc/blob/master/.travis/travis_build.sh#L73-L76, so feel free to file a PR.

@mbryzek

This comment has been minimized.

mbryzek commented Sep 18, 2016

@vlsi copying right in is great!

Thanks all for your help and work on the driver.

@vlsi

This comment has been minimized.

Member

vlsi commented Sep 18, 2016

So is it time to cut 1211? Does 9.4.1211-SNAPSHOT work fine for you?

@mbryzek

This comment has been minimized.

mbryzek commented Sep 18, 2016

Confirmed that 9.4.1211-SNAPSHOT works

On Sun, Sep 18, 2016 at 7:59 AM, Vladimir Sitnikov <notifications@github.com

wrote:

So is it time to cut 1211? Does 9.4.1211-SNAPSHOT work fine for you?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#639 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AADC06uzfcx9fyVB6EHh_3g-xiC4sUN9ks5qrSeqgaJpZM4J7l3p
.

@vlsi

This comment has been minimized.

Member

vlsi commented Sep 18, 2016

I've pushed PgJDBC 9.4.1211 to Central. Hopefully @davecramer will update the jdbc.postgresql.org soon.

@mbryzek

This comment has been minimized.

mbryzek commented Sep 18, 2016

Thanks! We're all upgraded and all looks good

On Sun, Sep 18, 2016 at 1:55 PM, Vladimir Sitnikov <notifications@github.com

wrote:

I've pushed PgJDBC 9.4.1211 to Central. Hopefully @davecramer
https://github.com/davecramer will update the jdbc.postgresql.org soon.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#639 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AADC0yzkiyGv6AUbMxmkOK_psF6TAFiLks5qrXsagaJpZM4J7l3p
.

@davecramer

This comment has been minimized.

Member

davecramer commented Sep 19, 2016

Site has been updated, thanks!

Dave Cramer

On 18 September 2016 at 16:24, Michael Bryzek notifications@github.com
wrote:

Thanks! We're all upgraded and all looks good

On Sun, Sep 18, 2016 at 1:55 PM, Vladimir Sitnikov <
notifications@github.com

wrote:

I've pushed PgJDBC 9.4.1211 to Central. Hopefully @davecramer
https://github.com/davecramer will update the jdbc.postgresql.org
soon.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#639 (comment),
or mute
the thread
<https://github.com/notifications/unsubscribe-
auth/AADC0yzkiyGv6AUbMxmkOK_psF6TAFiLks5qrXsagaJpZM4J7l3p>
.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#639 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAYz9pKafzogUdD_nmvLGSswVWi5HGksks5qrZ4NgaJpZM4J7l3p
.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment