#188: new Flow service in API v2

[lsulak]

• created new controller, service, and repository: Flow
• new REST API, v2: get checkpoints (with measurements) of a flow, based on a partitioning
• tested with Postman and wrote many integration tests as well as unit tests
• APIv2: following kebab-case format for API paths, APIv1: keeping camelCase for backward compatibility

Closes #188

[github-actions]

JaCoCo server module code coverage report - scala 2.13.11

Build Failed

[lsulak]

@salamonpavel I was thinking what to do here, perhaps this is not 'best-zio-practice' - would you have a better idea how to handle this? Also, check FlowServiceImpl where it is used please

[salamonpavel]

I think you can and should return Either instead of throwing exception which in this version of code in not handled properly. I would go with this code (only small change of yours).

def toCheckpointDTO(
    partitioning: PartitioningDTO,
    checkpointQueryResult: CheckpointFromDB
  ): Either[DecodingFailure, CheckpointDTO] = {
    val measureResultOrErr = checkpointQueryResult.measurementValue.as[MeasureResultDTO]

    measureResultOrErr match {
      case Left(err) => Left(err)
      case Right(measureResult) =>
        Right(
          CheckpointDTO(
            id = checkpointQueryResult.idCheckpoint,
            name = checkpointQueryResult.checkpointName,
            author = checkpointQueryResult.author,
            measuredByAtumAgent = checkpointQueryResult.measuredByAtumAgent,
            partitioning = partitioning,
            processStartTime = checkpointQueryResult.checkpointStartTime,
            processEndTime = checkpointQueryResult.checkpointEndTime,
            measurements = Set(
              MeasurementDTO(
                measure = MeasureDTO(
                  measureName = checkpointQueryResult.measureName,
                  measuredColumns = checkpointQueryResult.measuredColumns
                ),
                result = measureResult
              )
            )
          )
        )
    }
  }

[salamonpavel]

This method is not needed. You can directly deserialize into MeasureResultDTO in toCheckpointDTO method.

checkpointQueryResult.measurementValue.as[MeasureResultDTO]

[lsulak]

hmm, that's true now. I had a few iterations of this code - thanks! Will change it

[salamonpavel]

This method is not needed. You can directly deserialize into MeasureResultDTO in toCheckpointDTO method.

checkpointQueryResult.measurementValue.as[MeasureResultDTO]

[salamonpavel]

In this version the exception coming from the deserialization is not handled. In some other comment I am mentioning you should return rather either in the serde code. Then you could create zios from those eithers and collect them. See bellow two version of the same, one with sequential processing (the one with foreach), another one with parallel processing (collectAll). I would personally choose the foreach variant.

def getFlowCheckpointsCollectAll(checkpointQueryDTO: CheckpointQueryDTO): IO[ServiceError, Seq[CheckpointDTO]] = {
    for {
      checkpointsFromDB <- repositoryCall(flowRepository.getFlowCheckpoints(checkpointQueryDTO), "getFlowCheckpoints")
      checkpointDTOs <- ZIO.collectAll {
        checkpointsFromDB.map { checkpointFromDB =>
          ZIO.fromEither(CheckpointFromDB.toCheckpointDTO(checkpointQueryDTO.partitioning, checkpointFromDB))
            .mapError(error => ServiceError(error.getMessage))
        }
      }
    } yield checkpointDTOs
  }

  def getFlowCheckpointsForeach(checkpointQueryDTO: CheckpointQueryDTO): IO[ServiceError, Seq[CheckpointDTO]] = {
    for {
      checkpointsFromDB <- repositoryCall(flowRepository.getFlowCheckpoints(checkpointQueryDTO), "getFlowCheckpoints")
      checkpointDTOs <- ZIO.foreach(checkpointsFromDB) { checkpointFromDB =>
          ZIO.fromEither(CheckpointFromDB.toCheckpointDTO(checkpointQueryDTO.partitioning, checkpointFromDB))
            .mapError(error => ServiceError(error.getMessage))
      }
    } yield checkpointDTOs
  }

[salamonpavel]

Actually now reading the documentation I can see that you could also use foreachPar for parallel processing. So the difference between foreach(Par) and collectAll is mainly in the fact that collectAll takes sequence of effects on the input whereas foreach takes a normal collection and function to convert the elements into zio. The return value is the same, and both return failed effect if any of the zios fail.

[lsulak]

Thanks, it's quite educative, I'll give it some reading.

I think I'll stick with the sequential processing - the only 'parallelism' would be what happens in toCheckpointDTO and that's not that 'slow' (i.e. I like performance / multiprocessing optimizations where it significantly impacts performance, on the other hand if it doesn't, parallelism can introduce additional overhead and, god forbids, debugging of problems is a bit more difficult)

[salamonpavel]

What's the motivation to place json related encoders/decoders alongside doobie implicits?

[lsulak]

Not sure I understand the question, those JSON related SerDe code was already there & I needed those MeasureResult DTOs to be serialized/deserialized as well

[salamonpavel]

DoobieImplicits object is there for defining Put/Get/Read/Write instances for Doobie. Then we have PlayJsonImplicits for Reads/Writes/Format type classes for Play Json. And what you have defined is actually related to Circe.

[lsulak]

Aaah. Yes, I'm sorry I understand now. I'll move them to CirceImplicits.scala

I know that I could move them directly to CheckpointFromDB.scala, but I anticipate that @TebaleloS will create a bunch of them later as well, so it might be a good idea for them to be centralized

[salamonpavel]

It's customary to place them in companion objects.

[lsulak]

moved into companion object of a given DTO, thanks for the recommendation

[salamonpavel]

Maybe we could already serialize it into Json from Circe instead of using String derived by play json.

[lsulak]

I don't want to do that in this PR, it's been open for far too long and we have a ticket for this already. Let's make it later & in one bunch, not in pieces in these feature PRs I think

#200

[salamonpavel]

Let's merge #199 before this PR so you can incorporate the envelope.

[lsulak]

Okay, happy to do that - I approved #199 just now

[salamonpavel]

It's a unit test and should be executed as such. Please rename to FlowControllerUnitTests.

[lsulak]

sure

[salamonpavel]

Remove the annotation and make it an object. Also as above, it's a unit test and should be executed as such. Please rename the object to FlowRepositoryUnitTests

[lsulak]

Okay, I'll actually make these changes in the whole repo. I was not paying particular attention to it, but it's time to change it

[salamonpavel]

@lsulak
Maybe you could also rename all other test files where the suffix 'IntegrationTests' was incorrectly used?

[lsulak]

naturally, I also did it already :)

[salamonpavel]

As above.

[salamonpavel]

This PR showcase high-quality code that adheres to best practices and coding standards. However, it would be beneficial to add automated tests for newly created endpoint. While manual testing via tools like Postman is useful for exploratory testing and very specific scenarios, it cannot replace automated tests.

[lsulak]

This PR showcase high-quality code that adheres to best practices and coding standards. However, it would be beneficial to add automated tests for newly created endpoint. While manual testing via tools like Postman is useful for exploratory testing and very specific scenarios, it cannot replace automated tests.

Thanks, appreciate it!

Re API tests: actually it's a good idea. Ticket here: #210 for the future as it's quite difficult to implement all the good ideas immediately :D

[lsulak]

Release notes

• Implements a new REST API endpoint: POST api/v2/get-flow-checkpoints for getting checkpoints (with measurements) of a flow; flow being identifiable based on an input partitioning (v2).
• Using kebab-case convention for our API paths in v2 (v1 is kept camelCase as it was before, for backward compatibility reasons).

[salamonpavel]

This PR showcase high-quality code that adheres to best practices and coding standards. However, it would be beneficial to add automated tests for newly created endpoint. While manual testing via tools like Postman is useful for exploratory testing and very specific scenarios, it cannot replace automated tests.

Thanks, appreciate it!

Re API tests: actually it's a good idea. Ticket here: #210 for the future as it's quite difficult to implement all the good ideas immediately :D

There are also these unit tests for endpoints that could be implemented also for the new endpoint.

[lsulak]

This PR showcase high-quality code that adheres to best practices and coding standards. However, it would be beneficial to add automated tests for newly created endpoint. While manual testing via tools like Postman is useful for exploratory testing and very specific scenarios, it cannot replace automated tests.

Thanks, appreciate it!
Re API tests: actually it's a good idea. Ticket here: #210 for the future as it's quite difficult to implement all the good ideas immediately :D

There are also these unit tests for endpoints that could be implemented also for the new endpoint.

I personally don't like covering everything with all types of tests - but in this case I'll add them, perhaps it's a good enough balance to have at least 1 test for each endpoint type / service, and since here I introduced 'flow' service / functionality, it might be nice to have it.