vapor-community / mongo-driver

MongoDB driver for Fluent
28 stars 26 forks source link

Modify is not working properly #27

Open voronianski opened 7 years ago

voronianski commented 7 years ago

I noticed a critical bug in driver behavior. Modify functionality is not working as expected. I think issue is connected to https://github.com/vapor/mongo-driver/issues/25 but I want to describe it in more detail. Steps to reproduce (you can run locally example app - https://github.com/voronianski/vapor-server-example/ to repro):

{ "_id" : ObjectId("5f214f58d66cdc2b53b99b99"), "id" : null, "text" : "some" }
{ "_id" : ObjectId("63214f58d66cdc2b54b99b99"), "id" : null, "text" : "some 2" }
{ "_id" : ObjectId("5f214f58d66cdc2b53b99b99"), "id" : "63214f58d66cdc2b54b99b99", "text" : "some change" }
{ "_id" : ObjectId("63214f58d66cdc2b54b99b99"), "id" : null, "text" : "some 2" }

Observe that "text" was updated in a wrong document. It looks like driver makes wrong query to update document and matches first one without id field.

Implementation of Controller and Model:

voronianski commented 7 years ago

Ok, it looks like I've found a workaround for this problem.

MongoDriver doesn't care about syncing Fluent's required id key and Mongo's _id. So you need to do it by yourself - https://github.com/voronianski/vapor-server-example/commit/d372780633db5456959b16d98da8d191df5fc098

Required changes:

 final class Post: Model {
+   var id: Node? // required to conform Fluent protocol
+   var _id: Node? // required to store correct value in db
    var text: String

   init(node: Node, in context: Context) throws {
+     id = try node.extract("id")
+     _id = try node.extract("_id")
      text = try node.extract("text")
    }

   func makeNode(context: Context) throws -> Node {
      return try Node(node: [
-       "id": id, // we don't want to double values in response
+       "_id": id,
        "text": text
      ])
    }

But I think this issue still needs to be addressed on driver level.

tanner0101 commented 7 years ago

This should work by just having id = try node.extract("_id") and "_id": id right?

voronianski commented 7 years ago

@tannernelson probably yes, but I decided to keep track of _id in class instance for safety.

tanner0101 commented 7 years ago

What kind of safety? Fluent expects that the model.id be the same id as what is in the database. Storing them as separate values might cause issues elsewhere.

voronianski commented 7 years ago

Yeah, your example is cleaner, just thought _id maybe needed somewhere.

osmarogo commented 7 years ago

Hi Every one!

I'm facing an issue when I fetch my documents by Entity.All(), this happens when I get a Document with another embedded document.

In my model, I'm setting a relationship between User Entity that contains an embedded Token document in the userTokenId property, bellow you can see how I wrote my objects.

User Model.

final class User: Model {
    var id: Node?
    var _id: Node?
    var username: String
    var exists: Bool = false
    var userTokenId : Token?

    init(username: String, userTokenId:Token? = nil) {
        self.username = username
        self.userTokenId = userTokenId
    }

    init(node: Node, in context: Context) throws {
        id = try node.extract("id")
        _id = try node.extract("_id")
        username = try node.extract("username")
        userTokenId = try node.extract("userTokenId")
    }

    func makeNode(context: Context) throws -> Node {
        return try Node(node: [
            "_id": id,
            "username": username,
            "userTokenId": userTokenId
            ])
    }
}

extension UserNout: Preparation {
    static func prepare(_ database: Database) throws {
        //
    }

    static func revert(_ database: Database) throws {
        //
    }
}

Token Model

final class Token: Model {
    var id: Node?
    var _id: Node?
    var exists: Bool = false
    var token: String

    init(token: String) {
        self.token = token
    }

    init(node: Node, in context: Context) throws {
        id = try node.extract("id")
        _id = try node.extract("_id")
        token = try node.extract("token")
    }

    func makeNode(context: Context) throws -> Node {
        return try Node(node: [
            "_id": id,
            "token": token
            ])
    }
}

extension Token: Preparation {
    static func prepare(_ database: Database) throws {
        //
    }

    static func revert(_ database: Database) throws {
        //
    }
}

When I save one user at the first time I had this response.

screen shot 2017-04-07 at 10 57 43 am

The relation seems works fine when I save the object through Modify Fluent method as you can see below.

screen shot 2017-04-07 at 10 47 12 am

On the DB the collection looks ok as well.

User Document.

screen shot 2017-04-07 at 10 49 35 am

Token Document.

screen shot 2017-04-07 at 10 49 26 am

However the issue comes up when I get my document through User.all(), the id of the token Entity comes nil.

screen shot 2017-04-07 at 10 51 13 am

I'm not sure if this is an issue on my model, I appreciate some advice to know what can I do about that.

valeriomazzeo commented 7 years ago

you guys should try again after https://github.com/vapor-community/mongo-driver/pull/48 is merged