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 enhancements #1566

Merged
merged 10 commits into from Jun 2, 2014

Conversation

Projects
None yet
3 participants
@eltimn
Copy link
Member

eltimn commented May 21, 2014

This is a bunch of mongo related stuff I added. Hopefully the commit messages explain well enough.

Also fixes #1549

@Shadowfiend

This comment has been minimized.

Would charSplit do better here, since it directly gives a List?

This comment has been minimized.

Copy link
Member Author

eltimn replied May 28, 2014

It looks like it will. I'll update it.

@Shadowfiend

This comment has been minimized.

Should we have generalized extractors for these specialized serializations of mongo types?

This comment has been minimized.

Copy link
Member Author

eltimn replied May 28, 2014

That would be nice as they're currently in a couple of different places, but I wasn't sure how to do that.

They are also defined in the Serializers.scala file.

It would be nice to be able to use the json serializers that are defined on owner.meta.formats, instead of having to use the hack below to determine which date serializer is being used.

I'll take another look at this. Do you have any suggestions?

val dt = owner.meta.formats.dateFormat.parse(s).getOrElse(throw new MongoException("Can't parse "+ s + " to Date"))
if (owner.meta.formats.customSerializers.exists { _.isInstanceOf[DateTimeSerializer] }) new DateTime(dt)
else dt
case JObject(JField("$uuid", JString(s)) :: Nil) => UUID.fromString(s)

This comment has been minimized.

@Shadowfiend

Shadowfiend May 28, 2014

Member

Sorry, I'll make my remark here…

Can we not just have:

object ObjectIdJValue {
  def unapply(json: JValue): Option[ObjectId] = {
    json match {
      case JObject(JField("$oid", JString(objectIdString))) if ObjectId.isValid(objectIdString) =>
        Some(new ObjectId(objectIdString))
      case _ =>
        None
    }
  }
}

And use that as:

  case JArray(array) => setBox(Full(array.map {
    case JsonObjectId(objectId) => objectId
    case JsonRegex(regex) => regex
    case JsonDate(date) => date
    case other => other.values
  }))

(Another thing to note here: we can leave the match off of the map, since you can use pattern matching directly in the function body instead of going through the intermediate jv variable.)

The date is the more complicated one, because it requires a date format; I do think we can provide it to the unapply implicitly, however?

This comment has been minimized.

@eltimn

eltimn May 28, 2014

Author Member

The extractors look much cleaner, I'll update the code.

The date is more that just a format, it's java.util.Date vs org.joda.time.DateTime.

They both use the same JValue format, which now seems to have been a mistake.

This comment has been minimized.

@Shadowfiend

Shadowfiend May 28, 2014

Member

Well since they're represented the same way in BSON, I think that actually makes sense. We can only decide which one it's going to be once we go to deserialize to an actual object, right? Does seem a bit hacky as is though… :/

This comment has been minimized.

@eltimn

eltimn May 28, 2014

Author Member

Yes, that's right. We do know the type as there's a type parameter on MongoListField, but I wasn't sure how to utilize that.

This comment has been minimized.

@Shadowfiend

Shadowfiend May 28, 2014

Member

Perhaps with a manifest?

This comment has been minimized.

@Shadowfiend

Shadowfiend May 28, 2014

Member

Although the manifest would only work if we got DateTime as the type parameter, whereas we'd want date deserialization to also work if we got Any as the type parameter, for example.

This comment has been minimized.

@eltimn

eltimn Jun 1, 2014

Author Member

I ended up using the manifest. It would be nice to support Any as a type parameter, but this Field only supports a limited set of types, which I documented in the ScalaDoc.

This comment has been minimized.

@eltimn

eltimn Jun 1, 2014

Author Member

I ended up using the manifest. It would be nice to support Any as a type parameter, but this only supports a limited set of types. I did document the supported types in the ScalaDoc.

/**
* An ObjectId extractor.
*/
object AsObjectId {

This comment has been minimized.

@Shadowfiend

Shadowfiend Jun 1, 2014

Member

Does this become unnecessary with JsonObjectId?

This comment has been minimized.

@eltimn

eltimn Jun 1, 2014

Author Member

I don't think so as this is for extracting an ObjectId from a String. This is useful in RestHelper APIs, for example.

This comment has been minimized.

@Shadowfiend

Shadowfiend Jun 1, 2014

Member

Lol. Yeah, of course. Too cursory on my part :D

@Shadowfiend

This comment has been minimized.

Copy link
Member

Shadowfiend commented Jun 1, 2014

This looks good to me and tests pass. 👍 , will try to merge later today or tomorrow barring any additional feedback/concerns.

Shadowfiend added a commit that referenced this pull request Jun 2, 2014

@Shadowfiend Shadowfiend merged commit 826d2a2 into master Jun 2, 2014

@Shadowfiend Shadowfiend deleted the tcn_mongo_stuff branch Jun 2, 2014

@fmpwizard fmpwizard added this to the 2.6-M4 milestone Jun 7, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment