feature: Added APIGatewayAuthorizerEvent & results#639
Conversation
|
@armanbilge please review |
| } | ||
|
|
||
| sealed abstract class ApiGatewayCustomAuthorizerEvent { | ||
| def `type`: String |
There was a problem hiding this comment.
Hi Elias, great job!
I'm wondering if it would be better to have this as a custom object instead of String, similar to here, since "type" can only be "TOKEN" and "REQUEST" (reference).
I also noticed that DefinitelyTyped uses a single RequestContext function for both the proxy events and the authorizer ones, so I am also wondering if, in the future, unifying the implementations would be better than having separate ones (like the one in WebSocketEvent).
There was a problem hiding this comment.
I also noticed that DefinitelyTyped uses a single RequestContext function for both the proxy events and the authorizer ones, so I am also wondering if, in the future, unifying the implementations would be better than having separate ones (like the one in WebSocketEvent).
Oh this was meant for @armanbilge, sorry I didn't mention you!
There was a problem hiding this comment.
I also noticed that DefinitelyTyped uses a single RequestContext function for both the proxy events and the authorizer ones
Great catch! I agree, instead of defining a new class here, maybe let's re-use the existing RequestContext class and add all the additional fields. I'm not sure why it is currently missing most of them 🤔
armanbilge
left a comment
armanbilge
left a comment
There was a problem hiding this comment.
Great idea from Ching about sharing the RequestContext class, let's do that!
| def resourcePath: String | ||
| def httpMethod: String | ||
| def extendedRequestId: String | ||
| def requestTime: String |
There was a problem hiding this comment.
I see there is requestTime: String and requestTimeEpoch: Long. Probably they are redundant, and we could use requestTime: Instant for the best UX?
There was a problem hiding this comment.
I don't think they are redundant since it's enumerated here in the output format
There was a problem hiding this comment.
What I mean by "redundant" is that the field contain identical information, just presented in different formats. We should be able to retrieve the time entirely using the epoch timestamp.
| def domainPrefix: String | ||
| def requestTimeEpoch: Long | ||
| def requestId: String | ||
| def identity: Map[String, Option[String]] |
There was a problem hiding this comment.
This type looks a bit strange 🤔 I wonder in what case would there be a key would no value? (instead of just not having the key at all).
There was a problem hiding this comment.
From the Input format of a Lambda function for proxy integration there are such cases . Also, I'd specifically update the identity key to use an identity object as done here
There was a problem hiding this comment.
Interesting, I see. It looks like it would be better to define a new class for identity instead of using a Map since the fields appear to be predefined.
| } | ||
|
|
||
| sealed abstract class ApiGatewayCustomAuthorizerEvent { | ||
| def `type`: String |
There was a problem hiding this comment.
I also noticed that DefinitelyTyped uses a single RequestContext function for both the proxy events and the authorizer ones
Great catch! I agree, instead of defining a new class here, maybe let's re-use the existing RequestContext class and add all the additional fields. I'm not sure why it is currently missing most of them 🤔
3 participants
#48 : adds AWS Lambda event type APIGatewayAuthorizerEvent: