Basic parsing of the mappings into our model #5
Conversation
| err => Failure(err), | ||
| json => Success(json) | ||
| suc => Success(suc) |
There was a problem hiding this comment.
Minor point, but I would rename 'suc' to 'mappings' (or something similar)
| tryJson.map { json => | ||
| val mappings = json.hcursor.downField("mappings").as[List[Json]] | ||
| val c: HCursor = jsonTargets.hcursor | ||
| // TODO: do we want to return an empty list, or throw a error here? |
There was a problem hiding this comment.
I would suggest throwing an error in the future - if we don't have a target, we can't notify - and that's the primary function of this service, so we might as well fail fast!
| ??? | ||
| } | ||
| val c: HCursor = jsonContacts.hcursor | ||
| // TODO: do we want to return an empty list, or throw a error here? |
|
|
||
| private[models] def parseTargets(jsonTargets: Json): List[Target] = { |
There was a problem hiding this comment.
I feel like this could fail and thus should return a Result as well?
There was a problem hiding this comment.
Agreed, I would like to change this so we fail fast... yet for now I will feel happier picking it up after we have something working end to end 🙈
|
|
||
| private[models] def parseTargets(jsonTargets: Json): List[Target] = { | ||
| def parseTarget(key: String, value: String): Option[Target] = { |
There was a problem hiding this comment.
Is there a reason why this fn is inlined? It could be a separate function and then it would be independently testable.
| private[models] def parseTargets(jsonTargets: Json): List[Target] = { | ||
| def parseTarget(key: String, value: String): Option[Target] = { | ||
| key.toLowerCase match { | ||
| case "stack" => Some(Stack(value)) |
There was a problem hiding this comment.
I think this might confuse people because these tags are all (strangely) fussy about their case elsewhere. By convention we use Stack, App and Stage.
There was a problem hiding this comment.
Oh sorry, I missed the toLowerCase. Do we need to lower case it here, or can we just assume people will use the correct case? If you give people the chance to misconfigure something they will do it!
There was a problem hiding this comment.
toLowerCase felt like an easy win for catching a small frustration that could creep into the test data or prod data. Maybe we should error, yet I feel casing is an easy thing to recover from.
If the config.json is ultimately going to be machine generated, where should the responsibility of sanity checking it fall? I feel like the json generator should hold the majority of that responsibility... and maybe that allows anghammarad to mostly trust the input string?
|
|
||
| def parseMapping(json: Json): Result[(List[Target], List[Contact])] = { | ||
| ??? | ||
| keys.flatMap { key => |
There was a problem hiding this comment.
would this be clearer as a for comp? (I'm not sure, just wondering :-) )
| Serialization.parseTargets(testJson) shouldEqual List(Stack("stack-1"), App("app-1")) | ||
| } | ||
|
|
||
| "will return a failure if it cannot parse the keys" ignore { |
| "will correctly return the contacts" in { | ||
| val testJson = parse( | ||
| """{"email":"stack.email","hangouts":"stack.room"}""" | ||
| ).getOrElse(Json.Null) |
There was a problem hiding this comment.
Maybe just a get here? If this fails we want to know about it straight away. At the moment it'll return empty JSON and then we'll get a test failure. At that point it won't be clear whether the problem is in our function or the test data.
| """{"target":{"AwsAccount":"123456789"},"contacts":{"email":"awsAccount.email"}}""" | ||
| ).getOrElse(Json.Null) | ||
|
|
||
| Serialization.parseMapping(testJson).getOrElse(Json.Null) shouldEqual Mapping(List(AwsAccount("123456789")), List(EmailAddress("awsAccount.email"))) |
There was a problem hiding this comment.
If you mix in OptionValues into the test class you can do anOption.value. This will fail the test if it is a None.
|
|
||
| "will return a failure if the string is not valid json" in { | ||
| val brokenJsonString = | ||
| """{ |
There was a problem hiding this comment.
This is very hard to read! Can it be indented nicely? Makes it hard to see in what sense this is "broken". If it's just invalid JSON then would a very short invalid string by a reduced test case?
This is the happy path of parsing the mappings.
When parsing Contacts and Targets there is no checking for weirdness - we are trusting that the config will be in an acceptable format for now.
We will probably want to fail immediately and send a notification to the owners or the lambda if the parser runs into issues at any point. Aspirational test cases added is that is the best option.