Skip to content
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

'Invalid syntax' generated with negative years #380

Open
ysangkok opened this issue Oct 5, 2024 · 1 comment
Open

'Invalid syntax' generated with negative years #380

ysangkok opened this issue Oct 5, 2024 · 1 comment
Assignees

Comments

@ysangkok
Copy link
Contributor

ysangkok commented Oct 5, 2024

I noticed that if you try to insert a UTCTime with a negative year, it fails even though PostgreSQL does handle this case. It seems that we are generating invalid syntax.

On PostgresSQL 15, the error is:

SqlExecutionError
  { sqlExecutionErrorExecStatus = Just FatalError
  , sqlExecutionErrorMessage = "ERROR:  invalid input syntax for type timestamp with time zone: \"-0001-01-01 00:00:00+00\"\n"
  , sqlExecutionErrorSqlState = Just "22007"
  , sqlExecutionErrorSqlQuery = "INSERT INTO \"foo\" (\"id\",\"name\",\"birth\") VALUES ($1,$2,$3)"
  }
Reproduction program
import qualified Orville.PostgreSQL as O
import qualified Orville.PostgreSQL.AutoMigration as AutoMigration

import qualified Data.Int as Int
import qualified Data.Time as Time
import qualified Data.Text as T
type FooId = Int.Int32
type FooName = T.Text
type FooBirth = Time.UTCTime

data Foo = Foo
  { fooId :: FooId
  , fooName :: FooName
  , fooBirth :: FooBirth
  }
  deriving (Eq, Show)
fooIdField :: O.FieldDefinition O.NotNull FooId
fooIdField =
  O.integerField "id"

fooNameField :: O.FieldDefinition O.NotNull FooName
fooNameField =
  O.unboundedTextField "name"

fooBirthField :: O.FieldDefinition O.NotNull FooBirth
fooBirthField =
  O.utcTimestampField "birth"
fooMarshaller :: O.SqlMarshaller Foo Foo
fooMarshaller =
  Foo
    <$> O.marshallField fooId fooIdField
    <*> O.marshallField fooName fooNameField
    <*> O.marshallField fooBirth fooBirthField
table :: O.TableDefinition (O.HasKey FooId) Foo Foo
table =
  O.mkTableDefinition "foo" (O.primaryKey fooIdField) fooMarshaller
main :: IO ()
main = do
  pool <-
    O.createConnectionPool
        O.ConnectionOptions
          { O.connectionString = ""
          , O.connectionNoticeReporting = O.DisableNoticeReporting
          , O.connectionPoolStripes = O.OneStripePerCapability
          , O.connectionPoolLingerTime = 10
          , O.connectionPoolMaxConnections = O.MaxConnectionsPerStripe 1
          }

  mFoo <- O.runOrville pool $ do
    AutoMigration.autoMigrateSchema AutoMigration.defaultOptions [AutoMigration.SchemaTable table]
    _ <- O.deleteEntity table 0
    _ <- O.insertEntity table Foo { fooId = 0, fooName = T.pack "Name", fooBirth = Time.UTCTime (Time.fromGregorian (-1) 1 1) 0 }
    O.findEntity table 0
  print mFoo

I don't think we can actually map all UTCTimes to SQL, since PostgreSQL has a limit on the year, but the time library uses an Integer, which is unbounded.

One way to improve handling of UTCTime specifically, might be to use the function make_timestamptz(year, month, ...), which explicitly states in its documentation that it supports negative years. Or maybe we just don't wanna support those? Either way, I think the choice should be documented.

So I propose we use Hedgehog to run property tests for all sqlTypes, to see which ones round trip and which ones don't. Then, we can attach documentation to the relevant SqlTypes and FieldDefinitions, with a comment warning that they are partial and will work for only a certain range. We can also test whether the ordering in SQL works the same as in Haskell.

@jlavelle
Copy link
Member

jlavelle commented Oct 8, 2024

Great catch! I don't think there is a straightforward way to use make_timestamptz here because we can't put a function call in parameter. Instead we should remove the leading - and append BC to the timestamp, since negative years represent B.C.E. years in PostgreSQL.

It is unfortunate that we can't encode all Haskell UTCTimes in SQL. The current implementation requires converting a Haskell value to a SqlValue to be infallible, i.e. we need a function UTCTime -> SqlValue, as opposed to decoding where we can say SqlValue -> Either String UTCTime.

We could introduce a newtype wrapper and conversion functions around UTCTime that restricts the timestamp to the range allowed by PostgreSQL. That would push the range validation of the UTCTime out to the caller.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants