go-openapi / strfmt

openapi toolkit common string formats
Apache License 2.0
102 stars 62 forks source link

strfmt. DataTime and GetBson #41

Closed menta2k closed 5 years ago

menta2k commented 5 years ago

Hellom i found a strange behavior when persisting the data into mongodb. The model have a field like that one : CreatedAt strfmt.DateTime json:"created_at" the model is persised into mongodb using a github.com/globalsign/mgo Created record inside mongo looks like "createdat" : { "data" : "2018-12-04T11:21:10.320Z" } Excpected record is "createdat" : ISODate("2018-12-04T11:21:10.320Z")

Basically it stores datatime as bson map instand of ISODate

From what i can see that is because of GetBson function https://github.com/go-openapi/strfmt/blob/e471370ae57ac74eaf0afe816a66e4ddd7f1b027/time.go#L177

Can you give me a direction what am I doing wrong ?

fredbi commented 5 years ago

Yes this has been seen since the change of bson driver (https://github.com/go-openapi/strfmt/pull/31). Looks like the driver change silently modified the behavior. Slack discussion about this: https://goswagger.slack.com/archives/C04R30YMU/p1540200651000100?thread_ts=1540176946.000100&cid=C04R30YMU

menta2k commented 5 years ago

Thanks for the replay @fredbi can you share a little bit more information about it. I cant see the slack archive cuz dont have access to there and have to contact the workspace administrator for an invitation. Are there any workaround Should I using a official mongodb driver like github.com/mongodb/mongo-go-driver

I mean there is a very big limitation to store datetime as string into the database the field become useless.

fredbi commented 5 years ago

Pasted from slack below.

As stated, I suspect the PR mentioned above to have introduced a change. The mongodb driver has been changed to a maintained package. Maybe the contributor, @jerome-laforge, could shed some light on this.

Igor Kim [il y a 2 mois] I have the following field definition in the swagger.yaml file:

    x-go-custom-tag: "bson:\"createdAt\""
    type: string
    format: date-time

In the generated model: CreatedAt strfmt.DateTime json:"createdAt,omitempty" bson:"createdAt"

But to the database it is added as: "createdAt" : {"data" : "2018-01-01T00:00:00.000Z"}, Not as a datetime object as expected: "createdAt" : ISODate("2018-01-01T00:00:00.000Z") Using mgo I use the following code: err := coll.Insert(&params.Body)

What am I doing wrong?

fredbi [il y a 2 mois] this the way the GetBSON() method is implemented in strfmt

fredbi [il y a 2 mois] FYI: the mgo dependency in strfmt has been updated about 1 month ago. Might have caused some change here

Igor Kim [il y a 2 mois] Thanks for the response. you mean what I described is how it should work? Because I have the same problem with an ObjectId type field. and I can't find a way to store an object with correct types to the Mongodb.

fredbi [il y a 2 mois] I am not familiar with mgo and bson. I am just analyzing what has changed around this feature which has been developed about 2 years ago, so I assume it works. So what you observe is exactly what the BSON getter is doing. The only thing that has changed recently is the repo used for bson package.

fredbi commented 5 years ago

I suspect that some implicit type conversion used to be carried on by the driver, and this is no more the case with the new driver.

The problem is probably the same for Date, Datetime and possibly Duration.

You may try to fix the issue in the SetBSON() methods for these types.

igorskh commented 5 years ago

I have some fast fix for the current globalsign/mgo driver, it's just a fast fix only to make it work for ObjectID and datetime, so I don't think it should be pull requested, but maybe somebody find it useful: https://github.com/igorskh/strfmt Commits:

I want to implement using an official mongodb driver for Go later, but currently have no time.

casualjim commented 5 years ago

I"ve changed to the official driver for mongodb on master now. It doesn't yet implement the ValueMarshaller interface, so this issue is not yet fixed but now it should be possible to fix it