lpsmith / postgresql-simple

Mid-level client library for accessing PostgreSQL from Haskell
Other
206 stars 71 forks source link

Fails to read from an hstore column when there is a null value for one of the keys #204

Open sras opened 7 years ago

sras commented 7 years ago

When I try to read from an hstore column into a HStoreList or HStoreMap, the following error gets thrown.

*** Exception: ConversionFailed {errSQLType = "hstore", errSQLTableOid = Just (Oid 26693), errSQLField = "properties", errHaskellType = "HStoreList", errMessage = "endOfInput"}

This is the code

import Database.PostgreSQL.Simple
import Database.PostgreSQL.Simple.HStore

getUserRowsPG :: IO [(Int, String, String, HStoreList)]
getUserRowsPG = do
  conn <- getConn
  res <- PGS.query_ conn "select * from users"
  return res

Table structure

   Column   |  Type   |                     Modifiers
------------+---------+----------------------------------------------------
 id         | integer | not null default nextval('users_id_seq'::regclass)
 name       | text    |
 email      | text    | not null default 'a@a.com'::text
 properties | hstore  | not nul

The offending row is

id | name | email | properties ----+--------+-----------+---------------------- 1 | abcd | s@a.com | "name"=>NULL

lpsmith commented 7 years ago

Yes, the inability to deal with NULL hstore values is a shortcoming in the HStore module. And unfortunately I chose the wrong API, and I'm not sure exactly to what degree we are stuck with it.

You don't have to use the HStore module; you can add your own support for HStore to your program without modifying postgresql-simple. Though I would like to fix it, but I don't know to what degree I should preserve the existing interface, as I have no idea how common the use of hstore is.

Another option which may or may not be suitable for your purposes is to consider using jsonb instead.

saurabhnanda commented 7 years ago

@lpsmith general feedback - can the error/exception type be extended to give the exact column/value which caused the conversion error ALONG WITH the row that was being converted? There have been other instances where I've been stuck with conversion failures due to NULL and tracking down the exact row takes longer than it should.

Would you be open to a PR which add a deprecation notice to the current HStore API and adds a new module called Database.PostgreSQL.Simple.HStoreV2 or something?

saurabhnanda commented 7 years ago

Btw, we also have https://www.stackage.org/haddock/lts-7.14/postgresql-simple-0.5.2.1/Database-PostgreSQL-Simple-HStore.html#t:HStoreMap -- will this also error on keys that have NULL values, or will it simply not insert them in the map?

saurabhnanda commented 7 years ago

It seems that HStoreMap also errors out on keys with NULL values. Can we just fix this? Would anyone be depending on this behaviour?

lpsmith commented 7 years ago

The HStoreMap will also error on NULL values inside the HStore.

Hmm, are the keys in an hstore allowed to be null as well? It appears the answer is no:

$ psql
psql (9.6.1, server 9.4.10)
Type "help" for help.

lpsmith=> select 'NULL=>"hello"'::hstore;
     hstore      
-----------------
 "NULL"=>"hello"
(1 row)

lpsmith=> select 'NULL=>NULL'::hstore;
    hstore    
--------------
 "NULL"=>NULL
(1 row)

As for both of your suggestions, I would be very receptive to them, and it shouldn't be overly difficult to implement. My only question is, what do you mean when you say "the row that was being converted"?

lpsmith commented 7 years ago

Oh, I didn't see @saurabhnanda's third post on this issue. I can't say I'm overly enthusaistic about that change; the behavior seems a little obscure, and I prefer to avoid those kinds of things. If you want that behavior, I'd rather you be explicit about it, either by tweaking your SQL to filter hstore pairs with null values, or by filtering it yourself from a HStoreV2 result.

saurabhnanda commented 7 years ago

I didn't mean NULL keys, but keys having null values, ie the value is NULL, not the key.

Given that context, what do you think about a HashMap which simply skips the key-value pairs that have a NULL value? There is loss of fidelity here, because a missing key cannot be distinguished from a key with a NULL value. (btw can a HashMap distinguish between these cases in the first place?)

Also, if a FromField conversion fails at runtime due to any error, it's very helpful getting the entire row that the field belonged to.

lpsmith commented 7 years ago

Right, I have understood for a while now that NULL values inside an hstore is possible, and that postgresql-simple's HStore module couldn't accomodate them. This is definitely a real issue I was aware of, but you are the first to bring it up. But, with an eye to revising the API, I was curious if we were overlooking another feature of the hstore data model. (We aren't)

The default conversions try to maintain fidelity as best as possible. Maybe we could change both HStoreList and HStoreMap to filter out NULL keys, maybe I'm softening on that idea a bit, but I'm definitely against the idea of changing one and not the other, and I'm still not overly enthusiastic about it.

I am definitely interested in a proper fix to the interface, however, possibly as a new HStoreV2 module.

lpsmith commented 7 years ago

Keep in mind I still want to fix this issue, but here's one possible workaround if you simply want to remove elements from the hstore with null values:

create function filter_not_null(hstore) returns hstore as $$
  select hstore(array_agg(key),array_agg(value)) from each($1) where value is not null 
$$ language 'sql' strict immutable;

Then you can apply this function as needed, e.g.

select id, name, email, filter_not_null(properties) from users;
saurabhnanda commented 7 years ago

@lpsmith we have a working version in our internal repo and want to raise a PR. Is there a contributor's guide? A simple stack build didn't work after cloning the repo.