use official ZIO-lambda library in product-move-api #2115

johnduffell · 2023-11-23T20:01:27Z

I noticed that the docs endpoint for product-move-api was returning a 500 error

Failed with: Die(scala.MatchError: InputStreamRangeBody (of class sttp.tapir.RawBodyType$InputStreamRangeBody$),Stack trace for thread "zio-fiber-227": at com.gu.productmove.framework.TIOInterpreter.given_MonadError_TIO.$anon.flatMap(TIOInterpreter.scala:39) at com.gu.productmove.framework.TIOInterpreter.given_MonadError_TIO.$anon.handleWrappedError(TIOInterpreter.scala:46) at com.gu.productmove.framework.TIOInterpreter.given_MonadError_TIO.$anon.map(TIOInterpreter.scala:37) at com.gu.productmove.framework.TIOInterpreter.given_MonadError_TIO.$anon.flatMap(TIOInterpreter.scala:39) at com.gu.productmove.framework.TIOInterpreter.given_MonadError_TIO.$anon.map(TIOInterpreter.scala:37) at com.gu.productmove.framework.ZIOApiGatewayRequestHandler.handleWithLoggerAndErrorHandling(ZIOApiGatewayRequestHandler.scala:120) at com.gu.productmove.framework.ZIOApiGatewayRequestHandler.handleWithLoggerAndErrorHandling(ZIOApiGatewayRequestHandler.scala:121) at com.gu.productmove.framework.ZIOApiGatewayRequestHandler.handleWithLoggerAndErrorHandling(ZIOApiGatewayRequestHandler.scala:122))

When I looked into it, it was because I hadn't covered all the cases in the custom ZIO/aws lambda interpreter I wrote for tapir early last year.

Rather than adding the one case, I realised that a few months ago they added an official interpreter if we bumped to a later version of tapir. softwaremill/tapir#2975

So I switched over to the new one, which worked fine. I also had to change to the more standard ZIO Task rather than using our own custom TIO, which meant changing the error type from Any to Throwable. This might help our error stack traces, which can often be a bit unclear with other types.

Tested in CODE and working again
Feel free to take a look yourself at the docs https://product-move-api-code.support.guardianapis.com/docs/
Also tested the available moves endpoint
{{product-movement-baseurl}}/available-product-moves/A-S00737090
response data looks ok

[
    {
        "id": "2c92c0f84bbfec8b014bc655f4852d9d",
        "name": "Digital Pack",
        "billing": {
            "amount": 1499,
            "currency": {
                "code": "GBP",
                "symbol": "£"
            },
            "frequency": {
                "name": {
                    "month": {}
                },
                "count": 1
            },
            "startDate": "2024-03-27"
        },
        "trial": {
            "dayCount": 14
        },
        "introOffer": {
            "billing": {
                "percentage": 50,
                "startDate": "2023-12-28"
            },
            "duration": {
                "name": {
                    "month": {}
                },
                "count": 3
            }
        }
    }
]

johnduffell · 2023-11-23T20:02:27Z

build.sbt

@@ -568,6 +568,7 @@ lazy val `product-move-api` = lambdaProject(
    "com.softwaremill.sttp.tapir" %% "tapir-core" % tapirVersion,
    "com.softwaremill.sttp.tapir" %% "tapir-json-zio" % tapirVersion,
    "com.softwaremill.sttp.tapir" %% "tapir-aws-lambda" % tapirVersion,
+    "com.softwaremill.sttp.tapir" %% "tapir-aws-lambda-zio" % tapirVersion,


this is the new library added in softwaremill/tapir#2975

johnduffell · 2023-11-23T20:03:12Z

handlers/product-move-api/src/main/scala/com/gu/productmove/AwsCredentialsLive.scala

+  val layer: Layer[Throwable, AwsCredentialsProvider] =
+    ZLayer.scoped(ZIO.fromAutoCloseable(ZIO.attempt(impl)))


the official zio interpreter expects Throwable as the error, rather than Any, so I had to make everything comply. That is the majority of the changes.

johnduffell · 2023-11-23T20:03:42Z

handlers/product-move-api/src/main/scala/com/gu/productmove/Handler.scala

+  @main
+  // this will output the HTML to the console
+  def testDocsHtml(): Unit = {
+    Handler.runTest(
+      "GET",
+      "/docs/",
+      None,
+    )
+  }


new manual test so I could reproduce the issue locally.

johnduffell · 2023-11-23T20:06:48Z

handlers/product-move-api/src/main/scala/com/gu/productmove/SQS.scala

    ZLayer.scoped(for {
      stage <- ZIO.service[Stage]
      sqsClient <- initializeSQSClient().mapError(ex =>
-        InternalServerError(s"Failed to initialize SQS Client with error: $ex"),
+        new Throwable(s"Failed to initialize SQS Client with error", ex),


we were returning InternalServerError but I don't know if it was actually getting used directly, or just converted into an internal server error later anyway

Can you check? We obvs want to maintain the existing behaviour

yes that's a really good point I will check that for sure

yes, existing code we end up with Any as the failure type in the ZIO, and that ends up here in the code

support-service-lambdas/handlers/product-move-api/src/main/scala/com/gu/productmove/framework/ZIOApiGatewayRequestHandler.scala

Line 126 in 1b7e0ee

AwsResponse(false, 500, Map.empty, "")

So no matter whether we called it InternalServerError or BadRequest etc, it it would still be an internal server error.

If/where we map the error to a ZIO.success then we could use it as a response.
I think the new (official) way of using Throwable rather than my way of using Any as the error type is better as it's pretty clear that a Throwable will be a 500 error, and if we try to use a BadRequest or something, expecting it will really be a bad request, it won't compile unless we recover it into a success.

johnduffell · 2023-11-28T11:15:24Z

handlers/product-move-api/src/main/scala/com/gu/productmove/Handler.scala

+object Handler
+    extends ZIOApiGatewayRequestHandler(
+      List(
+        AvailableProductMovesEndpoint.server,
+        ProductMoveEndpoint.server,
+        UpdateSupporterPlusAmountEndpoint.server,
+        SubscriptionCancelEndpoint.server,
+      ),
+    )


I removed all the test stuff from the handler and made the endpoints a constructor parameter.

This was because with inheritance and vals I was getting issues with initialisation order, and server was coming out as null.
Rather than just going down the lazy val option (which worked) I decided to separate/simplify appropriately.

johnduffell · 2023-11-28T11:15:55Z

handlers/product-move-api/src/main/scala/com/gu/productmove/Handler.scala

    "POST",
-    "/product-move/A-S123",
-    Some(ProductMoveEndpointTypes.ExpectedInput(49.99, false, None, None).toJson),
+    "/product-move/recurring-contribution-to-supporter-plus/" + devSubscriptionNumber,


this was not matching the actual routes definiton.

johnduffell · 2023-11-28T11:16:44Z

handlers/product-move-api/src/main/scala/com/gu/productmove/Handler.scala

+    )
+  }
+
+  // for testing


this functions moved out of the superclass ZIOApiGatewayRequestHandler

johnduffell · 2023-11-28T11:18:54Z

handlers/product-move-api/src/main/scala/com/gu/productmove/Secrets.scala

-
-  val credentialsProvider = AwsCredentialsProviderChain
+  private lazy val secretsClient = SecretsManagerClient.builder().credentialsProvider(credentialsProvider).build()
+  lazy val credentialsProvider = AwsCredentialsProviderChain


in another PR this should probably use the credentials provider layer like the other AWS layers

johnduffell · 2023-11-28T12:30:50Z

...oduct-move-api/src/main/scala/com/gu/productmove/framework/ZIOApiGatewayRequestHandler.scala

+            str = new String(allBytes, StandardCharsets.UTF_8)
+            _ <- ZIO.log("input: " + str)
+            loggedOutputStream = new ByteArrayOutputStream()
+            _ <- handler.process[AwsRequestV1](new ByteArrayInputStream(allBytes), loggedOutputStream)


we have to use V1 because we are still using the 1.0 integration
https://docs.aws.amazon.com/apigateway/latest/developerguide/http-api-develop-integrations-lambda.html

I'm not sure if we should be updating or not, it doesn't seem to encourage it in the documentation.

johnduffell · 2023-11-28T12:34:54Z

...oduct-move-api/src/main/scala/com/gu/productmove/framework/ZIOApiGatewayRequestHandler.scala

-          .provideLayer(Runtime.removeDefaultLoggers)
-          .provideLayer(Runtime.addLogger(new AwsLambdaLogger(context.getLogger))),


I removed the lambda logger support, this means we use the default logging.
I'm not sure this was actually still working anyway, as looking in PROD I can't see the timestamps being generated.
Ultimately everything that goes to stdout will end up in the cloudwatch logs, it might just not look nice.

johnduffell · 2023-11-28T12:51:05Z

...oduct-move-api/src/main/scala/com/gu/productmove/framework/ZIOApiGatewayRequestHandler.scala


-object ZIOApiGatewayRequestHandler {
+abstract class ZIOApiGatewayRequestHandler(val server: List[ServerEndpoint[Any, Task]]) extends RequestStreamHandler {


this class has basically been rewritten according to the example
https://github.com/softwaremill/tapir/blob/master/serverless/aws/lambda-zio-tests/src/main/scala/sttp/tapir/serverless/aws/ziolambda/tests/ZioLambdaHandlerImpl.scala
with the addition of request and response stream logging

johnduffell · 2023-11-30T09:40:30Z

handlers/product-move-api/src/main/scala/com/gu/productmove/Secrets.scala

@@ -65,20 +62,11 @@ object Secrets {
  def getSalesforceSSLSecrets: RIO[Secrets, SalesforceSSLSecrets] =
    ZIO.environmentWithZIO[Secrets](_.get.getSalesforceSSLSecrets)
 }
-object SecretsLive extends Secrets {
+class SecretsLive(secretsClient: SecretsManagerClient) extends Secrets {


this is updated to match how the S3 client works, the credentials come from the AwsCredentialsLive.layer

johnduffell · 2023-11-30T09:41:28Z

handlers/product-move-api/src/main/scala/com/gu/productmove/Secrets.scala

+    ZLayer.scoped {
+      for {
+        creds <- ZIO.service[AwsCredentialsProvider]
+        s3Client <- ZIO.fromAutoCloseable(ZIO.attempt(impl(creds)))


this is an important change - the SecretsManagerClient needs to be .close()d after use

johnduffell · 2023-11-30T09:44:08Z

...ers/product-move-api/src/test/scala/com/gu/productmove/zuora/InvoiceItemAdjustmentSpec.scala

@@ -84,7 +85,7 @@ object InvoiceItemAdjustmentSpec extends ZIOSpecDefault {
          ),
        )
        assert(adjustments.length)(equalTo(2)) &&
-          assert(adjustments.head.AdjustmentDate.getDayOfMonth)(equalTo(15))
+        assert(adjustments.head.AdjustmentDate.getDayOfMonth)(equalTo(15))


I don't agree with this, but scalafmt complains otherwise

[warn] scalafmt: ~/code/support-service-lambdas/handlers/product-move-api/src/test/scala/com/gu/productmove/zuora/InvoiceItemAdjustmentSpec.scala isn't formatted properly!

rupertbates

Just one question about error types

johnduffell · 2023-12-11T12:14:43Z

...api/src/main/scala/com/gu/productmove/endpoint/available/AvailableProductMovesEndpoint.scala

        run(subscriptionName).tapEither(result => ZIO.log("result tapped: " + result)).map(Right.apply)
      }
  }

-  // sub to test on: "A-S00334930"
-  private def run(subscriptionName: String): TIO[OutputBody] =
-    runWithEnvironment(SubscriptionName("A-S00334930")).provide(


this shouldn't have been hard coded, it should have used the one passed from the client!

use official ZIO-lambda library in product-move-api

446f63a

johnduffell commented Nov 23, 2023

View reviewed changes

johnduffell added 2 commits November 23, 2023 20:36

small fixes and tweaks

3a6fcb6

make the handler use the zio-aws-lambda one

4cef536

johnduffell commented Nov 28, 2023

View reviewed changes

johnduffell added 2 commits November 28, 2023 11:27

remove unneeded logger and revert accidental reordering of functions

0a27525

CDK uses AWS v1 API gateway format

4fd4f86

johnduffell commented Nov 28, 2023

View reviewed changes

johnduffell added 2 commits November 30, 2023 09:36

close AWS Secrets client after use

11758b0

make sure Secrets client has credentials

855f231

johnduffell commented Nov 30, 2023

View reviewed changes

rupertbates approved these changes Dec 6, 2023

View reviewed changes

johnduffell commented Dec 11, 2023

View reviewed changes

johnduffell merged commit fd0c598 into main Dec 11, 2023

johnduffell deleted the jd-fix-docs-endpoint branch December 11, 2023 12:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use official ZIO-lambda library in product-move-api #2115

use official ZIO-lambda library in product-move-api #2115

johnduffell commented Nov 23, 2023 •

edited

Loading

johnduffell Nov 23, 2023

johnduffell Nov 23, 2023

johnduffell Nov 23, 2023 •

edited

Loading

johnduffell Nov 23, 2023

rupertbates Dec 6, 2023

johnduffell Dec 11, 2023

johnduffell Dec 11, 2023

johnduffell Nov 28, 2023

johnduffell Nov 28, 2023

johnduffell Nov 28, 2023

johnduffell Nov 28, 2023

johnduffell Nov 28, 2023

johnduffell Nov 28, 2023

johnduffell Nov 28, 2023

johnduffell Nov 30, 2023

johnduffell Nov 30, 2023

johnduffell Nov 30, 2023

rupertbates left a comment

johnduffell Dec 11, 2023

		val layer: Layer[Throwable, AwsCredentialsProvider] =
		ZLayer.scoped(ZIO.fromAutoCloseable(ZIO.attempt(impl)))

		.provideLayer(Runtime.removeDefaultLoggers)
		.provideLayer(Runtime.addLogger(new AwsLambdaLogger(context.getLogger))),


		object ZIOApiGatewayRequestHandler {
		abstract class ZIOApiGatewayRequestHandler(val server: List[ServerEndpoint[Any, Task]]) extends RequestStreamHandler {

use official ZIO-lambda library in product-move-api #2115

use official ZIO-lambda library in product-move-api #2115

Conversation

johnduffell commented Nov 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnduffell Nov 23, 2023 • 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

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

rupertbates left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnduffell commented Nov 23, 2023 •

edited

Loading

johnduffell Nov 23, 2023 •

edited

Loading