typelevel / doobie

Functional JDBC layer for Scala.
MIT License
2.17k stars 359 forks source link

Analysis produces false positives for enums in a non-default schema #1474

Open nigredo-tori opened 3 years ago

nigredo-tori commented 3 years ago

With Doobie 0.12.1 and PostgreSQL 11. Given this setup:

create schema custom;
create type custom.foo as enum('Bar');
create type foo as enum('Bar');

an analysis of a simple query using the type in the custom schema produces an error:

scala> val inCustom = sql"select 'Bar'::custom.foo".query[Foo].analysis.transact(xa).unsafeRunSync
inCustom: doobie.util.analysis.Analysis = Analysis(select 'Bar'::custom.foo,List(),List(Both((Basic(NonEmptyList(Some(Foo), Some(String)),NonEmptyList(Other, VarChar),List(),cats.free.Coyoneda$$anon$2@613d7c91),NoNulls),ColumnMeta(Other,"custom"."foo",NullableUnknown,foo))))

scala> inCustom.columnTypeErrors
res35: List[doobie.util.analysis.ColumnTypeError] = List(ColumnTypeError(1,Basic(NonEmptyList(Some(Foo), Some(String)),NonEmptyList(Other, VarChar),List(),cats.free.Coyoneda$$anon$2@613d7c91),NoNulls,ColumnMeta(Other,"custom"."foo",NullableUnknown,foo)))

A similar query using the type in the default schema (public) doesn't trigger this error:

scala> val inPublic = sql"select 'Bar'::foo".query[Foo].analysis.transact(xa).unsafeRunSync
inPublic: doobie.util.analysis.Analysis = Analysis(select 'Bar'::foo,List(),List(Both((Basic(NonEmptyList(Some(Foo), Some(String)),NonEmptyList(Other, VarChar),List(),cats.free.Coyoneda$$anon$2@613d7c91),NoNulls),ColumnMeta(VarChar,foo,NullableUnknown,foo))))

scala> inPublic.columnTypeErrors
res36: List[doobie.util.analysis.ColumnTypeError] = List()

The difference seems to be caused by the JDBC type in the ColumnMeta. For inCustom it's Other, but for inPublic it's VarChar.

There is a possibility that this is caused by a driver bug rather then by a Doobie bug - but I don't have the time to dig deeper at the moment, and I need something to link to. :smile:

nigredo-tori commented 3 years ago

As a hacky workaround, I've added a wrapper like this:

final case class AnalysisWorkaround[A](
  value: A,
  tweak: Analysis => Analysis
)

, which tweaks the produced Analysis in its Analyzable instance. The tweak itself is to replace JdbcType.Other with JdbcType.VarChar in ColumnMetas where vendorTypeName matches the expected string ("$schemaName"."$typeName").

jatcwang commented 3 years ago

Yeah this seems like a JDBC driver bug. I've seen some issue in the past around pgjdbc and enums where enums from different schemas but with the same name are confused with one another. (Probably unrelated to your problem, but that area of the pgjdbc code is definitely...shady :grimacing: )

mkows commented 2 years ago

This seems to be resolved by now. The issue, as reported by @nigredo-tori, is no longer appearing. Tested with Postgres 14.2, doobie-core 1.0.0-RC2, doobie-postgres 1.0.0-RC2.

DB setup:

# with postgres running on port 5432:
createuser --createdb enum_user
createdb -O enum_user enum_db

psql -U enum_user enum_db

# inside psql:
enum_db=> create schema custom;
create type custom.foo as enum('Bar');
create type foo as enum('Bar');

in sbt console

// ...
// [info] welcome to sbt 1.7.1 ...
// ...
// Welcome to Scala 2.13.8 (OpenJDK 64-Bit Server VM, Java 17.0.2).

scala> import cats._
     | import cats.data._
     | import cats.effect._
     | import cats.implicits._
     | import doobie._
     | import doobie.implicits._
     | import doobie.postgres._
     | import doobie.postgres.implicits._
     |
     | import doobie.util.ExecutionContexts
     |
     | import cats.effect.unsafe.implicits.global
     |
import cats._
import cats.data._
import cats.effect._
import cats.implicits._
import doobie._
import doobie.implicits._
import doobie.postgres._
import doobie.postgres.implicits._
import doobie.util.ExecutionContexts
import cats.effect.unsafe.implicits.global

scala> val xa = Transactor.fromDriverManager[IO](
     |   "org.postgresql.Driver",
     |   "jdbc:postgresql://localhost:5432/enum_db",
     |   "enum_user",
     |   ""
     | )
val xa: doobie.util.transactor.Transactor.Aux[cats.effect.IO,Unit] = doobie.util.transactor$Transactor$$anon$13@7c8ec2d7

scala> object Foo extends Enumeration {
     |   val Bar = Value
     | }
     |
object Foo

scala> implicit val FooMeta = pgEnum(Foo, "foo")
val FooMeta: doobie.Meta[Foo.Value] = doobie.util.meta.Meta@31e15092

// no errors using public schema:
scala> val inCustom1 = sql"select 'Bar'::foo".query[Foo.Value].analysis.transact(xa).unsafeRunSync()
val inCustom1: doobie.util.analysis.Analysis = Analysis(select 'Bar'::foo,List(),List(Both((Basic(NonEmptyList(Some(e.Value), Some(String)),NonEmptyList(Other, VarChar),List(),cats.free.Coyoneda$$anon$2@295ee292),NoNulls),ColumnMeta(VarChar,foo,NullableUnknown,foo))))

scala> inCustom1.columnTypeErrors
val res1: List[doobie.util.analysis.ColumnTypeError] = List()

// no errors using custom schema
scala> val inCustom2 = sql"select 'Bar'::custom.foo".query[Foo.Value].analysis.transact(xa).unsafeRunSync()
val inCustom2: doobie.util.analysis.Analysis = Analysis(select 'Bar'::custom.foo,List(),List(Both((Basic(NonEmptyList(Some(e.Value), Some(String)),NonEmptyList(Other, VarChar),List(),cats.free.Coyoneda$$anon$2@295ee292),NoNulls),ColumnMeta(VarChar,"custom"."foo",NullableUnknown,foo))))

scala> inCustom2.columnTypeErrors
val res2: List[doobie.util.analysis.ColumnTypeError] = List()

Comparing analysis results side-by-side:

// before:
Analysis(select 'Bar'::custom.foo,List(),List(Both((Basic(NonEmptyList(Some(Foo),     Some(String)),NonEmptyList(Other, VarChar),List(),cats.free.Coyoneda$$anon$2@613d7c91),NoNulls),ColumnMeta(Other,"custom"."foo",NullableUnknown,foo))))

// now:
Analysis(SELECT 'Bar'::custom.foo,List(),List(Both((Basic(NonEmptyList(Some(e.Value), Some(String)),NonEmptyList(Other, VarChar),List(),cats.free.Coyoneda$$anon$2@5a1c6d94),NoNulls),ColumnMeta(VarChar,"custom"."foo",NullableUnknown,foo))))

So now column meta is no longer Other but VarChar.

@jatcwang IMO this issue can be closed as per the above.

jatcwang commented 7 months ago

Thanks you for the detailed report @mkows 🙏

jatcwang commented 7 months ago

Reopening as I want to make sure there's a test for this