JSON character prefix type information #170

Closed
gregwebs opened this Issue Oct 7, 2013 · 21 comments

Projects

None yet

3 participants

@gregwebs
Yesod Web Framework member

see #136 and #168

I would like to solve this issue for more than just this use case. It is true that we need to retain type information to the point of the conversion. However, there are multiple ways to carry that information, and there are big drawbacks to character prefixing.

I think a big improvement would be something like {"type":"Int","value":2}

An alternative is to send a separate schema object with data. That could be more efficient but could be more error prone.

@maxcan

But then can you parse data without the schema? I'd guess not so we still lose interoperability with the rest of the world. What are your thoughts about my point that it the only constructor of PersistValue that matters is PersistObjectId?

@gregwebs
Yesod Web Framework member

So is the main mistake is that PersistObjectId is being leaked? But is that not also the case for PersistInt64 for Sql users and potentially something else for future users of a different database?

@gregwebs
Yesod Web Framework member

oh, PersistInt64 is a Number so it doesn't have this issue because String is the only overloaded JSON type.

@gregwebs
Yesod Web Framework member

So should unKey ideally be a function that unwraps the PersistValue so that the a Key does not expose a user to a PersistValue?

One note on implementation that may be related is that I would like to remove PersistObjectId and instead just use PersistDbSpecific if possible.

@maxcan

Ok, so that's getting deeper into persistent than I can intelligently comment on. Another solution to this would be, as you suggest, to stop the PersistObjectIds from leaking out.

@gregwebs
Yesod Web Framework member

yeah, so that is what I am proposing. The existing type is:

newtype KeyBackend backend entity = Key { unKey  PersistValue }
type Key val = KeyBackend (PersistEntityBackend val) val

@snoyberg does it make sense to have functions to be used in the place of unKey (or possibly change the type of unKey) such that the user will not get back a PersistValue from a Key? I have a helper in MongoDB already that gives back the native DB type instead.

keyToOid  (PersistEntity entity, PersistEntityBackend entity ~ MongoBackend)
          Key entity  DB.ObjectId
keyToOid (Key k) = persistObjectIdToDbOid k
@gregwebs
Yesod Web Framework member

I don't think I really have any usage for a PersistObjectId. I either want a Key or the underlying MongoDB data type (DB.ObjectId from above). Is there a desire for users to deal with PersistInt64 ?

@gregwebs
Yesod Web Framework member

I am wondering if now is the time to take a look at composite keys.

@maxcan
@gregwebs
Yesod Web Framework member

SQL users want composite keys, which may mean another API breakage to Key

@snoyberg
Yesod Web Framework member

Sorry, I haven't had a chance to look at this issue yet. I will try to get to it next week.

@gregwebs
Yesod Web Framework member

no problem, I need a little more time still to look at this in greater depth.

@gregwebs
Yesod Web Framework member

I guess what this is getting at is that we give back a Haskell record, not [PersistValue] for the record. What is stopping us from giving back a Haskell value for the key? Actually, we need to support composite fields, so we could give back a Haskell record, but the global record fields issue doesn't allow for that. But key fields are few and can remain unnamed so we can just give back a constructor of Haskell values. It does require a different constructor for each record, but I think KeyPerson 5 is better than Key $ PersistInt64 5. Note that key construction now becomes type-safe, whereas right now someone can stick whatever PersistValue they want into a key.

The only problem with this is the backend parameterization. I think we just need how to figure out how to make it work for the one database a user is actually using and we can later figure out how to make it possible to have keys for different backends when someone needs that.

@snoyberg
Yesod Web Framework member

If we're willing to give up on datatypes that work across multiple backends, then your approach makes perfect sense. But the whole purpose in using PersistValue in the first place was because of the multi-backend issue. I doubt anything's changed in such a way that it makes sense to change key handling.

@gregwebs
Yesod Web Framework member

Perhaps we can still have a backend agnostic type underneath. personKey = Key . PersistInt64. Rather than exporting the Key constructor we could have functions to retrieve the Haskell value. Not sure what to call that, perhaps fromPersonKey. We could have more general less type-safe functions such as keyInt.

@snoyberg
Yesod Web Framework member

Are you talking about having smart constructors and extractors? I'm not necessarily opposed, but I'm not sure what the motivation is here.

@gregwebs
Yesod Web Framework member

This issue was opened up because we are exposing a PersistValue (PersistObjectId) which gets converted into JSON that was only meant for the database. After thinking about this I realized that PersistValue is an internal persistent implementation detail that a user should never be exposed to in usage. It should only be exposed for the purpose of defining a PersistField.

I am also trying to kepp in mind the possibility of increasing type safety and handling composite keys.

@gregwebs
Yesod Web Framework member

With composite keys or a primary key of a record field, the key type is dependent on the record. This by itself would be easy to make more type-safe, but there is also the option of the auto-generated id key depending on the backend.

Does this express what we want?

data Family KeyBackend :: * -> *
instance Family KeyBackend MongoBackend User  = DB.Oid -- auto-generated primary key
instance Family KeyBackend SqlBackend   User  = Int    -- auto-generated primary key
instance Family KeyBackend SqlBackend   User1 = Text -- primary key on e-mail
instance Family KeyBackend SqlBackend   User3 = PersistValue -- composite key with multiple types, need to use PersistList or a generated constructor
@gregwebs
Yesod Web Framework member

work is under way for a better Key type on #186

@gregwebs gregwebs added the 2.0 label Jun 11, 2014
@gregwebs
Yesod Web Framework member

The change has been made on the persistent2 branch so that the Key is not just a PersistValue wrapper.

@gregwebs gregwebs closed this Aug 25, 2014
@gregwebs
Yesod Web Framework member

Note that we did not fix the JSON character prefix, but instead fixed where it leaked out in JSON generation for MongoDB. It should now only be visible as serialized JSON in the database (for SQL users). Changing how type prefixes operate is a backwards-incompatible change that would be good, as a MongoDB user I will leave it up to the SQL users to do that.

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