Skip to content

Commit

Permalink
Drop unmaintained diff library for logging MediaAtom changes
Browse files Browse the repository at this point in the history
This removes the unmaintained bizzabo/diff library ("ai.x" %% "diff") from media-atom-maker,
as a prelude to the Scala 2.13 upgrade in PR #1140 (the diff library is only available for
Scala 2.11 & 2.12).

The library is only used for the media-atom-maker audit log, logging a diff when certain
fields on MediaAtom entities change. The diff text only goes to the ELK logs in the
free-format message & description fields.

Originally, there was some functionality in the media-atom-maker UI involving an audit
button, but this was removed with #759 in February 2018, and the ELK became the place this
information was sent to (the only place) with #767. See also #868 (comment) for a bit of
additional context.

All this means that the exact format of the diff logged to the ELK is not that important -
right now, the information could only really be used by a developer as an informative
diagnostic - there is no process that cares if format changes.

We looked at using alternative diffing libraries (diffx or difflicious) to produce the diff,
but getting them to produce the format we wanted (a one-line diff with only changed fields
taken from an accept-list, and no colouring) looked like it would take a bit of work - so,
given that there were only a few fields to look at, we wrote a custom diff function, and
extracted the diffing method out, so it could be easily unit-tested.
  • Loading branch information
rtyley committed Jan 31, 2024
1 parent 5b081ad commit 0d0535c
Show file tree
Hide file tree
Showing 3 changed files with 23 additions and 20 deletions.
34 changes: 20 additions & 14 deletions app/model/commands/UpdateAtomCommand.scala
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package model.commands

import java.util.Date
import ai.x.diff.DiffShow
import com.gu.atom.data.{AtomSerializer, VersionConflictError}
import com.gu.contentatom.thrift.{Atom, ContentAtomEvent, EventType, ChangeRecord => ThriftChangeRecord}
import com.gu.media.logging.Logging
Expand Down Expand Up @@ -124,22 +123,29 @@ case class UpdateAtomCommand(id: String, atom: MediaAtom, override val stores: D
}

object UpdateAtomCommand {
private val interestingFields = List("title", "category", "description", "duration", "source", "youtubeCategoryId", "license", "commentsEnabled", "channelId", "legallySensitive")
private val interestingFields = Seq[(String, MediaAtom => Option[Any])](
("title", a => Some(a.title)),
("category", a => Some(a.category)),
("description", _.description),
("duration", _.duration),
("source", _.source),
("youtubeCategoryId", _.youtubeCategoryId),
("license", _.license),
("commentsEnabled", _.composerCommentsEnabled),
("channelId", _.channelId),
("legallySensitive", _.legallySensitive)
)

// We don't use HTTP patch so diffing has to be done manually
def createDiffString(before: MediaAtom, after: MediaAtom): String = {
val fieldDiffs = DiffShow.diff[MediaAtom](before, after).string
.replaceAll("\\[*[0-9]+m", "") // Clean out the silly console coloring stuff
.split('\n')
.map(_.trim())
.filter(line => !line.contains("ERROR")) // More silly stuff from diffing library
.filter(line => interestingFields.exists(line.contains))
.mkString(", ")

if (fieldDiffs == "") { // There's a change, but in some field we're not interested in (or rather, unable to format nicely)
val changedFields = for {
(name, extractor) <- interestingFields
distinctValues = Seq(extractor(before), extractor(after)).distinct
if distinctValues.size > 1
} yield s"$name: ${distinctValues.map(_.getOrElse("[NONE]")).mkString(" -> ")}"

if (changedFields.isEmpty) { // There's a change, but in some field we're not interested in (or rather, unable to format nicely)
"Updated atom fields"
} else {
s"Updated atom fields ($fieldDiffs)"
}
} else s"Updated atom fields (${changedFields.mkString(", ")})"
}
}
3 changes: 0 additions & 3 deletions build.sbt
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,6 @@ val scanamoVersion = "1.0.0-M9"

val playJsonExtensionsVersion = "0.40.2"
val okHttpVersion = "2.4.0"
val diffVersion = "2.0.1"

val capiAwsVersion = "0.5"

val scalaTestVersion = "3.0.8"
Expand Down Expand Up @@ -135,7 +133,6 @@ lazy val app = (project in file("."))
libraryDependencies ++= Seq(
ehcache,
"com.fasterxml.jackson.core" % "jackson-databind" % jacksonDatabindVersion,
"ai.x" %% "diff" % diffVersion,
"com.amazonaws" % "aws-java-sdk-sts" % awsVersion,
"com.amazonaws" % "aws-java-sdk-ec2" % awsVersion,
"org.scalatestplus.play" %% "scalatestplus-play" % scalaTestPlusPlayVersion % "test",
Expand Down
6 changes: 3 additions & 3 deletions test/model/commands/UpdateAtomCommandTest.scala
Original file line number Diff line number Diff line change
Expand Up @@ -40,18 +40,18 @@ class UpdateAtomCommandTest extends FunSuite with MustMatchers {
)

test("Diff output when nothing changes") {
createDiffString(mediaAtomFixture, mediaAtomFixture) must be("Updated atom fields (category = News$,, channelId = None,, description = Some( \"Example description\" ),, duration = Some( 1 ),, legallySensitive = None,, license = None,, source = Some( \"source\" ),, title = \"title\",, youtubeCategoryId = None,, youtubeTitle = \"title\")")
createDiffString(mediaAtomFixture, mediaAtomFixture) must be("Updated atom fields")
}

test("Diff output when description changes") {
createDiffString(mediaAtomFixture, mediaAtomFixture.copy(description = Some("New description"))) must be(
"Updated atom fields (MediaAtom( ..., description = Some( \u001B\"Example description\"\u001B -> \u001B\"New description\"\u001B ) ))"
"Updated atom fields (description: Example description -> New description)"
)
}

test("Diff output when description is removed") {
createDiffString(mediaAtomFixture, mediaAtomFixture.copy(description = None)) must be(
"Updated atom fields (MediaAtom( ..., description = \u001BSome( \"Example description\" )\u001B -> \u001BNone\u001B ))"
"Updated atom fields (description: Example description -> [NONE])"
)
}
}

0 comments on commit 0d0535c

Please sign in to comment.