Skip to content

Conversation

@ngwin-ab
Copy link

@ngwin-ab ngwin-ab commented Oct 14, 2025

Added Application Load Balancer Request Event as part of #48, all tests passed.

Screenshot 2025-10-23 at 12 25 45 PM

@ngwin-ab ngwin-ab marked this pull request as ready for review October 14, 2025 17:04
@ngwin-ab
Copy link
Author

hi @armanbilge : Could you please review this PR? Thank you!

@ngwin-ab
Copy link
Author

@bpholt : hi Brian, thanks for reviewing the PR! I was away from my laptop last week and did not have a chance to work on this until now. I've updated the PR to address your comments, could you please take a look? Thank you!

@ngwin-ab
Copy link
Author

@armanbilge : hi Arman, could you please review this PR when you have time? Thank you!

Comment on lines 64 to 67
private implicit val mapStringDecoder: Decoder[Map[CIString, String]] =
Decoder.decodeMap[CIString, String]
private implicit val mapListDecoder: Decoder[Map[CIString, List[String]]] =
Decoder.decodeMap[CIString, List[String]]
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, are you sure these are necessary? I feel surprised/confused.

Copy link
Author

Choose a reason for hiding this comment

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

They are not called directly in the code, they are used implicitly by Circe's Decoder when decoding JSON into an ApplicationLoadBalancerRequestEvent instance

Copy link
Member

Choose a reason for hiding this comment

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

Can you please double-check? I tried removing them, and it looks like the code still compiles.

@ngwin-ab
Copy link
Author

@armanbilge : I've addressed the review comments, could you please take a look?

Comment on lines +134 to +135
val encodedBody =
java.util.Base64.getEncoder.encodeToString("hello world".getBytes("UTF-8"))
Copy link
Member

Choose a reason for hiding this comment

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

scodec-bits has a nice syntax for this:

Suggested change
val encodedBody =
java.util.Base64.getEncoder.encodeToString("hello world".getBytes("UTF-8"))
val encodedBody = asciiBytes"hello world".toBase64

import org.typelevel.ci.CIString
import scodec.bits.ByteVector

sealed abstract class Elb {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this name is more clear :)

Suggested change
sealed abstract class Elb {
sealed abstract class ElasticLoadBalancer {

Comment on lines 64 to 67
private implicit val mapStringDecoder: Decoder[Map[CIString, String]] =
Decoder.decodeMap[CIString, String]
private implicit val mapListDecoder: Decoder[Map[CIString, List[String]]] =
Decoder.decodeMap[CIString, List[String]]
Copy link
Member

Choose a reason for hiding this comment

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

Can you please double-check? I tried removing them, and it looks like the code still compiles.

object Elb {
def apply(targetGroupArn: String): Elb = Impl(targetGroupArn)

implicit val decoder: Decoder[Elb] = Decoder.forProduct1("targetGroupArn")(Elb.apply)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
implicit val decoder: Decoder[Elb] = Decoder.forProduct1("targetGroupArn")(Elb.apply)
private[events] implicit val decoder: Decoder[Elb] = Decoder.forProduct1("targetGroupArn")(Elb.apply)

def apply(elb: Elb): ApplicationLoadBalancerRequestContext =
Impl(elb)

implicit val decoder: Decoder[ApplicationLoadBalancerRequestContext] =
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
implicit val decoder: Decoder[ApplicationLoadBalancerRequestContext] =
private[events] implicit val decoder: Decoder[ApplicationLoadBalancerRequestContext] =

queryStringParameters = Some(Map("query" -> "1234ABCD")),
headers = Some(
Map(
org.typelevel.ci.CIString("accept") -> "text/html,application/xhtml+xml",
Copy link
Member

Choose a reason for hiding this comment

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

There is a special syntax to shorten this.

Suggested change
org.typelevel.ci.CIString("accept") -> "text/html,application/xhtml+xml",
ci"accept" -> "text/html,application/xhtml+xml",

Comment on lines +188 to +191
assert(
decoded
.bodyDecoded
.exists(_ == ByteVector.encodeUtf8("hello world").getOrElse(ByteVector.empty)))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
assert(
decoded
.bodyDecoded
.exists(_ == ByteVector.encodeUtf8("hello world").getOrElse(ByteVector.empty)))
assertEquals(decoded.bodyDecoded, Some(asciiBytes"hello world"))

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.

3 participants