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

use circe as serde lib for json: 200 #214

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

Conversation

TebaleloS
Copy link
Collaborator

@TebaleloS TebaleloS commented Jun 18, 2024

Release notes:

  • Added Circe Json serde module library for Json for serialization.
  • Added 'Circe generic` in the serde dependencies.
  • Added CirceJsonImplicit class and defined implicit conversion for DTO's.
  • Integrated Circe with the server module.
  • Removed play Json as part of the server dependencies.
  • Used for Circe to implicitly convert DTO's to Json throughout the project.
  • Modified the test cases to suit the Circe format in the for http.
  • Replaced jsonbArrayPutUsingString with jsonbArrayPut in DoobieImplicits for both Json and Jsonb objects.
  • Reimplemented SerislizationUtils using circe format.
  • Replaced Json4s with circe in the agent module.
  • Modified SerializationUtilsUnitTest.

closes #200

@TebaleloS TebaleloS self-assigned this Jun 18, 2024
@TebaleloS TebaleloS added the work in progress Work on this item is not yet finished (mainly intended for PRs) label Jun 18, 2024
Copy link

github-actions bot commented Jun 26, 2024

JaCoCo model module code coverage report - scala 2.13.11

Overall Project 59.93% -92.06%
Files changed 14.72%

File Coverage
SerializationUtils.scala 26.37% -259.34%
ZonedDateTimeSerializer.scala 0% -475%

Copy link

github-actions bot commented Jun 26, 2024

JaCoCo agent module code coverage report - scala 2.13.11

Overall Project 83.19% 🍏
File Coverage
HttpDispatcher.scala 38.14% 🍏

Copy link

github-actions bot commented Jun 26, 2024

JaCoCo server module code coverage report - scala 2.13.11

Overall Project 64.14% -77.25% 🍏
Files changed 71.09%

File Coverage
CirceJsonImplicits.scala 100% -1081.25% 🍏
FlowControllerImpl.scala 100% 🍏
BaseController.scala 100% 🍏
PartitioningForDB.scala 95.45% -381.82%
PartitioningControllerImpl.scala 88.89% 🍏
CheckpointFromDB.scala 84.35% 🍏
WriteCheckpoint.scala 77.52% 🍏
CheckpointControllerImpl.scala 76% 🍏
GetPartitioningMeasures.scala 71.43% 🍏
GetPartitioningAdditionalData.scala 70.82% 🍏
CreateOrUpdateAdditionalData.scala 69.79% -37.5%
CreatePartitioningIfNotExists.scala 69.47% -37.89%
ErrorResponse.scala 69.39% -2932.65%
GetPartitioningCheckpoints.scala 54.75% 🍏
GetFlowCheckpoints.scala 0% -2.26%

@TebaleloS TebaleloS added no RN No release notes required and removed no RN No release notes required labels Jun 27, 2024
@TebaleloS
Copy link
Collaborator Author

Release notes:

Added Circe Json serde module library for Json for serialization.
Added 'Circe generic` in the serde dependencies.
Added CirceJsonImplicit class and defined implicit conversion for DTO's.
Integrated Circe with the server module.
Removed play Json as part of the server dependencies.
Used for Circe to implicitly convert DTO's to Json throughout the project.
Modified the test cases to suit the Circe format in the for http.
Replaced jsonbArrayPutUsingString with jsonbArrayPut in DoobieImplicits for both Json and Jsonb objects.
Reimplemented SerislizationUtils using circe format.
Replaced Json4s usage with circe in the agent module.
Modified SerializationUtilsUnitTest.

@TebaleloS TebaleloS marked this pull request as ready for review June 27, 2024 08:39
@TebaleloS TebaleloS removed the work in progress Work on this item is not yet finished (mainly intended for PRs) label Jun 27, 2024
import java.time.format.DateTimeFormatter
import java.util.UUID

object SerializationUtils {
Copy link
Collaborator

@salamonpavel salamonpavel Jun 28, 2024

Choose a reason for hiding this comment

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

The whole object SerializationUtils should be removed (or kept only for third party types serde implicits). It's customary to define serde implicits in companion objects - for default de/serialization. If needed one could then provide alternative implementation(s) either in direct scope where the de/serialization is performed or create yet another object for non-default implementations inside the companion object and in import into direct scope only the desired implementation (the preffered approach in my opinion).

A simple example would be:

import io.circe.{Decoder, Encoder, HCursor, Json}
import io.circe.generic.semiauto.{deriveDecoder, deriveEncoder}

case class Person(name: String, age: Int)

object Person {
  implicit val defaultPersonEncoder: Encoder[Person] = deriveEncoder[Person]
  implicit val defaultPersonDecoder: Decoder[Person] = deriveDecoder[Person]

  object JsonImplicits {
    implicit val nonDefaultPersonEncoder: Encoder[Person] = new Encoder[Person] {
      final def apply(a: Person): Json = Json.obj(
        ("name", Json.fromString(a.name)),
        ("isAdult", Json.fromBoolean(a.age >= 18))
      )
    }

    implicit val nonDefaultPersonDecoder: Decoder[Person] = new Decoder[Person] {
      final def apply(c: HCursor): Decoder.Result[Person] =
        for {
          name <- c.downField("name").as[String]
          isAdult <- c.downField("isAdult").as[Boolean]
        } yield {
          val age = if (isAdult) 18 else 17
          Person(name, age)
        }
    }
  }
}

Usage would look like this:

import io.circe.syntax._
import io.circe.parser._

val person = Person("John Doe", 20)

// Using default serde from companion (always available in direct scope without the need to import it)
val defaultJson = person.asJson // implicitly looked up from companion Person.defaultPersonEncoder

// Using non-default serde, passed implicitly
import Person.JsonImplicits.nonDefaultPersonEncoder
val nonDefaultJson = person.asJson

// Using non-default serde, passed explicitly
val nonDefaultJson = person.asJson(Person.JsonImplicits.nonDefaultPersonEncoder)

@@ -107,26 +90,19 @@ object Dependencies {
}

private def jsonSerdeDependencies(scalaVersion: Version): Seq[ModuleID] = {
val json4sVersion = Versions.json4s(scalaVersion)

lazy val jacksonModuleScala = "com.fasterxml.jackson.module" %% "jackson-module-scala" % Versions.jacksonModuleScala
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we still need this?

import java.time.ZonedDateTime
import java.time.format.DateTimeFormatter

object ZonedDateTimeSerializer {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's not used anywhere anymore and could be removed.

Put.Advanced
.other[PGobject](
NonEmptyList.of("json[]")
)
.tcontramap { a =>
val o = new PGobject
o.setType("json[]")
o.setValue(a.mkString("{", ",", "}"))
val arrayElements = a.map { x =>
Copy link
Collaborator

Choose a reason for hiding this comment

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

You could define private method in DoobieImplicits object as below in order to avoid repetition.

private def circeJsonListToPGJsonArrayString(jsonList: List[Json]): String = {
    val arrayElements = jsonList.map { x =>
      // Convert to compact JSON string and escape inner quotes
      val escapedJsonString = x.noSpaces.replace("\"", "\\\"")
      // Wrap in double quotes for the array element
      s"\"$escapedJsonString\""
    }

    // Join all elements into a single string wrapped in curly braces
    arrayElements.mkString("{", ",", "}")
  }

Usage:

implicit val jsonArrayPut: Put[List[Json]] = {
      Put.Advanced
        .other[PGobject](
          NonEmptyList.of("json[]")
        )
        .tcontramap { a =>
          val o = new PGobject
          o.setType("json[]")
          o.setValue(circeJsonListToPGJsonArrayString(a))
          o
        }
    }

import za.co.absa.atum.model.dto.MeasureResultDTO.ResultValueType
import za.co.absa.atum.model.dto._

object CirceJsonImplicits {
Copy link
Collaborator

@salamonpavel salamonpavel Jun 28, 2024

Choose a reason for hiding this comment

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

This object shouldn't exist. Encoder/Decoder instances should be defined in companions of given types or in either separate objects for third party types (or in SerializationUtils) but anyway it belongs to model module, not in module package in server module.
Previously we had data serialized with different libraries in agent and server modules, that's why we had to have these "*JsonImplicits" objects as we had to define the serde implicits outside of model module. That's not the case anymore.

keys: Seq[String],
keysToValues: Map[String, String]
)
version: Int = 1,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please fix the formatting.

@@ -53,7 +54,7 @@ object WriteCheckpointIntegrationTests extends ConfigProviderTest {
).provide(
WriteCheckpoint.layer,
PostgresDatabaseProvider.layer,
TestTransactorProvider.layerWithRollback
TestTransactorProvider.layerWithoutRollback
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add back the layer with rollback. These are the changes you have made when we were debugging the code on the call, but it should be reverted.

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

Successfully merging this pull request may close these issues.

Use Circe as serde library for JSON in Agent, Server and Model modules
2 participants