-
Notifications
You must be signed in to change notification settings - Fork 272
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Mongo async support #1825
Mongo async support #1825
Conversation
! 💯 Don't know that I can review this, but this souuunds pretty sweet :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just have the one question. I'll test it out locally in the next few days. Thanks for sending this over.
if (elem.isInstanceOf[org.bson.Document]) { | ||
setFromDocumentList(jlist.asInstanceOf[java.util.List[org.bson.Document]]) | ||
} else { | ||
setBox(Full(jlist.asScala.toList.asInstanceOf[MyType])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the toList
call here still needed?
if(elem.isInstanceOf[org.bson.Document]) { | ||
setFromDocumentList(jlist.asInstanceOf[java.util.List[org.bson.Document]]) | ||
} else { | ||
setBox(Full(jlist.toList.asInstanceOf[MyType])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another toList
call
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unfortunately it is needed to match override type MyType = List[ListType]
as jlist
is java list and we need Scala List[ListType]
. Same thing with previous toList
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought I saw an asScala
call before the toList
call. The previous one has an asScala
call before the toList
call, which, I would think, would already have converted it to a Scala List.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well - maybe it can be done better - I'm open for suggestions. when I tried in scala REPL:
scala> :paste
// Entering paste mode (ctrl-D to finish)
val l = new java.util.ArrayList[Int](2)
l.add(1)
l.add(2)
import scala.collection.JavaConverters._
l.asScala
// Exiting paste mode, now interpreting.
l: java.util.ArrayList[Int] = [1, 2]
import scala.collection.JavaConverters._
res6: scala.collection.mutable.Buffer[Int] = Buffer(1, 2)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I understand now. asScala
returns a scala.collection.mutable.Buffer in this instance, which still needs to be converted to List.
Anyway, this should be good then. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I could go through and change every concrete List
declaration in Lift to Seq
, I would sooo do it <_<
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was able to take a closer look. Looking good, just a few minor things and a few questions.
useCollAsync { coll => | ||
val p = Promise[BaseRecord] | ||
val doc: Document = inst.asDocument | ||
val options = new UpdateOptions().upsert(true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably want to use the passed in parameter here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks to be the only thing not addressed. The upsert
parameter should be used here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marekzebrowski I think this was the only thing left, do you mind if I just go ahead and update the branch?
* replaces document with new one with given id. if `upsert` is set to true inserts new document | ||
* in similar way as save() from sync api | ||
*/ | ||
def replaceOneAsync(inst: BaseRecord, upsert: Boolean, concern: WriteConcern) = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think of adding default values for upsert (true) and concern (ACKNOWLEDGED)?
Also, please add an explicit return type.
val doc: Document = inst.asDocument | ||
val options = new UpdateOptions().upsert(true) | ||
val filter = new org.bson.Document("_id", doc.get("_id")) | ||
coll.replaceOne(filter, doc, options, new SingleResultCallback[UpdateResult] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we be calling before/afterSave and resetDirty here, like the other functions?
@@ -147,6 +160,11 @@ class MongoListField[OwnerType <: BsonRecord[OwnerType], ListType: Manifest](rec | |||
// set this field's value using a DBObject returned from Mongo. | |||
def setFromDBObject(dbo: DBObject): Box[MyType] = | |||
setBox(Full(dbo.asInstanceOf[BasicDBList].toList.asInstanceOf[MyType])) | |||
|
|||
def setFromDocumentList(list: java.util.List[Document]): Box[MyType] = { | |||
throw new RuntimeException("Warning, , setting Document as field with no converstion, probably not something you want to do") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: converstion
val map = Map("a" -> "4", "b" -> "5", "c" -> "6") | ||
val doc = new Document(map) | ||
rec.mandatoryStringMapField.setFromDocument(doc) | ||
rec.mandatoryStringMapField.get must_== map |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Codacy complained about this get
call here. It must think it's being called on an Option
. Changing it to value
should shut it up.
*/ | ||
private[this] val dbs = new ConcurrentHashMap[ConnectionIdentifier, (MongoClient, String)] | ||
val codecRegistry = CodecRegistries.fromRegistries(com.mongodb.MongoClient.getDefaultCodecRegistry(), | ||
CodecRegistries.fromCodecs(new LongPrimitiveCodec, new IntegerPrimitiveCodec) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain why the need to add these 2 codecs? Does it have to do with Java/Scala differences?
Also, should we make the registry customizable?
I'm still wrapping my head around the new java driver and the async support.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
without that codecs old-style (2.x with BasicDbObject) fails to work. That is caused because default com.mongodb.async.client.MongoClientImpl.getDatabase
does not include the same codec registry as old com.mongodb.MongoClient.getDatabase
that leads to runtime errors in serialization/deserialization.
Yes, it could be made customizable - in Mongo driver it is.
limit scope of privates in MongoAsync CR fixes use upsert parameter
0bba74e
to
00fd3d2
Compare
please go ahead, I don't mind
2017-02-20 11:52 GMT+01:00 Tim Nelson <notifications@github.com>:
… ***@***.**** commented on this pull request.
------------------------------
In persistence/mongodb-record/src/main/scala/net/liftweb/mongodb/record/
MongoMetaRecord.scala
<#1825 (comment)>:
> @@ -280,6 +292,33 @@ trait MongoMetaRecord[BaseRecord <: MongoRecord[BaseRecord]]
}
/**
+ * replaces document with new one with given id. if `upsert` is set to true inserts new document
+ * in similar way as save() from sync api
+ */
+ def replaceOneAsync(inst: BaseRecord, upsert: Boolean, concern: WriteConcern) = {
+ useCollAsync { coll =>
+ val p = Promise[BaseRecord]
+ val doc: Document = inst.asDocument
+ val options = new UpdateOptions().upsert(true)
@marekzebrowski <https://github.com/marekzebrowski> I think this was the
only thing left, do you mind if I just go ahead and update the branch?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1825 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAqHLIQGkmUxyBL1oNNUUqRyOkZ_Aaaoks5reXCGgaJpZM4LsqRn>
.
--
Marek Żebrowski
|
Since this branch is on the sgrouples org and I don't have access to modify it, I'm going to close this PR and open a new one. |
as discussed
sgrouples/rogue-fsqio#15
mongo async support for lift 3.1
pre-requisite for porting rogue-async to lift (separate project):
https://github.com/sgrouples/rogue-fsqio/tree/lift31