Skip to content
This repository has been archived by the owner on Jul 2, 2020. It is now read-only.

Sorting JSON arrays by toString removed #51

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Sorting JSON arrays by toString removed #51

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
92a15ee
Sorting JSON arrays by toString removed
Oct 27, 2016
8d94b76
Added logback-classic dependency
Nov 7, 2016
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
1 change: 1 addition & 0 deletions build.sbt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ lazy val baseSettings = Seq(
),
scalacOptions in (Compile, console) := compilerOptions,
libraryDependencies ++= Seq(
"ch.qos.logback" % "logback-classic" % "1.1.7",
"com.twitter" %% "finagle-http" % "6.28.0",
"com.twitter" %% "finagle-thriftmux" % "6.28.0",
"com.twitter" %% "scrooge-generator" % "4.0.0",
Expand Down
2 changes: 1 addition & 1 deletion src/main/scala/com/twitter/diffy/lifter/JsonLifter.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ object JsonLifter {
case JsonToken.START_ARRAY =>
node.elements.toSeq.map {
element => lift(element)
} sortBy { _.toString }
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorting is meant to prevent false positives caused by inconsistent orders in which the same Set can be rendered as a Json array. Since Json does not have any notion of Set we have to sort just in case the collection may have originally been a Set. The down side is that this creates a blind spot for OrderingDifferences in real lists. i.e. we have false negatives instead of false positives.

Copy link
Author

Choose a reason for hiding this comment

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

Sorting in our case creates a lot of PrimitiveDifferences (30% - 50%) instead of OrderingDifferences.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's weird. Can you share a bit more about what the differences look like. By definition, if all you have are ordering differences then there should be no differences left after sorting. This might be a bug that requires a different fix.

Copy link
Author

@x87-va x87-va Nov 11, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I made a demo for this case. Try to send 10 requests https://fifth-howl-149214.appspot.com/_ah/api/diff/v1/reply through Diffy.

case JsonToken.START_OBJECT => {
val fields = node.fieldNames.toSet
if (fields.exists{ field => Try(toolbox.parse(s"object ${field}123")).isThrow}) {
Expand Down