UpsertedReader should handle other types than BSONObjectID for the _id field

[akkie]

I have a document which uses a composite key in the _id field.

{
  "_id": {"providerID": "basic-auth", "providerKey": "user1"}
}

If I try to save the document with the upsert flag set to true, I get an error if the document gets inserted.

((0)/_id,List(ValidationError(List(error.objectId),WrappedArray())))

I'm not entirely sure what happened internally. But it seems that if the documented gets inserted, then Play-ReactiveMongo tries to return the created BSONObjectID. But in my case the '_id' field doesn't contain a BSONObjectID. It seems that the error occurs in the UpsertedReader.

[totekp]

I submitted a PR last week with changes to fix validation error. #153. You can make a similar change with case obj: JsObject => JsSuccess(obj) or conform to the code style so only BSONObjectId type is allowed in _id.

[akkie]

@totekp I think you have misinterpreted what @cchantep has meant with code style. I think he meant that Any types should be avoided and not that only BSONObjectId type is allowed in _id. @cchantep Please correct me if I'm wrong.

[cchantep]

You're right. Typeless PR cannot be accepted.

[totekp]

The tests from the PR are valid. If tests were added to allow _id to be non-BSONObjectId, then regression introduced by recent changes should have been prevented. https://github.com/ReactiveMongo/Play-ReactiveMongo/pull/153/files#diff-c3831b134298aeb236c72a942c3fafc3R99

[cchantep]

@tokekp testing that the altered code is working as you designed it is different from testing if fixes the issue the way it should. Having a current issue is not a reason to introduce unsafe code. PR without Any and asInstanceOf is welcome.

[totekp]

The problem seems to lie with _id: Any as the type of Upserted in https://github.com/ReactiveMongo/ReactiveMongo/blob/dba5b068203fd6aee41c0fc4125b4714fd9b4d10/driver/src/main/scala/api/commands/rwcommands.scala#L113. Someone could make it a bounded generic type [T <: Any] if that's what TODO means. My workaround is using a different field name since performance/storage isn't an issue.

[cchantep]

See ReactiveMongo/ReactiveMongo#378

[cchantep]

The Upserted._id being a BSONValue after the driver change, it can be different from ObjectId.

import reactivemongo.bson.BSONValue
import reactivemongo.api.commands.Upserted
import play.modules.reactivemongo.json.BSONFormats

val upserted: Future[Seq[Upserted]] = collection.update(Json.obj("name" -> "Foo"),
  Json.obj("$set" -> Json.obj("role" -> "Bar")),
  upsert = true).map(_.upserted)

val ids: Seq[JsValue] = upserted.foreach { up =>
  val id: BSONValue = up._id
  BSONFormats.toJSON(id)
}

[totekp]

Changes look great. I didn't know there was the function BSONFormats.toBSON since I've been using ImplicitBSONHandlers.JsObjectWriter.write for a while. Thanks.