JSON in types #46
JSON in types #46
Conversation
clhodapp
force-pushed
the
feature/json-in-types
branch
2 times, most recently
from
5c3a0ad to
de29d19
Compare
clhodapp
force-pushed
the
feature/json-in-types
branch
3 times, most recently
from
1b0d55f to
00e077d
Compare
| @@ -69,7 +69,7 @@ object PostgresClient { | |||
|
|
|||
| case class TypeSpecifier(receiveFunction: String, typeName: String, elemOid: Long = 0) | |||
|
|
|||
| private[finagle] val defaultTypes = Map( | |||
| val defaultTypes = Map( | |||
There was a problem hiding this comment.
Why does it need to be public?
There was a problem hiding this comment.
I'm making it public to make things slightly easier for the next person who finds that one of the types they use isn't part of defaultTypes and who doesn't want to use the fatally-bugged dynamic-type-map-building feature. Right now, if you want to use withCustomTypes, chances are that you'll have to copy this definition of defaultTypes into your code so you can have a Map that adds the mappings you need to pass to withCustomTypes.
There was a problem hiding this comment.
(I say it's "fatally-bugged" because the Future here caches any failure to pull the typeMap at startup forever, thus borking the client)
There was a problem hiding this comment.
Reading these comments again, I think my tone may be slightly abrasive. Sorry about that!
| @@ -83,6 +83,7 @@ object PostgresClient { | |||
| Types.TID -> TypeSpecifier("tidrecv", "tid"), | |||
| Types.XID -> TypeSpecifier("xidrecv", "xid"), | |||
| Types.CID -> TypeSpecifier("cidrecv", "cid"), | |||
| Types.JSON -> TypeSpecifier("json_recv", "json"), | |||
There was a problem hiding this comment.
In general I've tried to only include types in the default map that are baked-in to postgres without installing an extension. I think that's now the case for json so this should be OK. Can you confirm?
There was a problem hiding this comment.
JSON was added as a baked-in feature in Postgres 9.2 (released in 2012, so it's pretty likely that most people are on at least that version at this point).
clhodapp
force-pushed
the
feature/json-in-types
branch
from
00e077d to
4ac382f
Compare
|
(Rebased to fix merge conflict) |
clhodapp
force-pushed
the
feature/json-in-types
branch
from
4ac382f to
b28ecd9
Compare
clhodapp
force-pushed
the
feature/json-in-types
branch
2 times, most recently
from
9b1ed39 to
bbd43d5
Compare
|
@clhodapp I'm sorry for the lack of attention. If you rebase this I'd give it a 👍 (though we should also add some retries to |
|
(fixed conflicts and this now builds but it needs a rebase and I don't know how to do that to somebody else's PR) |
clhodapp
force-pushed
the
feature/json-in-types
branch
from
1080a2b to
4e23133
Compare
|
Finally rebased this. Sorry for the delay. It looks like some other JSON(B)-related work already made its way onto master. I think that this is still needed, though. |
Adds a mapping for json to default type mappings and makes them public. Right now, I've had to duplicate the whole thing into my code, add json to it, and use
withCustomTypes. Why not letfinagle-postgresquery the database for the types at startup? Well.. right now there's a bug such that failures to pull the type-map at startup get memoized in aFuture, causing your client to be dead forever (which I intend to fix in a subsequent pull request).