From 37a641619dd6e0c145af4cf522f401ce9ebe3673 Mon Sep 17 00:00:00 2001 From: Roberto Tyley Date: Thu, 2 Oct 2025 16:15:57 +0100 Subject: [PATCH 1/5] Rename end field that uses reserved Thrift word --- build.sbt | 2 -- models/src/main/thrift/content/v1.thrift | 10 ++++------ 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/build.sbt b/build.sbt index caf745b..b0bee30 100644 --- a/build.sbt +++ b/build.sbt @@ -26,8 +26,6 @@ val snapshotReleaseSuffix = "-SNAPSHOT" lazy val artifactProductionSettings = Seq( scalaVersion := "2.13.12", - // This old attempt to downgrade scrooge reserved word clashes is now insufficient... https://github.com/twitter/scrooge/issues/259#issuecomment-1900743695 - Compile / scroogeDisableStrict := true, // scrooge 21.3.0: Builds are now only supported for Scala 2.12+ // https://twitter.github.io/scrooge/changelog.html#id11 crossScalaVersions := Seq("2.12.18", scalaVersion.value), diff --git a/models/src/main/thrift/content/v1.thrift b/models/src/main/thrift/content/v1.thrift index 1ef481c..867a558 100644 --- a/models/src/main/thrift/content/v1.thrift +++ b/models/src/main/thrift/content/v1.thrift @@ -394,10 +394,9 @@ struct AssetFields { 65: optional string price - 66: optional CapiDateTime start + 66: optional CapiDateTime startDate - /* `end` is a reserved word in scrooge, so use Compile / scroogeDisableStrict := true in build.sbt */ - 67: optional CapiDateTime end + 67: optional CapiDateTime endDate 68: optional bool safeEmbedCode @@ -724,9 +723,8 @@ struct MembershipElementFields { 7: optional string identifier 8: optional string image 9: optional string price - 10: optional CapiDateTime start - /* `end` is a reserved word in scrooge, so use Compile / scroogeDisableStrict := true in build.sbt */ - 11: optional CapiDateTime end + 10: optional CapiDateTime startDate + 11: optional CapiDateTime endDate 12: optional string role } From c952332f7e43b8a24d0e8d065d109ad518d94b64 Mon Sep 17 00:00:00 2001 From: Emily Bourke Date: Fri, 3 Oct 2025 11:58:24 +0100 Subject: [PATCH 2/5] Try using circe-generic-extras to rename fields MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit To keep using the old names in the json produced and consumed by these encoders and decoders, it should be possible to use circe-generic-extras to define configured encoders and decoders that do the field renaming. However, this currently fails because deriveConfiguredEncoder and deriveConfiguredDecoder can’t tell that it’s possible to use fezziwig to get the necessary shapeless representations they can normally get already for case classes and traits. It might be possible to update fezziwig to support using it with circe-generic-extras (by outputting whatever deriveConfiguredEncoder needs that deriveEncoder doesn’t), but it’s probably easier to avoid using circe-generic-extras altogether: I’ll investigate that next. (Keeping this commit around for history.) --- build.sbt | 2 ++ .../com/gu/contentapi/json/CirceDecoders.scala | 13 +++++++++++-- .../com/gu/contentapi/json/CirceEncoders.scala | 13 +++++++++++-- 3 files changed, 24 insertions(+), 4 deletions(-) diff --git a/build.sbt b/build.sbt index b0bee30..208cc0e 100644 --- a/build.sbt +++ b/build.sbt @@ -116,6 +116,8 @@ lazy val json = Project(id = "content-api-models-json", base = file("json")) "io.circe" %% "circe-generic" % circeVersion, "io.circe" %% "circe-parser" % circeVersion, "io.circe" %% "circe-optics" % circeVersion, + // note below is deliberately not circeVersion as circe-generic-extras uses a different version number + "io.circe" %% "circe-generic-extras" % "0.14.4", "org.scalatest" %% "scalatest" % scalaTestVersion % Test, "com.google.guava" % "guava" % guavaVersion % Test, "org.gnieh" %% "diffson-circe" % diffsonVersion % Test diff --git a/json/src/main/scala/com/gu/contentapi/json/CirceDecoders.scala b/json/src/main/scala/com/gu/contentapi/json/CirceDecoders.scala index 60fabb2..70d5d54 100644 --- a/json/src/main/scala/com/gu/contentapi/json/CirceDecoders.scala +++ b/json/src/main/scala/com/gu/contentapi/json/CirceDecoders.scala @@ -1,6 +1,8 @@ package com.gu.contentapi.json import io.circe._ +import io.circe.generic.extras._ +import io.circe.generic.extras.semiauto.deriveConfiguredDecoder import io.circe.generic.semiauto._ import com.gu.contentapi.client.model.v1._ import com.gu.contentapi.client.model.schemaorg.{SchemaOrg, SchemaRecipe, AuthorInfo, RecipeStep} @@ -104,7 +106,7 @@ object CirceDecoders { implicit val podcastDecoder: Decoder[Podcast] = deriveDecoder implicit val podcastCategoryDecoder: Decoder[PodcastCategory] = deriveDecoder implicit val assetDecoder: Decoder[Asset] = deriveDecoder - implicit val assetFieldsDecoder: Decoder[AssetFields] = deriveDecoder + implicit val assetFieldsDecoder: Decoder[AssetFields] = deriveConfiguredDecoder implicit val cartoonVariantDecoder: Decoder[CartoonVariant] = deriveDecoder implicit val cartoonImageDecoder: Decoder[CartoonImage] = deriveDecoder implicit val elementDecoder: Decoder[Element] = deriveDecoder @@ -120,7 +122,14 @@ object CirceDecoders { implicit val standardElementFieldsDecoder: Decoder[StandardElementFields] = deriveDecoder implicit val witnessElementFieldsDecoder: Decoder[WitnessElementFields] = deriveDecoder implicit val richLinkElementFieldsDecoder: Decoder[RichLinkElementFields] = deriveDecoder - implicit val membershipElementFieldsDecoder: Decoder[MembershipElementFields] = deriveDecoder + implicit val renameDatesConfiguration: Configuration = Configuration.default.copy( + transformMemberNames = { + case "startDate" => "start" + case "endDate" => "end" + case x => x + } + ) + implicit val membershipElementFieldsDecoder: Decoder[MembershipElementFields] = deriveConfiguredDecoder implicit val embedElementFieldsDecoder: Decoder[EmbedElementFields] = deriveDecoder implicit val instagramElementFieldsDecoder: Decoder[InstagramElementFields] = deriveDecoder implicit val commentElementFieldsDecoder: Decoder[CommentElementFields] = deriveDecoder diff --git a/json/src/main/scala/com/gu/contentapi/json/CirceEncoders.scala b/json/src/main/scala/com/gu/contentapi/json/CirceEncoders.scala index d713773..342f994 100644 --- a/json/src/main/scala/com/gu/contentapi/json/CirceEncoders.scala +++ b/json/src/main/scala/com/gu/contentapi/json/CirceEncoders.scala @@ -8,6 +8,8 @@ import com.gu.contententity.thrift.entity._ import com.gu.contententity.{thrift => contententity} import com.twitter.scrooge.ThriftEnum import io.circe._ +import io.circe.generic.extras._ +import io.circe.generic.extras.semiauto.deriveConfiguredEncoder import io.circe.generic.semiauto._ import io.circe.syntax._ import com.gu.fezziwig.CirceScroogeWhiteboxMacros._ @@ -54,7 +56,7 @@ object CirceEncoders { implicit val podcastEncoder: Encoder[Podcast] = deriveEncoder implicit val podcastCategoryEncoder: Encoder[PodcastCategory] = deriveEncoder implicit val assetEncoder: Encoder[Asset] = deriveEncoder - implicit val assetFieldsEncoder: Encoder[AssetFields] = deriveEncoder + implicit val assetFieldsEncoder: Encoder[AssetFields] = deriveConfiguredEncoder implicit val cartoonVariantEncoder: Encoder[CartoonVariant] = deriveEncoder implicit val cartoonImageEncoder: Encoder[CartoonImage] = deriveEncoder implicit val elementEncoder: Encoder[Element] = deriveEncoder @@ -70,7 +72,14 @@ object CirceEncoders { implicit val standardElementFieldsEncoder: Encoder[StandardElementFields] = deriveEncoder implicit val witnessElementFieldsEncoder: Encoder[WitnessElementFields] = deriveEncoder implicit val richLinkElementFieldsEncoder: Encoder[RichLinkElementFields] = deriveEncoder - implicit val membershipElementFieldsEncoder: Encoder[MembershipElementFields] = deriveEncoder + implicit val renameDatesConfiguration: Configuration = Configuration.default.copy( + transformMemberNames = { + case "startDate" => "start" + case "endDate" => "end" + case x => x + } + ) + implicit val membershipElementFieldsEncoder: Encoder[MembershipElementFields] = deriveConfiguredEncoder implicit val embedElementFieldsEncoder: Encoder[EmbedElementFields] = deriveEncoder implicit val instagramElementFieldsEncoder: Encoder[InstagramElementFields] = deriveEncoder implicit val commentElementFieldsEncoder: Encoder[CommentElementFields] = deriveEncoder From 0b5a99acf230edc11dd6803808857d00242089f9 Mon Sep 17 00:00:00 2001 From: Emily Bourke Date: Fri, 3 Oct 2025 12:59:31 +0100 Subject: [PATCH 3/5] Avoid using circe-generic-extras MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since the changes required are pretty straightforward, it’s easier to use .prepare and .mapJson rather than get circe-generic-extras working with fezziwig. However, this does mean we need to check this code quite carefully: working directly with Json means we lose a lot of static typing guarantees, which we will want to get back with tests. (For example, as it is this doesn’t handle the case where only one of the fields is present correctly!) --- build.sbt | 2 -- .../gu/contentapi/json/CirceDecoders.scala | 26 +++++++++++-------- .../gu/contentapi/json/CirceEncoders.scala | 26 +++++++++++-------- 3 files changed, 30 insertions(+), 24 deletions(-) diff --git a/build.sbt b/build.sbt index 208cc0e..b0bee30 100644 --- a/build.sbt +++ b/build.sbt @@ -116,8 +116,6 @@ lazy val json = Project(id = "content-api-models-json", base = file("json")) "io.circe" %% "circe-generic" % circeVersion, "io.circe" %% "circe-parser" % circeVersion, "io.circe" %% "circe-optics" % circeVersion, - // note below is deliberately not circeVersion as circe-generic-extras uses a different version number - "io.circe" %% "circe-generic-extras" % "0.14.4", "org.scalatest" %% "scalatest" % scalaTestVersion % Test, "com.google.guava" % "guava" % guavaVersion % Test, "org.gnieh" %% "diffson-circe" % diffsonVersion % Test diff --git a/json/src/main/scala/com/gu/contentapi/json/CirceDecoders.scala b/json/src/main/scala/com/gu/contentapi/json/CirceDecoders.scala index 70d5d54..67ea232 100644 --- a/json/src/main/scala/com/gu/contentapi/json/CirceDecoders.scala +++ b/json/src/main/scala/com/gu/contentapi/json/CirceDecoders.scala @@ -1,8 +1,6 @@ package com.gu.contentapi.json import io.circe._ -import io.circe.generic.extras._ -import io.circe.generic.extras.semiauto.deriveConfiguredDecoder import io.circe.generic.semiauto._ import com.gu.contentapi.client.model.v1._ import com.gu.contentapi.client.model.schemaorg.{SchemaOrg, SchemaRecipe, AuthorInfo, RecipeStep} @@ -97,6 +95,19 @@ object CirceDecoders { } } + def renameStartAndEndFields(c: ACursor): ACursor = { + c.withFocus { + _.mapObject { x => + (for { + endDate <- x("end") + startDate <- x("start") + } yield { + x.add("endDate", endDate).add("startDate", startDate) + }).getOrElse(x) + } + } + } + implicit val contentFieldsDecoder: Decoder[ContentFields] = deriveDecoder implicit val editionDecoder: Decoder[Edition] = deriveDecoder implicit val sponsorshipDecoder: Decoder[Sponsorship] = deriveDecoder @@ -106,7 +117,7 @@ object CirceDecoders { implicit val podcastDecoder: Decoder[Podcast] = deriveDecoder implicit val podcastCategoryDecoder: Decoder[PodcastCategory] = deriveDecoder implicit val assetDecoder: Decoder[Asset] = deriveDecoder - implicit val assetFieldsDecoder: Decoder[AssetFields] = deriveConfiguredDecoder + implicit val assetFieldsDecoder: Decoder[AssetFields] = deriveDecoder[AssetFields].prepare(renameStartAndEndFields) implicit val cartoonVariantDecoder: Decoder[CartoonVariant] = deriveDecoder implicit val cartoonImageDecoder: Decoder[CartoonImage] = deriveDecoder implicit val elementDecoder: Decoder[Element] = deriveDecoder @@ -122,14 +133,7 @@ object CirceDecoders { implicit val standardElementFieldsDecoder: Decoder[StandardElementFields] = deriveDecoder implicit val witnessElementFieldsDecoder: Decoder[WitnessElementFields] = deriveDecoder implicit val richLinkElementFieldsDecoder: Decoder[RichLinkElementFields] = deriveDecoder - implicit val renameDatesConfiguration: Configuration = Configuration.default.copy( - transformMemberNames = { - case "startDate" => "start" - case "endDate" => "end" - case x => x - } - ) - implicit val membershipElementFieldsDecoder: Decoder[MembershipElementFields] = deriveConfiguredDecoder + implicit val membershipElementFieldsDecoder: Decoder[MembershipElementFields] = deriveDecoder[MembershipElementFields].prepare(renameStartAndEndFields) implicit val embedElementFieldsDecoder: Decoder[EmbedElementFields] = deriveDecoder implicit val instagramElementFieldsDecoder: Decoder[InstagramElementFields] = deriveDecoder implicit val commentElementFieldsDecoder: Decoder[CommentElementFields] = deriveDecoder diff --git a/json/src/main/scala/com/gu/contentapi/json/CirceEncoders.scala b/json/src/main/scala/com/gu/contentapi/json/CirceEncoders.scala index 342f994..44176ae 100644 --- a/json/src/main/scala/com/gu/contentapi/json/CirceEncoders.scala +++ b/json/src/main/scala/com/gu/contentapi/json/CirceEncoders.scala @@ -8,8 +8,6 @@ import com.gu.contententity.thrift.entity._ import com.gu.contententity.{thrift => contententity} import com.twitter.scrooge.ThriftEnum import io.circe._ -import io.circe.generic.extras._ -import io.circe.generic.extras.semiauto.deriveConfiguredEncoder import io.circe.generic.semiauto._ import io.circe.syntax._ import com.gu.fezziwig.CirceScroogeWhiteboxMacros._ @@ -47,6 +45,19 @@ object CirceEncoders { ) } + def renameStartAndEndFields(j: Json): Json = { + j.mapObject { x => + (for { + endDate <- x("endDate") + startDate <- x("startDate") + } yield { + x + .remove("endDate").add("end", endDate) + .remove("startDate").add("start", startDate) + }).getOrElse(x) + } + } + implicit val contentFieldsEncoder: Encoder[ContentFields] = deriveEncoder implicit val editionEncoder: Encoder[Edition] = deriveEncoder implicit val sponsorshipEncoder: Encoder[Sponsorship] = deriveEncoder @@ -56,7 +67,7 @@ object CirceEncoders { implicit val podcastEncoder: Encoder[Podcast] = deriveEncoder implicit val podcastCategoryEncoder: Encoder[PodcastCategory] = deriveEncoder implicit val assetEncoder: Encoder[Asset] = deriveEncoder - implicit val assetFieldsEncoder: Encoder[AssetFields] = deriveConfiguredEncoder + implicit val assetFieldsEncoder: Encoder[AssetFields] = deriveEncoder[AssetFields].mapJson(renameStartAndEndFields(_)) implicit val cartoonVariantEncoder: Encoder[CartoonVariant] = deriveEncoder implicit val cartoonImageEncoder: Encoder[CartoonImage] = deriveEncoder implicit val elementEncoder: Encoder[Element] = deriveEncoder @@ -72,14 +83,7 @@ object CirceEncoders { implicit val standardElementFieldsEncoder: Encoder[StandardElementFields] = deriveEncoder implicit val witnessElementFieldsEncoder: Encoder[WitnessElementFields] = deriveEncoder implicit val richLinkElementFieldsEncoder: Encoder[RichLinkElementFields] = deriveEncoder - implicit val renameDatesConfiguration: Configuration = Configuration.default.copy( - transformMemberNames = { - case "startDate" => "start" - case "endDate" => "end" - case x => x - } - ) - implicit val membershipElementFieldsEncoder: Encoder[MembershipElementFields] = deriveConfiguredEncoder + implicit val membershipElementFieldsEncoder: Encoder[MembershipElementFields] = deriveEncoder[MembershipElementFields].mapJson(renameStartAndEndFields(_)) implicit val embedElementFieldsEncoder: Encoder[EmbedElementFields] = deriveEncoder implicit val instagramElementFieldsEncoder: Encoder[InstagramElementFields] = deriveEncoder implicit val commentElementFieldsEncoder: Encoder[CommentElementFields] = deriveEncoder From 1efa264d046a265e9db4708069b5fba391d15d88 Mon Sep 17 00:00:00 2001 From: Emily Bourke Date: Fri, 3 Oct 2025 13:35:50 +0100 Subject: [PATCH 4/5] Add fresh tests for renamed fields MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit These tests exercise the case I mentioned in the last commit, by omitting the start date from the latter test’s json. Fixing the bug should fix this test, I’ll do that next. --- .../membership-element-no-start-date.json | 14 ++++++++++++++ .../resources/templates/membership-element.json | 15 +++++++++++++++ .../gu/contentapi/json/CirceRoundTripSpec.scala | 5 +++++ 3 files changed, 34 insertions(+) create mode 100644 json/src/test/resources/templates/membership-element-no-start-date.json create mode 100644 json/src/test/resources/templates/membership-element.json diff --git a/json/src/test/resources/templates/membership-element-no-start-date.json b/json/src/test/resources/templates/membership-element-no-start-date.json new file mode 100644 index 0000000..e33dd21 --- /dev/null +++ b/json/src/test/resources/templates/membership-element-no-start-date.json @@ -0,0 +1,14 @@ +{ + "response": { + "venue": "The Scott room", + "identifier": "guardian-live", + "image": "https://media.guim.co.uk/e50e166a08e4279c352d83fa2f3210186999bd13/0_586_2064_1239/500.jpg", + "price": "£10", + "linkText": "Guardian Live | Guardian Newsroom: Should the UK bomb Syria?", + "location": "The Guardian, Kings Place, 90 York Way, London, N1 9GU", + "end": "2015-09-30T19:30:00Z", + "originalUrl": "https://membership.theguardian.com/event/guardian-live-guardian-newsroom-should-the-uk-bomb-syria-18761779989", + "title": "Guardian Live | Guardian Newsroom: Should the UK bomb Syria?", + "linkPrefix": "Membership Event: " + } +} diff --git a/json/src/test/resources/templates/membership-element.json b/json/src/test/resources/templates/membership-element.json new file mode 100644 index 0000000..c548bb3 --- /dev/null +++ b/json/src/test/resources/templates/membership-element.json @@ -0,0 +1,15 @@ +{ + "response": { + "venue": "The Scott room", + "identifier": "guardian-live", + "image": "https://media.guim.co.uk/e50e166a08e4279c352d83fa2f3210186999bd13/0_586_2064_1239/500.jpg", + "price": "£10", + "start": "2015-09-30T18:00:00Z", + "linkText": "Guardian Live | Guardian Newsroom: Should the UK bomb Syria?", + "location": "The Guardian, Kings Place, 90 York Way, London, N1 9GU", + "end": "2015-09-30T19:30:00Z", + "originalUrl": "https://membership.theguardian.com/event/guardian-live-guardian-newsroom-should-the-uk-bomb-syria-18761779989", + "title": "Guardian Live | Guardian Newsroom: Should the UK bomb Syria?", + "linkPrefix": "Membership Event: " + } +} diff --git a/json/src/test/scala/com/gu/contentapi/json/CirceRoundTripSpec.scala b/json/src/test/scala/com/gu/contentapi/json/CirceRoundTripSpec.scala index 0026c65..184d3da 100644 --- a/json/src/test/scala/com/gu/contentapi/json/CirceRoundTripSpec.scala +++ b/json/src/test/scala/com/gu/contentapi/json/CirceRoundTripSpec.scala @@ -167,6 +167,11 @@ class CirceRoundTripSpec extends FlatSpec with Matchers { checkRoundTrip[EntitiesResponse]("entities.json") } + it should "serialise and deserialise the renamed fields “startDate” and “endDate” as “start” and “end” respectively" in { + checkRoundTrip[MembershipElementFields]("membership-element.json") + checkRoundTrip[MembershipElementFields]("membership-element-no-start-date.json") + } + def checkRoundTrip[T : Decoder : Encoder]( jsonFileName: String, transformBeforeDecode: Json => Json = identity, From 622b10f52c2f5d9a64af7f016bf56cdae408afaf Mon Sep 17 00:00:00 2001 From: Emily Bourke Date: Fri, 3 Oct 2025 15:39:55 +0100 Subject: [PATCH 5/5] Allow renamed fields to be missing This commit fixes the renaming of fields by still working if one of them is missing. (Replacing the single for-expression with two composed .maps.) --- .../com/gu/contentapi/json/CirceDecoders.scala | 12 ++++-------- .../com/gu/contentapi/json/CirceEncoders.scala | 16 +++++----------- 2 files changed, 9 insertions(+), 19 deletions(-) diff --git a/json/src/main/scala/com/gu/contentapi/json/CirceDecoders.scala b/json/src/main/scala/com/gu/contentapi/json/CirceDecoders.scala index 67ea232..e616b78 100644 --- a/json/src/main/scala/com/gu/contentapi/json/CirceDecoders.scala +++ b/json/src/main/scala/com/gu/contentapi/json/CirceDecoders.scala @@ -95,16 +95,12 @@ object CirceDecoders { } } + def renameField(oldName: String, newName: String): JsonObject => JsonObject = + o => o(oldName).fold(o)(v => o.remove(oldName).add(newName, v)) + def renameStartAndEndFields(c: ACursor): ACursor = { c.withFocus { - _.mapObject { x => - (for { - endDate <- x("end") - startDate <- x("start") - } yield { - x.add("endDate", endDate).add("startDate", startDate) - }).getOrElse(x) - } + _.mapObject (renameField("end", "endDate").andThen(renameField("start", "startDate"))) } } diff --git a/json/src/main/scala/com/gu/contentapi/json/CirceEncoders.scala b/json/src/main/scala/com/gu/contentapi/json/CirceEncoders.scala index 44176ae..22c1b14 100644 --- a/json/src/main/scala/com/gu/contentapi/json/CirceEncoders.scala +++ b/json/src/main/scala/com/gu/contentapi/json/CirceEncoders.scala @@ -45,17 +45,11 @@ object CirceEncoders { ) } - def renameStartAndEndFields(j: Json): Json = { - j.mapObject { x => - (for { - endDate <- x("endDate") - startDate <- x("startDate") - } yield { - x - .remove("endDate").add("end", endDate) - .remove("startDate").add("start", startDate) - }).getOrElse(x) - } + def renameField(oldName: String, newName: String): JsonObject => JsonObject = + o => o(oldName).fold(o)(v => o.remove(oldName).add(newName, v)) + + def renameStartAndEndFields: Json => Json = { + _.mapObject(renameField("endDate", "end").andThen(renameField("startDate", "start"))) } implicit val contentFieldsEncoder: Encoder[ContentFields] = deriveEncoder