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.

Sign up for GitHub

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

Jump to bottom

Work on improved Tuple support in lift-json #1768

Merged
merged 22 commits into from
Apr 21, 2017
Merged

Work on improved Tuple support in lift-json #1768

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
d232d9c
Add a broken spec from the bug report on tuple serialization.
farmdawgnation Feb 6, 2016
931f48e
Make Meta smart enough to detect Tuples.
farmdawgnation Feb 6, 2016
79962e3
Serialize tuples as Arrays.
farmdawgnation Feb 6, 2016
1ecbaa9
Pull mkMapping out into its own first-class function.
farmdawgnation Feb 7, 2016
2eb534f
Various readability improvements.
farmdawgnation Feb 7, 2016
7151444
Implement support for homogonous tuples.
farmdawgnation Feb 7, 2016
fc5513c
(Mostly) support for heterogenous tuples during deserialization.
farmdawgnation Feb 14, 2016
f62f8f4
Add a comment explaining the awkward condition this is currently in
farmdawgnation Feb 14, 2016
8733e6a
Merge remote-tracking branch 'origin/master' into msf_issue_1757
farmdawgnation Jan 24, 2017
f8c730f
Make extract a bit easier to read.
farmdawgnation Jan 29, 2017
307a17d
Remove some merge boo boos.
farmdawgnation Jan 29, 2017
b73d660
Correctly map tuples to the HCol concept.
farmdawgnation Jan 29, 2017
128f3fe
Retain the ability to interpret old tuples.
farmdawgnation Jan 29, 2017
a6fb65d
Quick style fix to use private[this].
Shadowfiend Mar 9, 2017
246c504
Merge remote-tracking branch 'origin/master' into msf_issue_1757
farmdawgnation Mar 18, 2017
fe10c38
Address some minor stylistic concerns
farmdawgnation Mar 18, 2017
681616e
Correctly handle out-of-order old style tuples
farmdawgnation Apr 15, 2017
3b6a4e5
Add experimentalTupleSupport flag to formats
farmdawgnation Apr 15, 2017
7798c02
Merge remote-tracking branch 'origin/master' into msf_issue_1757
farmdawgnation Apr 15, 2017
c15c9be
Take the constructor reflection hit for tuples at construction
farmdawgnation Apr 15, 2017
8c026bf
Rename experimentalTupleSupport to tuplesAsArrays
farmdawgnation Apr 20, 2017
9ebf73a
Update readme for new tuple implementation
farmdawgnation Apr 20, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 12 additions & 7 deletions core/json/src/main/scala/net/liftweb/json/Extraction.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ object Extraction {
case x: Iterable[_] => JArray(x.toList map decompose)
case x if (x.getClass.isArray) => JArray(x.asInstanceOf[Array[_]].toList map decompose)
case x: Option[_] => x.flatMap[JValue] { y => Some(decompose(y)) }.getOrElse(JNothing)
case x: Product if tuple_?(x.getClass) =>
case x: Product if tuple_?(x.getClass) && formats.experimentalTupleSupport =>
JArray(x.productIterator.toList.map(decompose))
case x =>
val fields = getDeclaredFields(x.getClass)
Expand Down Expand Up @@ -192,7 +192,7 @@ object Extraction {
Col(TypeInfo(clazz, None), mkMapping(typeArgs.head, typeArgs.tail))
} else if (clazz == classOf[Map[_, _]]) {
Dict(mkMapping(typeArgs.tail.head, typeArgs.tail.tail))
} else if (tuple_?(clazz)) {
} else if (tuple_?(clazz) && formats.experimentalTupleSupport) {
val childMappings = typeArgs.map(c => mkMapping(c, Nil)).toList
HCol(TypeInfo(clazz, None), childMappings)
} else {
Expand Down Expand Up @@ -339,18 +339,23 @@ object Extraction {
build(item, mapping).asInstanceOf[Object]
})
val tupleIndex = items.length - 1
val tupleClass = tuples.lift(tupleIndex).getOrElse {

val typedTupleConstructor = tupleConstructors.get(tupleIndex).getOrElse {
throw new IllegalArgumentException(s"Cannot instantiate a tuple of length ${items.length} even though that should be a valid tuple length.")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still think it may be worth turning tuples into a Map (only doing the .getConstructors()(0) bit once during the singleton intialization).

The error here also doesn't seem quite right (at this point we actually couldn't load the tuple class, so while technically we can't instantiate it there's a deeper reason). Additionally, if we hadn't been able to get this class, we would have blown up while initializing the whole singleton object. I'm wondering if we just do a .get here and point to the initialization of tuples to show that if we've gotten to this point in the code, we know that entry exists.

Copy link
Member Author

@farmdawgnation farmdawgnation Mar 21, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if we just do a .get here and point to the initialization of tuples to show that if we've gotten to this point in the code, we know that entry exists.

No, I think having a getOrElse with an exception here is a good way of refactor-proofing the code. Having worked in a project that made way-too-ample use of Option.get last year I think I've landed on always justifying a retrieval that has no alternative much like Box's openOrThrowException requires us to do. The alternative is making a bet that this code can't be fouled by an accidental fat finger and then surfacing an opaque error message to a user of the Framework. Certainly, surfacing this error at all would be bad, but making it opaque is just insult to injury. :)

Still think it may be worth turning tuples into a Map (only doing the .getConstructors()(0) bit once during the singleton intialization).

I'm not quite following the entirety of this. What's the relevance of the getConstructors bit? Do we not already do that once?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, basically doing something like this:

val tupleClasses =
  Seq(classOf[Tuple1[_]]...)
val tupleConstructors =
  tupleClasses.map(clazz => (clazz -> clazz.getConstructors()(0))).toMap

And then looking the constructor up in the map every time.

}

val typedTupleConstructor = tupleClass.getConstructors()(0)
typedTupleConstructor.newInstance(builtItems: _*)

case JArray(items) =>
throw new IllegalArgumentException("Cannot create a tuple of length " + items.length)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to move this condition above the JObject one so the two array possibilities are together.


case JObject(items) if items.forall(_.name.startsWith("_")) =>
newTuple(JArray(items.map(_.value)), mappings)
val sortedItems = items.sortWith { (i1, i2) =>
val numerialName1 = i1.name.drop(1).toInt
val numerialName2 = i2.name.drop(1).toInt

numerialName1 < numerialName2
}
newTuple(JArray(sortedItems.map(_.value)), mappings)

case x =>
throw new IllegalArgumentException("Got unexpected while attempting to create tuples: " + x)
Expand All @@ -370,7 +375,7 @@ object Extraction {
case Arg(path, m, optional) =>
mkValue(root, m, path, optional)

case HCol(targetType, mappings) =>
case HCol(targetType, mappings) if formats.experimentalTupleSupport =>
val c = targetType.clazz
if (tuples.find(_.isAssignableFrom(c)).isDefined) {
newTuple(root, mappings)
Expand Down
8 changes: 8 additions & 0 deletions core/json/src/main/scala/net/liftweb/json/Formats.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,14 @@ trait Formats { self: Formats =>
val customSerializers: List[Serializer[_]] = Nil
val fieldSerializers: List[(Class[_], FieldSerializer[_])] = Nil

/**
* Support for the experimental tuple decomposition/extraction that represents tuples as JSON
* arrays. This provides better support for heterogenous arrays in JSON, but enable it at your
* own risk as it does change the behavior of serialization/deserialization and comes
* with some caveats (such as Scala primitives not being recognized reliably during extraction).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, should we really label this “experimental”? We're not planning on iterating on it really, it's just a different serialization approach? Perhaps serializeTuplesAsArrays?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there should be a yellow flag of sorts that alerts someone who sees nothing except this flag being used that they should tread carefully. If you'd like to create an alias value named "serializeTuplesAsArrays" that reads from this and reference that directly in the extractor, that seems sensible to me. But at least until we're getting some positive reports from the field on this feature I'd like to have a warning inherent in the name. And calling it experimental fits the bill, imo.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my perspective, the implementation is tested. It may have bugs, which we will fix because we support the feature. But we don't expect the API to change, and we do intend on supporting it, at least until this whole extraction paradigm is ripped out in some potential future. That doesn't merit calling it experimental IMO.

Additionally, I'm opposed to having this as a temporary name, because then we create headaches when we want to make it the “correct” name.

I think the new work you're doing on a TypeTags-based extractor merits being called experimental, because the support horizon on it differs from that of other features (by virtue of being in a separate codebase hehe). But if we're introducing this I assume we intend to support it, and if we intend to support it I think it's not experimental.

If you agree, cool. If not, maybe let's see if someone else wants to chime in and break the tie. If we're nowhere within a day or two or when you circle back (whichever is longest), let's disagree and commit to do it your way.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the most compelling argument here are the headaches for converting to a correct name. I still think there are some dangers to turning this on because JSON is simple, but still manages to be bananas at finding bugs in code. That said, it's turned off by default and perhaps we can provide enough context as to the meaning and impact of this change in the documentation to be sufficient.

I'll change it to be named serializeTuplesAsArrays.

*/
val experimentalTupleSupport = false

/**
* The name of the field in JSON where type hints are added (jsonClass by default)
*/
Expand Down
7 changes: 6 additions & 1 deletion core/json/src/main/scala/net/liftweb/json/Meta.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ private[json] object Meta {
(mkContainer(t, `(*,*) -> *`, 1, Dict.apply _), false)
else if (classOf[Seq[_]].isAssignableFrom(raw))
(mkContainer(t, `* -> *`, 0, Col.apply(info, _)), false)
else if (tuples.find(_.isAssignableFrom(raw)).isDefined)
else if (tuples.find(_.isAssignableFrom(raw)).isDefined && formats.experimentalTupleSupport)
(mkHeteroContainer(t), false)
else
mkConstructor(t)
Expand Down Expand Up @@ -272,6 +272,11 @@ private[json] object Meta {
classOf[Tuple22[_,_,_,_,_,_,_,_,_,_,_,_,_,_,_,_,_,_,_,_,_,_]]
)

val tupleConstructors: Map[Int, JConstructor[_]] = tuples.zipWithIndex.map({
case (tupleClass, index) =>
index -> tupleClass.getConstructors()(0)
}).toMap

private val primaryConstructorArgumentsMemo = new Memo[Class[_], List[(String, Type)]]
private val declaredFieldsMemo = new Memo[Class[_], Map[String,Field]]

Expand Down
50 changes: 48 additions & 2 deletions core/json/src/test/scala/net/liftweb/json/ExtractionBugs.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ object ExtractionBugs extends Specification {
}
}

"deserialize list of homogonous tuples" in {
"deserialize list of homogonous tuples w/ experimental tuples disabled" in {
implicit val formats = DefaultFormats

case class Holder(items: List[(String, String)])
Expand All @@ -109,7 +109,7 @@ object ExtractionBugs extends Specification {
deserialized must_== holder
}

"deserialize a list of heterogenous tuples" in {
"deserialize a list of heterogenous tuples w/ experimental tuples disabled" in {
implicit val formats = DefaultFormats

// MSF: This currently doesn't work with scala primitives?! The type arguments appear as
Expand All @@ -122,4 +122,50 @@ object ExtractionBugs extends Specification {
val deserialized = parse(serialized).extract[Holder2]
deserialized must_== holder
}

"deserialize list of homogonous tuples w/ experimental tuples enabled" in {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the enabled/disabled tests are identical here, I wouldn't be opposed to collapsing them into a single test---but don't care too much.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer the way this comes out in the test report.

implicit val formats = new DefaultFormats {
override val experimentalTupleSupport = true
}

case class Holder(items: List[(String, String)])

val holder = Holder(List(("string", "string")))
val serialized = compactRender(Extraction.decompose(holder))

val deserialized = parse(serialized).extract[Holder]
deserialized must_== holder
}

"deserialize a list of heterogenous tuples w/ experimental tuples enabled" in {
implicit val formats = new DefaultFormats {
override val experimentalTupleSupport = true
}

// MSF: This currently doesn't work with scala primitives?! The type arguments appear as
// java.lang.Object instead of scala.Int. :/
case class Holder2(items: List[(String, Integer)])

val holder = Holder2(List(("string", 10)))
val serialized = compactRender(Extraction.decompose(holder))

val deserialized = parse(serialized).extract[Holder2]
deserialized must_== holder
}

"deserialize an out of order old-style tuple w/ experimental tuples enabled" in {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have existing tests for these working with experimental tuples disabled as well, right? I assume that was the old default, just want to make sure we didn't delete the tests in question at some point.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The old behavior probably isn't covered explicitly, but would be covered in Lift-Json's handling of constructor arguments in ExtractionExamplesSpec.

implicit val formats = new DefaultFormats {
override val experimentalTupleSupport = true
}

val outOfOrderTuple: JObject = JObject(List(
JField("_1", JString("apple")),
JField("_3", JString("bacon")),
JField("_2", JString("sammich"))
))

val extracted = outOfOrderTuple.extract[(String, String, String)]

extracted must_== ("apple", "sammich", "bacon")
}
}