mongoDB generated toJSON seems wrong #136

Closed
Zortaniac opened this Issue Jun 28, 2013 · 24 comments

4 participants

@Zortaniac

In the auto generated toJSON methods semms to be an issue in the EntityId key translation.
The generated object id key is of the form oUcnvZM+mRm1HAAAA and not as expected as in the form of 51c9ef64cfa6466d47000000.

Related to my post at the Google Yesod group:
https://groups.google.com/forum/#!topic/yesodweb/AP1z4ov4RNk

@gregwebs
Yesod Web Framework member

at the moment you can invoke toPathPiece on a Key to get hex text.

@gregwebs gregwebs closed this in 3b219ae Jun 30, 2013
@gregwebs
Yesod Web Framework member

I just pushed a commit to master to fix this. Let me know if you try it out, otherwise I will write a test case and release it tomorrow.

@Zortaniac

I've tried it out but now i getting this error:

Model.hs:33:1:
Overlapping instances for ToJSON (KeyBackend MongoBackend User)
arising from a use of .='
Matching instances:
instance ToJSON (KeyBackend backend entity)
-- Defined in
persistent-1.2.1:Database.Persist.Types.Base'
instance ToJSON (KeyBackend MongoBackend entity)
-- Defined in Database.Persist.MongoDB'
In the expression: ((pack "user") .= user_a81r)
In the first argument of
object', namely
`[((pack "name") .= name_a81l),
((pack "shortDescription") .= shortDescription_a81m),
((pack "description") .= description_a81n),
((pack "persons") .= persons_a81o), ....]'
In the expression:
object
[((pack "name") .= name_a81l),
((pack "shortDescription") .= shortDescription_a81m),
((pack "description") .= description_a81n),
((pack "persons") .= persons_a81o), ....]

It seems not working for relations. In this example user is an relation.

@gregwebs
Yesod Web Framework member

thanks, I was worried about the overlapping issue, but I thought it would show at compile time. It looks like I will have to make a slightly more invasive change.

@gregwebs gregwebs reopened this Jun 30, 2013
@gregwebs
Yesod Web Framework member

@snoyberg PersistObjectId is specifically for MongoDB, but it Aeson instances are in persistent modules where it would be bad to import any mongoDB dependencies. Should I try to copy over the needed code or is there a way to make this work by just making changes in persistent-mongoDB ?

@snoyberg
Yesod Web Framework member

I think you'd need to copy the serialization code into persistent.

@gregwebs
Yesod Web Framework member

How about changing the type to: PersistObjectId ByteString Text ?

persistent-mongoDB will fill up the Text.
The Text piece can potentially stay lazy even so the conversion effort need not take place normally.

Downside is that it is a breaking change, but that seems better than adding dependencies or trying to copy and paste code from different levels of dependencies.

@snoyberg
Yesod Web Framework member

I'd rather not have a breaking change for this. Is it really that difficult to include MongoDB Object ID parse and render code? It sounds pretty trivial to me.

@gregwebs gregwebs closed this in d388e38 Jul 5, 2013
@gregwebs
Yesod Web Framework member

@Zortaniac let me know if it works for you now. I did test it out myself this time, but only on a single case (although I wrote a quickcheck property for the JSON roundtrip.)

@Zortaniac

I've tested the new code.
Now it looks almost right except of an lowercased 'o' in front of the ObjectId "key":"o51c9ef64cfa6466d47000000".

@gregwebs
Yesod Web Framework member

I believe that is the expected behavior. @snoyberg I am not a fan of the type prefixes, maybe I am doing something wrong or maybe we can figure out how to get rid of them.

@snoyberg
Yesod Web Framework member

If you know of a way to rewrite the library that the type prefixes aren't necessary, let me know. But if this is appearing in the JSON output, it sounds like something fishy is happening.

@gregwebs
Yesod Web Framework member

where do the type prefixes get dropped then or is some different JSON generated in persistent-th?

@gregwebs gregwebs reopened this Jul 8, 2013
@snoyberg
Yesod Web Framework member

I'm sorry, I wasn't thinking this through correctly. The type prefixes are necessary for properly turning JSON data back into a Haskell data type. For example, without them, how would you know if "12345" should be text, byte string, or object ID?

@Zortaniac

@snoyberg I'm disagree. You can't now it. You have to assume that the data you get is in the right type. The probleme is. If you have for example an page wich loads a big set of JSON data you have to remove the type prefix to use it for a new API call but as we now JavaScript is not as that fast as other languages.
By the way in the routing we expectet the right data too and return a 404 if the data is not as expected. If you transform the JSON back into a Haskell type you now wich data type is expected and you can send an 400 or something like that if the request data is wrong.

@snoyberg
Yesod Web Framework member

I think you're not understanding the concern I raised. Suppose there were no type prefixes, and you saw the following JSON data:

{"foo":"12345"}

What Haskell value should that be serialized into? Now consider the following Haskell values:

PersistText "12345"
PersistByteString "12345"
PersistObjectId "12345"

What would be their JSON representations? Can you ensure round-tripping?

@Zortaniac

Yes you are right. I missed that a type in Haskell can inherit from multiple types. You mean something like this

data FooType = PersistText | PersistByteString | PersistObjectId 

right?
So you need the type prefix but it is not really satisfying.

@gregwebs
Yesod Web Framework member

So if this is the expected behavior we should probably close this issue and open a new one, although I should probably deal with the 404 issue right now.

The type prefix is necessary if you are slinging around raw pieces of persistent data without knowledge of a schema. But if we have pairs (key-values in a JSON object) of names to data where the name is for a persistent record field of a known type then we should be able to generate To/From JSON that can switch on the field name.

So I think the answer to Michael's question is: the type of the foo value is whatever the Persistent type for foo is given that we are trying to convert the JSON into a Bar record.

@gregwebs
Yesod Web Framework member

There is a big problem with type-prefixes as is: it is now impossible to make changes in a backwards compatible way or otherwise truly know if there is no type prefix but there is an accidental match. The type really needs to be separate from the value. Rather than {foo:"t1234"} it needs to be {foo:{"t":"1234"}} or {foo:["t","1234"]}.

@gregwebs
Yesod Web Framework member

I moved the JSON type prefixes discussion to a new github issue #139

@Zortaniac

Yes this issue can be closed. My initial bug is fixed now.

@gregwebs Nice idea. I thinked on splitting the prefix from the value too. Sadly JSON knows no tuples but an array should be ok too.

@gregwebs
Yesod Web Framework member

I just released persistent-mongoDB-1.2.1.1 with the routing fix and persistent-1.2.1.2 which has the JSON fixes

@gregwebs gregwebs closed this Jul 8, 2013
@maxcan

So this issue has come to bite me in the ass in terms of interoperating with NodeJS and front end javascript code. If we're going to be generating JSON, I think we should make every effort to support idiomatic usage.

While its true that type prefixes are needed because of the PersistValue ADT, it is also the case that in the generated record types from Persistent, the user basically never sees any types besides PersistObjectIds. In other words, a model like

Foo
   val Text

Bar
   foo FooId
   val2  Int

The generated record types will take a Text and a PersistObjectId and Int respectively. So, I propose that we modify the prefixes slightly. Change all of the prefixes that aren't for PersistObjectId to non-hex characters and then drop the 'o' prefix for ObjectIds

This wouldn't be backwards compatible but it would give us idiomatic behavior in pretty much any situation I can imagine.

@gregwebs
Yesod Web Framework member

I am opening a new #170 to fully address prefixing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment