Add path api gateway request #396
Conversation
| case Some("DELETE") => stepsToDelete _ | ||
| request => ((request.httpMethod, | ||
| request.path) match { // TODO will need to match against path params too to support edit endpoint | ||
| case (Some("GET"), Some(POTENTIAL_PROXY_RESOURCE_PATH)) => stepsForPotentialHolidayStop _ |
There was a problem hiding this comment.
@johnduffell and I did have a play around this morning to see if we could make something a bit more like a router - didn't quite get to anything just right, and might be worth seeing if there is a lightweight dependency for this purpose rather than re-inventing the wheel. We could really cut down the CloudFormation by having a catch all there and doing all the path parameter definition here - would like to explore more, given more time in future
| @@ -116,8 +117,8 @@ object Handler extends Logging { | |||
|
|
|||
| def stepsToCreate(req: ApiGatewayRequest, sfClient: SfClient): ApiResponse = { | |||
|
|
|||
| val verifyContactOwnsSubOp = SalesforceSFSubscription.SubscriptionForSubscriptionNameAndContact(sfClient.wrapWith(JsonHttp.getWithParams)) | |||
| val createOp = SalesforceHolidayStopRequest.CreateHolidayStopRequestWithDetail(sfClient.wrapWith(JsonHttp.post)) | |||
| val verifyContactOwnsSubOp: (SubscriptionName, Contact) => Types.ClientFailableOp[Option[MatchingSubscription]] = SalesforceSFSubscription.SubscriptionForSubscriptionNameAndContact(sfClient.wrapWith(JsonHttp.getWithParams)) | |||
There was a problem hiding this comment.
are these explicit types necessary?
There was a problem hiding this comment.
scala is not great for converting between equivalent things transparently, but if the type is a bit wtf, the equivalent to consider would be to use VerifyContact and then define either a type alias, or for proper type checking at the expense of boilerplate, a trait similar to this
trait VerifyContact {
def apply(subName: SubscriptionName, contact: Contact): Types.ClientFailableOp[Option[MatchingSubscription]]
}There was a problem hiding this comment.
I just expanded them to work out what was going on, ill remove them
what does that mean? I know you can specify parameters to validate in the cfn, but I think they get through regardless. Or did you mean something else? |
| case (Some("GET"), Some(POTENTIAL_PROXY_RESOURCE_PATH)) => stepsForPotentialHolidayStop _ | ||
| case (Some("GET"), Some(GET_ALL_AND_CREATE_PROXY_RESOURCE_REGEX())) => stepsToListExisting _ | ||
| case (Some("POST"), Some(GET_ALL_AND_CREATE_PROXY_RESOURCE_REGEX())) => stepsToCreate _ | ||
| case (Some("DELETE"), Some(GET_ALL_AND_CREATE_PROXY_RESOURCE_REGEX())) => stepsToDelete _ |
There was a problem hiding this comment.
is request.httpMethod really optional? it's not in real life, so if it's ended up in an option outside of our control, can we squash None into an error before it gets into this logic? same goes for request.path
There was a problem hiding this comment.
would it be worth pattern matching the resource first, and if it's GET_ALL_AND_CREATE_PROXY_RESOURCE_REGEX then match the method?
There was a problem hiding this comment.
this would be a lot cleaner if possible
|
looks good to me but I've chucked in a few random thoughts - use as you see fit!! 😆 |
|
@johnduffell RE:
All I was saying was that we currently use the pathParams map in the ApiGatewayRequest object to get hold of path parameter. As I understand it the keys for that map are derived from the cloud formation config. I seems a bit of a long winded process to get hold of path parameters when they could just pop out of the path pattern matching. |
|
I don't think we directly derive anything like that from cfn, but it's possible there is some hard coded expected values from when the code wasn't fully refactored in the past, but that's probably because fixing that up is still in the tech debt doc. I guess the main point I'm making is that we should design the URL structure to make sense, and then we should work out how to implement it. Not based on how the code looks now as changing the code safely is much easier than changing the URL structure later. |
| val GET_ALL_AND_CREATE_PROXY_RESOURCE_REGEX = """/hsr.*""".r | ||
|
|
||
| def operationForEffects(response: Request => Response, stage: Stage, | ||
| fetchString: StringFromS3): ApiGatewayOp[Operation] = { |
There was a problem hiding this comment.
not sure if this was intentiional, but generally I would stick with consistent indent for both parameters and code (i.e. 2 space in most of this code)
if it wasn't intentional, I think there's a setting in intellij to turn off to stop it doing this randomly!
There was a problem hiding this comment.
we managed to find the settings in intellij to stop that happening randomly - first found by Adam F:
This PR add a "path" field to the ApiGatewayRequest case class in order to support routing requests based on the full path.
Previously code could only match on path parameters making it difficult for a single lambdas to distinguish between request for different resources.
Making use of the path parameters strongly couples the code to the api gateway config/request routing, in the future it might be simpler just to use patten matching on the path?
Also included: