Zaid-Ajaj / LiteDB.FSharp

Advanced F# Support for LiteDB, an embedded NoSql database for .NET with type-safe query expression through F# quotations
MIT License
180 stars 22 forks source link

Single Case Discriminated Unions as Identifiers #41

Closed aspnetde closed 4 years ago

aspnetde commented 4 years ago

I am just playing around with the library and it works great so far. In the concrete use case I am considering to use it, however, we are using single case discriminated unions for ids quite heavily. The good news: Inserting records which use a DU as id works.

But I noticed that querying for a record that has just been inserted does not work. Also indexing isn't working. Sample:

open System
open System.IO
open LiteDB
open LiteDB.FSharp

type FooId = FooId of Guid

type Foo = 
    { Id : FooId
      ParentId : FooId option }

[<EntryPoint>]
let main argv =
    let mapper = FSharpBsonMapper()
    let path = Path.Combine(Directory.GetCurrentDirectory(), "test.db")
    use db = new LiteDatabase(path, mapper)

    let id = Guid.NewGuid()

    let demoFoo =
      { Id = FooId(id)
        ParentId = None }

    let collection = db.GetCollection<Foo>()

    collection.Insert(demoFoo) |> ignore
    let indexEnsured = collection.EnsureIndex(fun x -> x.Id)
    printfn "Index ensured: %b" indexEnsured

    let allFoos = collection.FindAll() |> Seq.toList
    printfn "Total: %i" allFoos.Length

    let result = collection.FindById(BsonValue(id))
    printfn "Last one: %s" (result.Id.ToString())
    0

collection.FindById(BsonValue(id)) will just return null, indexEnsured is false.

When removing the DU and using theGuid directly, everything works as expected.

open System
open System.IO
open LiteDB
open LiteDB.FSharp

type FooId = FooId of Guid

type Foo = 
    { Id : Guid
      ParentId : FooId option }

[<EntryPoint>]
let main argv =
    let mapper = FSharpBsonMapper()
    let path = Path.Combine(Directory.GetCurrentDirectory(), "test.db")
    use db = new LiteDatabase(path, mapper)

    let id = Guid.NewGuid()

    let demoFoo =
      { Id = id
        ParentId = None }

    let collection = db.GetCollection<Foo>()

    collection.Insert(demoFoo) |> ignore
    let indexEnsured = collection.EnsureIndex(fun x -> x.Id)
    printfn "Index ensured: %b" indexEnsured

    let allFoos = collection.FindAll() |> Seq.toList
    printfn "Total: %i" allFoos.Length

    let result = collection.FindById(BsonValue(id))
    printfn "Last one: %s" (result.Id.ToString())
    0

Do you think there is a way to teach the mapper how to handle DU's for such a case?

Thomas

Zaid-Ajaj commented 4 years ago

Hello Thomas, Thanks a lot for the detailed issue! It should be possible to tell the mapper that single case unions are specials and that the union should be erased (maybe an option for the mapper?), both when serializing and deserializing. However, if we were to implement the feature, then it must be backwards compatible.

Do you want to give it a try? PRs are always welcome :smile: I can give it a try myself but right now I am focusing on other libraries so it might take a while

aspnetde commented 4 years ago

Fair enough πŸ˜„

I am not sure my F# knowledge is advanced enough for solving this in sufficient time. But I could try to find some spare time to look into it.

Do you have some hints on where to start?

Zaid-Ajaj commented 4 years ago

Do you have some hints on where to start?

This part is for serializing the value and this part is for deserializing the value back into the object

aspnetde commented 4 years ago

Thanks. I took a quick look and added a failing test (*). It seems like serializing works as expected. But I don't see how, where, or when the ReadJson method would be invoked. A break point set there isn't hit when running the test.

(Furthermore, I assume that this one here contains all the necessary magic.)

(*)

testCase "Inserting and findOne with Single Case Discriminated Unions as Id work" <| fun _ ->
    useDatabase mapper<| fun db ->
        let records = db.GetCollection<RecordWithSingleCaseDUAsId>()
        let id = SingleCaseDU(42)
        let record = { Id = id }
        records.Insert(record) |> ignore
        let r = records.FindOne(fun r -> r.Id = id)
        match r with
        | { Id = id } -> pass()
        | otherwise -> fail()
aspnetde commented 4 years ago

In case I will never be able to finish this. This proofs that the data is correctly written to the database:

testCase "Inserting with a Single Case Discriminated Union as Id works" <| fun _ ->
    useDatabase mapper<| fun db ->
        let records = db.GetCollection<RecordWithSingleCaseDUAsId>()
        let id = SingleCaseDU(42)
        let record = { Id = id }
        records.Insert(record) |> ignore
        let insertSuccessful = records.FindAll() |> Seq.exists (fun x -> x.Id = id)
        if insertSuccessful then pass() else fail()

The reason the ReadJson() method is never called when FindOne() is being applied, is that no record is being found.

To be continued.

aspnetde commented 4 years ago

Using FindById() works when the id gets serialized with the internal utility:

testCase "Inserting and FindById with Single Case Discriminated Unions as Id work" <| fun _ ->
    useDatabase mapper<| fun db ->
        let records = db.GetCollection<RecordWithSingleCaseDUAsId>()
        let id = SingleCaseDU(42)
        let record = { Id = id }
        records.Insert(record) |> ignore
        let bsonId = Bson.serializeField id
        let r = records.FindById(bsonId)
        match r with
        | { Id = id } -> pass()
        | otherwise -> fail()

So what is left is FindOne() which does not work yet. Given that the internal serialization used by LiteDB.Fsharp for discriminated unions is okay ({"SingleCaseDU":42}), the dead end here is the QueryVisitor class of LiteDB, which unfortunately is internal. The query generated here results in a lookup for a value {"Tag":0,"Item":42} – which obviously does not exist.

So I see two options:

  1. Overwriting the serialization behavior of the QueryVisitor for discriminated unions -> No idea how to do that.
  2. Serializing discriminated unions the same way as the QueryVisitor does. I do not think this is a good idea as the implementation could change at any time.

tl;dr: This looks like a dead end right now to me. But at least querying single records by their ID is possible.

Zaid-Ajaj commented 4 years ago

Have you tried using the extensions in the LiteDB.FSharp.Extensions module? there are specific extension methods for LiteCollection<'t> that should work nicely with lambdas to define the search predicate.

open LiteDB.FSharp.Extensions

let records = db.GetCollection<RecordWithSingleCaseDUAsId>()
let id = SingleCaseDU(42)
let record = { Id = id }
records.Insert(record) |> ignore

match records.findOne (fun value -> value.Id = id) with
| { Id = id } -> pass()
| otherwise -> fail()

I will look into why FindOne doesn't work, sometime this week

aspnetde commented 4 years ago

Interesting, I wondered if the readme is wrong or why all the methods are written in lower case there - now I know ;). No, I did not use the extensions yet. Doing so results in an exception, as a DU isn't a proper BsonValue.

System.InvalidCastException: Value is not a valid BSON data type - Use Mapper.ToDocument for more complex types converts
   at LiteDB.BsonValue..ctor(Object value)
   at LiteDB.FSharp.Query.createQueryFromExpr[t](FSharpExpr expr) in /Users/thomas/dev/LiteDB.FSharp/LiteDB.FSharp/Query.fs:line 14
   at LiteDB.FSharp.Query.createQueryFromExpr[t](FSharpExpr expr) in /Users/thomas/dev/LiteDB.FSharp/LiteDB.FSharp/Query.fs:line 93
   at LiteDB.FSharp.Extensions.LiteCollection`1.findOne[T,t](LiteCollection`1 collection, FSharpExpr`1 expr) in /Users/thomas/dev/LiteDB.FSharp/LiteDB.FSharp/Extensions.fs:line 35
   at Tests.LiteDatabase.liteDatabaseUsage@140-3.Invoke(LiteDatabase db) in /Users/thomas/dev/LiteDB.FSharp/LiteDB.FSharp.Tests/Tests.LiteDatabase.fs:line 145
   at Tests.LiteDatabase.useDatabase(BsonMapper mapper, FSharpFunc`2 f) in /Users/thomas/dev/LiteDB.FSharp/LiteDB.FSharp.Tests/Tests.LiteDatabase.fs:line 58
   at Tests.LiteDatabase.liteDatabaseUsage@139-2.Invoke(Unit _arg2) in /Users/thomas/dev/LiteDB.FSharp/LiteDB.FSharp.Tests/Tests.LiteDatabase.fs:line 140
   at Expecto.Impl.execTestAsync@960-1.Invoke(Unit unitVar) in C:\Users\Anthony Lloyd\src\expecto\Expecto\Expecto.fs:line 964

So I guess there would be a serialization necessary for that case. But I am not sure if it's appropriate to just handle DU's differently here then the rest, from your point of view.

Zaid-Ajaj commented 4 years ago

I wondered if the readme is wrong or why all the methods are written in lower case there - now I know ;)

I used lower case functions for fsharp queries to distinguish them from the ones used with C# (upper case). You should almost always use the lower case ones.

Doing so results in an exception, as a DU isn't a proper BsonValue.

My bad, should be fixed now, update nuget from 2.13.0 -> 2.14.0

Zaid-Ajaj commented 4 years ago

Hello @aspnetde, is this issue considered resolved? If that is the case, can you please close the issue or let know so that I could do that!

aspnetde commented 4 years ago

I think we can consider this done πŸ‘