Skip to content

Loading…

Add SQL Field name to ResultError #40

Closed
wants to merge 2 commits into from

2 participants

@jochu

This adds the sql field name to fromField's error messages. This is useful information when debugging unexpected parse errors.

Please let me know if you need me make any changes.

@lpsmith
Owner

Two comments:

  1. Would it be better to keep the errSQLField a Maybe String, or should it be a String instead? The latter would be more in keeping with the spirit of the current ResultError, at least.

  2. Would it be helpful to also include the table name of the column? I don't have this functionality available at the moment, but it should be possible... At the very least, I'm pretty sure that libpq will give you the Object ID of the table that the column comes from, which could then be turned into a table name by querying the meta-schema.

The one potential problem here is that converting the Table OID into a table name is an IO operation, but the fromField method is pure. Converting the Type OID to a typename is also an IO operation, but this is hidden by the fact that postgresql-simple pre-computes the type name and includes it in the opaque Field value. We could do something similar for table names, but I would start to worry about doing too much work when the results are likely to be ignored.

Perhaps the fromField method needs to become an IO computation, or a restricted monad that allows certain IO computations such as fetching the type name and table name. In any case, maybe this is an issue for another day.

@jochu
  1. Yeah, I thought about doing that as well. But I went the safe route and leaned toward the more "correct" approach. But since the same thought crossed your mind, I'll go ahead and change it to be "" when it's unavailable.
  2. I wanted to include that as well - but I came across the same issues you raised with converting a Table OID to a table name. I figured that having a column name is better than nothing, so I simply left it out.

Do you think it'd be worthwhile to include the Table OID in ResultError as well? That way, people can look it up themselves if they find they need it. It's not ideal, but it might be better than nothing. If so, my thoughts are having it as errSQLTable :: String - which allows making it a table name in the future without needing to change this type.

@lpsmith
Owner

Regarding including the table OID, I suppose that's up to you. I really think that I'm going to have to change the type of the fromField method to allow it to take certain kinds of IO actions. I don't want to keep adding more work (and allocating memory) when there is a really good chance everything's going to get ignored anyway. So yeah, the changes needed to have the table name available definitely feel out of scope at the moment.

I don't think that storing the table Oid as a string is such a hot idea; this way when you change the semantics you are changing the type as well. (Or, perhaps it would make sense to simply add another field, once the table name becomes available.)

On the other hand, I'm not sure what the public interface of these exceptions is, either. I mean, most (but not all) uses of exceptions are simply to print out an error message, but not always. For example, withTransactionModeRetry checks the sqlState field of a SqlError exception to determine whether or not the transaction failed due to a serialization error, and if so, retries the transaction instead of re-throwing the exception. And I have no idea if people are constructing their own error values; if this is considered part of the public API, then adding fields also requires a major version bump.

@jochu

I don't think that storing the table Oid as a string is such a hot idea; this way when you change the semantics you are changing the type as well. (Or, perhaps it would make sense to simply add another field, once the table name becomes available.)

That's a good point. In which case, I'd lean toward the latter of adding another field and making the OID field errSQLTableOid :: ?. Although I'm not sure if it should be a String or Maybe Oid, what do you think?

Personally, I'd like to add the OID somewhere as a stop gap measure - because it's useful information to me and I don't mind the extra manual step. It also doesn't prevent us from adding table name in the future (whether that's accomplished via pre-loading or moving fromField into IO).

@lpsmith
Owner

Ok, I pulled your work into my 0.3 branch, and added you as a contributor.

I'd be happy to have the table OID as part of the error message, and it should definitely be a Maybe Oid, not a string. Though going through and documenting the FromField module made me realize that the ftable binding to libpq should probably return a Maybe Oid, not a plain Oid, which is probably a mistake that has carried over from when I took the bindings over myself. And that mistake cascaded from postgresql-libpq to postgresql-simple.

However I doubt that many people are using tableOid, and I am bumping a major version component, so...

@jochu

Thanks! I've added table oid to a different pull request based off your 0.3 branch.

@lpsmith lpsmith closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Sep 26, 2012
  1. @jochu

    Add sql field name ResultError

    jochu committed
  2. @jochu
View
3 src/Database/PostgreSQL/Simple.hs
@@ -568,10 +568,11 @@ finishQuery conn q result = do
, fmap ellipsis v )
throw (ConversionFailed
(show (unCol ncols) ++ " values: " ++ show vals)
+ ""
(show (unCol col) ++ " slots in target type")
"mismatch between number of columns to \
\convert and number in target type")
- Errors [] -> throwIO $ ConversionFailed "" "" "unknown error"
+ Errors [] -> throwIO $ ConversionFailed "" "" "" "unknown error"
Errors [x] -> throwIO x
Errors xs -> throwIO $ ManyErrors xs
PQ.CopyOut ->
View
12 src/Database/PostgreSQL/Simple/FromField.hs
@@ -74,15 +74,18 @@ import qualified Data.Text.Lazy as LT
-- | Exception thrown if conversion from a SQL value to a Haskell
-- value fails.
data ResultError = Incompatible { errSQLType :: String
+ , errSQLField :: String
, errHaskellType :: String
, errMessage :: String }
-- ^ The SQL and Haskell types are not compatible.
| UnexpectedNull { errSQLType :: String
+ , errSQLField :: String
, errHaskellType :: String
, errMessage :: String }
-- ^ A SQL @NULL@ was encountered when the Haskell
-- type did not permit it.
| ConversionFailed { errSQLType :: String
+ , errSQLField :: String
, errHaskellType :: String
, errMessage :: String }
-- ^ The SQL value could not be parsed, or could not
@@ -228,12 +231,12 @@ ff :: BuiltinType -> String -> (B8.ByteString -> Either String a)
-> Field -> Maybe B8.ByteString -> Ok a
ff pgType hsType parse f mstr
| typeOid f /= builtin2oid pgType
- = left (Incompatible (B8.unpack (typename f)) hsType "")
+ = left (Incompatible (B8.unpack (typename f)) (maybe "" B8.unpack (name f)) hsType "")
| Nothing <- mstr
- = left (UnexpectedNull (B8.unpack (typename f)) hsType "")
+ = left (UnexpectedNull (B8.unpack (typename f)) (maybe "" B8.unpack (name f)) hsType "")
| Just str <- mstr
= case parse str of
- Left msg -> left (ConversionFailed (B8.unpack (typename f)) hsType msg)
+ Left msg -> left (ConversionFailed (B8.unpack (typename f)) (maybe "" B8.unpack (name f)) hsType msg)
Right val -> return val
{-# INLINE ff #-}
@@ -281,9 +284,10 @@ doFromField f _ _ _ = returnError UnexpectedNull f ""
-- exception value and returns it in a 'Left . SomeException'
-- constructor.
returnError :: forall a err . (Typeable a, Exception err)
- => (String -> String -> String -> err)
+ => (String -> String -> String -> String -> err)
-> Field -> String -> Ok a
returnError mkErr f = left . mkErr (B.unpack (typename f))
+ (maybe "" B.unpack (name f))
(show (typeOf (undefined :: a)))
atto :: forall a. (Typeable a)
View
1 src/Database/PostgreSQL/Simple/FromRow.hs
@@ -79,6 +79,7 @@ fieldWith fieldP = RP $ do
[0..ncols-1]
convertError = ConversionFailed
(show (unCol ncols) ++ " values: " ++ show vals)
+ ""
("at least " ++ show (unCol column + 1)
++ " slots in target type")
"mismatch between number of columns to \
Something went wrong with that request. Please try again.