#245 Add the ability to query REST endpoints from Reader module #297

benedeki · 2024-11-01T08:29:24Z

Implemented Reader and ReaderWithPartitioningId base classes for all readers - they are to bringAtum server connectivity/querying ability
Created 'ServerConfig' case class representing values needed to connect to an Atum Server
RequestResult[R] represents an Atum server query response.
Offered implicits for MonadError type class needed for Reader and ReaderWithPartitioningId - there are Future, Cats IO and Zio Task ones (the last one available only in Scala 2.13)
AtumPartitions and AdditionalData moved from Agent to Module
ErrorResponse received a method to decode from Json based on http status code

Closes #245

Depends on #300

Release Notes:

Reader module now supports connection to Atum server.

* created new module Info * the new modul added to JaCoco and CI routines

* JaCoCo exclusion for model

* created Provider to query the data from server * support for Future, IO, and ZIO based providers * work in progress

* fixed license headers

github-actions · 2024-11-01T12:19:57Z

JaCoCo model module code coverage report - scala 2.13.11

Overall Project	61.27% `-35.88%`	🍏
Files changed	66.75%	❌

File	Coverage
JsonSyntaxExtensions.scala	94.34% `-5.66%`	🍏
ErrorResponse.scala	89.66%	❌
basic.scala	47.17%	❌

github-actions · 2024-11-01T12:19:58Z

JaCoCo agent module code coverage report - scala 2.13.11

Overall Project	78.2%	🍏
Files changed	100%	🍏

File	Coverage
AtumAgent.scala	97.13%	🍏
AtumContext.scala	91.79%	🍏

github-actions · 2024-11-01T12:19:59Z

JaCoCo reader module code coverage report - scala 2.13.11

Overall Project	88.71%	🍏
Files changed	40.19%	❌

File	Coverage
PartitioningReader.scala	100%	❌
Reader.scala	100% `-14.75%`	🍏
ReaderWithPartitioningId.scala	100%	🍏
io.scala	100%	🍏
future.scala	100%	🍏
ServerConfig.scala	88.24%	❌
FlowReader.scala	80% `-20%`	🍏
RequestResult.scala	12.5%	❌
zio.scala	0%	❌

github-actions · 2024-11-01T12:20:00Z

JaCoCo server module code coverage report - scala 2.13.11

Overall Project	68.89%	🍏

There is no coverage information present for the Files changed

benedeki · 2024-11-06T12:09:37Z

reader/src/main/scala/za/co/absa/atum/reader/server/GenericServerConnection.scala

+
+  protected def executeRequest[R](request: RequestT[Identity, RequestResult[R], Any]): F[Response[RequestResult[R]]]
+
+  def getQuery[R: Decoder](endpointUri: String, params: Map[String, String] = Map.empty): F[RequestResult[R]] = {


Not sure about the return type of this key function. Particularly the error case. But it can be relatively easily adjusted in future.

…endpoints-from-info-module

benedeki · 2024-11-22T20:58:36Z

This item depends on:

~~#291: Project improvements after 0.3.0 release #300~~
By Dependent Issues.

…endpoints-from-info-module

salamonpavel · 2024-11-25T11:50:35Z

agent/README.md


 ### Option 1
 ```scala
 val atumContextInstanceWithRecordCount = AtumContext(processor = processor)
-  .withMeasureAdded(RecordCount(MockMeasureNames.recordCount1, measuredColumn = "id"))
+  .withMeasureAdded(RecordCount(MockMeasureNames.recordCount1))


I am curious why you changed it to MockMeasureNames.

I didn't change the code. But I believe the example in the README was outdated, so I fixed it.

Possibly IntelliJ refactoring in the past, should be changed though

salamonpavel · 2024-11-25T11:56:15Z

model/src/main/scala/za/co/absa/atum/model/types/basic.scala

+import za.co.absa.atum.model.dto.{PartitionDTO, PartitioningDTO}
+
+
+object basic {


Good idea to move this into its dedicated object instead of the AtumContext. The Ops are also useful.

salamonpavel · 2024-11-25T12:01:08Z

model/src/main/scala/za/co/absa/atum/model/utils/JsonSyntaxExtensions.scala

      }
    }

+    def asSafe[T: Decoder]: Either[io.circe.Error, T] = {


You could make this method private.

I think I originally used it in Reader, but seems dropped it since. Will make it private 👍

I think it has only 1 usecase right now? And it's a 1 liner, so why to have it as a separate method?

salamonpavel · 2024-11-25T12:04:00Z

model/src/test/scala/za/co/absa/atum/testing/implicits/StringImplicits.scala

+ * limitations under the License.
+ */
+
+package za.co.absa.atum.model.testing.implicits


The package is incorrect. It's not inside model.

Thank you. Imperfect refactoring 😉

salamonpavel · 2024-11-25T12:18:27Z

reader/src/main/scala-2.13/za/co/absa/atum/reader/implicits/zio.scala

+import sttp.client3.impl.zio.RIOMonadAsyncError
+
+object zio {
+  implicit val ZIOMonad: RIOMonadAsyncError[Any] = new RIOMonadAsyncError[Any]


Please rename this. It's confusing. I would personally stick to its name rioMonadAsyncError.

salamonpavel · 2024-11-25T12:23:34Z

reader/src/main/scala/za/co/absa/atum/reader/implicits/io.scala

+import sttp.client3.impl.cats.CatsMonadAsyncError
+
+object io {
+  implicit val CatsIOMonad: CatsMonadAsyncError[IO] = new CatsMonadAsyncError[IO]


Same as with the ZIO counterpart, please rename this.

salamonpavel · 2024-11-25T12:24:45Z

reader/src/test/scala-2.13/za/co/absa/atum/reader/basic/Reader_ZIOUnitTests.scala

+// This test is disabled as is breaks on JaCoCo execution
+// Once the problem is figured out or how to cirmvent it, this can be re-enabled
+//
+//object Reader_ZIOUnitTests extends ZIOSpecDefault {


This test doesn't work. Please address this already as part of this PR.

The error message indicates that there is a NoSuchMethodError related to the zio.package$.RIO() method. This typically happens when there is a version mismatch between the ZIO library and the code that depends on it.

salamonpavel · 2024-11-25T12:25:32Z

reader/src/test/scala/za/co/absa/atum/reader/basic/Reader_CatsIOUnitTests.scala

+    assert(result == Right(partitionDTO))
+
+
+//      .map { result =>


clean up needed here :)

salamonpavel · 2024-11-25T12:28:32Z

reader/src/main/scala/za/co/absa/atum/reader/basic/Reader.scala

+  }
+}
+
+object Reader {


empty object could be removed

salamonpavel · 2024-11-25T12:48:52Z

reader/src/main/scala/za/co/absa/atum/reader/FlowReader.scala

+ *                              bounds, as it make the imports easier to follow
+ * @tparam F                    - the effect type (e.g. Future, IO, Task, etc.)
+ */
+class FlowReader[F[_]](val mainFlowPartitioning: AtumPartitions)


what's the point of the public val?

It's the identifier of the FlowReader. So having a collection of FlowReaders would allow a distinction among them.

Sure, that I understand, but I wonder why it needs to be public val? It would be reasonable if you were going to use the trait PartitioningIdProvider and by using public val partitioning in the constructor you provided the requirement for def partitioning: AtumPartitions.

salamonpavel · 2024-11-25T13:08:15Z

reader/src/main/scala/za/co/absa/atum/reader/basic/ReaderWithPartitioningId.scala

+import za.co.absa.atum.reader.basic.RequestResult.RequestResult
+import za.co.absa.atum.reader.server.ServerConfig
+
+abstract class ReaderWithPartitioningId[F[_]: MonadError](implicit serverConfig: ServerConfig, backend: SttpBackend[F, Any])


trait PartitioningIdProvider[F[_]] { self: Reader[F] => def partitioning: AtumPartitions def partitioningId()(implicit monad: MonadError[F]): F[RequestResult[Long]] = { val encodedPartitioning = partitioning.toPartitioningDTO.asBase64EncodedJsonString val queryResult = getQuery[SingleSuccessResponse[PartitioningWithIdDTO]]("/api/v2/partitionings", Map("partitioning" -> encodedPartitioning)) queryResult.map { result => result.map(_.data.id) } } }

By defining PartitioningIdProvider as a trait, you can mix this functionality into any class that needs it without being constrained by inheritance. This makes the code more flexible and reusable. The self-type constraint (self: Reader[F] =>) ensures that PartitioningIdProvider can only be mixed into classes that also mix in Reader[F]. This guarantees that the getQuery method from Reader[F] is available in PartitioningIdProvider, eliminating the need for abstract methods and making the code more cohesive.

case class PartitioningReader[F[_]](partitioning: AtumPartitions) (implicit serverConfig: ServerConfig, backend: SttpBackend[F, Any], ev: MonadError[F]) extends Reader[F] with PartitioningIdProvider[F] { def foo(): String = { // just to have some testable content "bar" } } class FlowReader[F[_]](val mainFlowPartitioning: AtumPartitions) (implicit serverConfig: ServerConfig, backend: SttpBackend[F, Any], ev: MonadError[F]) extends Reader[F] with PartitioningIdProvider[F] { override def partitioning: AtumPartitions = mainFlowPartitioning }

While I don't see the inheritance that much limiting here (not expecting a wide variety of successors, it's a cool code. Will switch to it. 👍

lsulak · 2024-11-25T15:47:58Z

.github/workflows/jacoco_report.yml

+          echo "Total 'model' module coverage ${{ steps.jacoco-model.outputs.coverage-overall }}"
+          echo "Changed files of 'model' module coverage ${{ steps.jacoco-model.outputs.coverage-changed-files }}"
+          echo "Total 'server' module coverage ${{ steps.jacoco-server.outputs.coverage-overall }}"
+          echo "Changed files of'server' module coverage ${{ steps.jacoco-server.outputs.coverage-changed-files }}"


Suggested change

echo "Changed files of'server' module coverage ${{ steps.jacoco-server.outputs.coverage-changed-files }}"

echo "Changed files of 'server' module coverage ${{ steps.jacoco-server.outputs.coverage-changed-files }}"

lsulak · 2024-11-25T15:48:53Z

README.md

@@ -247,6 +246,7 @@ Code coverage wil be generated on path:
 To make this project runnable via IntelliJ, do the following:
 - Make sure that your configuration in `server/src/main/resources/reference.conf` 
  is configured according to your needs
+- When building within the UI be sure to have the option `-language:higherKinds` on in the compiler options


I don't understand - what UI do you mean?

lsulak · 2024-11-25T15:55:23Z

model/src/main/scala/za/co/absa/atum/model/utils/JsonSyntaxExtensions.scala

        case Right(value) => value
-        case Left(error) => throw new RuntimeException(s"Failed to decode JSON: $error")
+        case Left(error) => throw error


why removing the prefix of the exception msg? I think it was quite useful, we would exactly know what this is coming from

benedeki added 12 commits October 31, 2024 01:23

#244: Create the Info module

b97b603

* created new module Info * the new modul added to JaCoco and CI routines

* fixed License headers

e623974

* renamed to _Reader_

5e4eadb

* README.md update

2e1e2ea

* fix

738c904

* JaCoCO action update

df8c9bd

* added dummy code for testing coverage

5affd82

* erroneous class renamed

0f1e121

* JaCoCo exclusion for model

* Deleted wrong files

d773a93

#245 Add the ability to query REST endpoints from Reader module

0776f9c

* created Provider to query the data from server * support for Future, IO, and ZIO based providers * work in progress

* Work still in progress

38fde1c

* the first working commit

1ac2233

benedeki added the work in progress Work on this item is not yet finished (mainly intended for PRs) label Nov 1, 2024

benedeki self-assigned this Nov 1, 2024

benedeki added 3 commits November 1, 2024 09:56

* Removed temporary notes

e6dcb52

* introduced MonadError into the GenericServerConnection

6968b02

* fixed license headers

* Fixed UTs

b9bacef

benedeki added 5 commits November 4, 2024 09:55

* trying to get rid of Java 11 dependency

bbb1e7f

* Downgraded sttpClient

33e6628

* further downgrade

f7ced56

* Removed exceptions

ca2116b

* commented out parts of README.md which are not yet part of the code

e5e6f63

benedeki commented Nov 6, 2024

View reviewed changes

benedeki marked this pull request as ready for review November 6, 2024 12:12

benedeki requested review from lsulak and TebaleloS as code owners November 6, 2024 12:12

benedeki and others added 5 commits November 17, 2024 04:56

* doc fix

7656f6f

Merge branch 'master' into feature/245-add-the-ability-to-query-rest-…

eb9a678

…endpoints-from-info-module

* disabled failing test

7641c07

* adjustments

bc82a5b

- further cleaning

0e7675e

benedeki added the dependent The item depends on some other open item (Issue or PR) label Nov 21, 2024

benedeki added 2 commits November 21, 2024 23:10

* tests progress

432716a

* several UTs added

11b0a16

benedeki removed the dependent The item depends on some other open item (Issue or PR) label Nov 22, 2024

Merge branch 'master' into feature/245-add-the-ability-to-query-rest-…

2c3f145

…endpoints-from-info-module

benedeki requested a review from ABLL526 as a code owner November 22, 2024 21:05

* last improvements before PR ready

e07dffb

benedeki removed the work in progress Work on this item is not yet finished (mainly intended for PRs) label Nov 24, 2024

benedeki added 2 commits November 25, 2024 10:57

* description to class ServerConfig

3955a50

* removed empty line

b287a66

salamonpavel reviewed Nov 25, 2024

View reviewed changes

lsulak reviewed Nov 25, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#245 Add the ability to query REST endpoints from Reader module #297

#245 Add the ability to query REST endpoints from Reader module #297

benedeki commented Nov 1, 2024 •

edited

Loading

github-actions bot commented Nov 1, 2024 •

edited

Loading

github-actions bot commented Nov 1, 2024 •

edited

Loading

github-actions bot commented Nov 1, 2024 •

edited

Loading

github-actions bot commented Nov 1, 2024 •

edited

Loading

benedeki Nov 6, 2024

benedeki commented Nov 22, 2024

salamonpavel Nov 25, 2024

benedeki Nov 25, 2024

lsulak Nov 25, 2024

salamonpavel Nov 25, 2024

salamonpavel Nov 25, 2024

benedeki Nov 25, 2024

lsulak Nov 25, 2024

salamonpavel Nov 25, 2024

benedeki Nov 25, 2024

salamonpavel Nov 25, 2024 •

edited

Loading

salamonpavel Nov 25, 2024

salamonpavel Nov 25, 2024

salamonpavel Nov 25, 2024

salamonpavel Nov 25, 2024

salamonpavel Nov 25, 2024

salamonpavel Nov 25, 2024

benedeki Nov 25, 2024

salamonpavel Nov 26, 2024

salamonpavel Nov 25, 2024 •

edited

Loading

benedeki Nov 25, 2024

lsulak Nov 25, 2024

lsulak Nov 25, 2024

lsulak Nov 25, 2024


		protected def executeRequest[R](request: RequestT[Identity, RequestResult[R], Any]): F[Response[RequestResult[R]]]

		def getQuery[R: Decoder](endpointUri: String, params: Map[String, String] = Map.empty): F[RequestResult[R]] = {

		import za.co.absa.atum.model.dto.{PartitionDTO, PartitioningDTO}


		object basic {

	echo "Changed files of'server' module coverage ${{ steps.jacoco-server.outputs.coverage-changed-files }}"
	echo "Changed files of 'server' module coverage ${{ steps.jacoco-server.outputs.coverage-changed-files }}"

#245 Add the ability to query REST endpoints from Reader module #297

Are you sure you want to change the base?

#245 Add the ability to query REST endpoints from Reader module #297

Conversation

benedeki commented Nov 1, 2024 • edited Loading

github-actions bot commented Nov 1, 2024 • edited Loading

JaCoCo model module code coverage report - scala 2.13.11

github-actions bot commented Nov 1, 2024 • edited Loading

JaCoCo agent module code coverage report - scala 2.13.11

github-actions bot commented Nov 1, 2024 • edited Loading

JaCoCo reader module code coverage report - scala 2.13.11

github-actions bot commented Nov 1, 2024 • edited Loading

JaCoCo server module code coverage report - scala 2.13.11

Choose a reason for hiding this comment

benedeki commented Nov 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

salamonpavel Nov 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

salamonpavel Nov 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benedeki commented Nov 1, 2024 •

edited

Loading

github-actions bot commented Nov 1, 2024 •

edited

Loading

github-actions bot commented Nov 1, 2024 •

edited

Loading

github-actions bot commented Nov 1, 2024 •

edited

Loading

github-actions bot commented Nov 1, 2024 •

edited

Loading

salamonpavel Nov 25, 2024 •

edited

Loading

salamonpavel Nov 25, 2024 •

edited

Loading