Skip to content
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

updateWith not handling Option correctly #166

Open
SmedbergM opened this issue Aug 19, 2016 · 2 comments
Open

updateWith not handling Option correctly #166

SmedbergM opened this issue Aug 19, 2016 · 2 comments

Comments

@SmedbergM
Copy link

It looks like ScalaVertex.updateWith is not correctly handling updates where the updating case class has members of type Option[T] and value None.

Here's a minimal working example:

object UpdateWithMWE extends App {
  case class XYZ(x: Int, y: String, z: Option[String])

  val xyz0 = XYZ(17,"foo", Some("bar"))
  val xyz1 = XYZ(21,"baz", None)

  val g = Neo4jGraph.open("/tmp/testdb")

  val v0 = g + xyz0
  println(v0.valueMap)
  // prints Map(x -> 17, y -> foo, z -> bar)

  v0.updateWith(xyz1)
  println(v0.valueMap)
  // prints Map(x -> 21, y -> baz, z -> bar, __gs -> )
}

I would expect the result of calling v0.updateWith(xyz1) to be to remove the vertex's z property, since the property can't be None or null.

This behavior is caused by the implementation of updateWith, which uses fromCC(xyz1)'s valueMap, which omits all the None members.

SmedbergM pushed a commit to SmedbergM/gremlin-scala that referenced this issue Aug 19, 2016
mpollmeier added a commit that referenced this issue Aug 20, 2016
[Issue #166] Make ScalaElement.updateWith(x) remove properties which …
@mpollmeier
Copy link
Owner

Agreed. Thanks for the PR, just merged.

@mikolak-net
Copy link
Contributor

IIRC the implementation was done this way to preserve functional symmetry with toCC. Since toCC allows to map a graph element to different case classes, one might argue that updateWith should allow to map from different case classes. Of course, the behavior @SmedbergM described is clearly a bug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants