{"action":"created","issue":{"url":"https://api.github.com/repos/broadinstitute/rawls/issues/547","repository_url":"https://api.github.com/repos/broadinstitute/rawls","labels_url":"https://api.github.com/repos/broadinstitute/rawls/issues/547/labels{/name}","comments_url":"https://api.github.com/repos/broadinstitute/rawls/issues/547/comments","events_url":"https://api.github.com/repos/broadinstitute/rawls/issues/547/events","html_url":"https://github.com/broadinstitute/rawls/pull/547","id":181280711,"number":547,"title":"[NOTYET] GAWB-1096 Reworked how attribute lists are stored in the database and exported to JSON","user":{"login":"helgridly","id":775136,"avatar_url":"https://avatars.githubusercontent.com/u/775136?v=3","gravatar_id":"","url":"https://api.github.com/users/helgridly","html_url":"https://github.com/helgridly","followers_url":"https://api.github.com/users/helgridly/followers","following_url":"https://api.github.com/users/helgridly/following{/other_user}","gists_url":"https://api.github.com/users/helgridly/gists{/gist_id}","starred_url":"https://api.github.com/users/helgridly/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/helgridly/subscriptions","organizations_url":"https://api.github.com/users/helgridly/orgs","repos_url":"https://api.github.com/users/helgridly/repos","events_url":"https://api.github.com/users/helgridly/events{/privacy}","received_events_url":"https://api.github.com/users/helgridly/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":2,"created_at":"2016-10-05T22:21:14Z","updated_at":"2016-10-07T00:08:04Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/broadinstitute/rawls/pulls/547","html_url":"https://github.com/broadinstitute/rawls/pull/547","diff_url":"https://github.com/broadinstitute/rawls/pull/547.diff","patch_url":"https://github.com/broadinstitute/rawls/pull/547.patch"},"body":"This PR covers GAWB-1114 as well. It has a few moving parts, so here's a walkthrough. The links dotted through the doc will jump you around the diff.\r\n\r\nIt also has companion PRs in Orch and UI that don't exist yet because I haven't written them. But I still want early feedback on this one.\r\n\r\n# The Context\r\n\r\nhttps://broadinstitute.atlassian.net/browse/GAWB-1096\r\n\r\nTL;DR: Put this.participant into a method config input and you don't get an error. You should.\r\n\r\n# The Bugs\r\n\r\n## Bug One: We don't throw an error when an attribute expression returns an entity\r\n\r\nHere's [the bug](https://github.com/broadinstitute/rawls/pull/547/files#diff-5311caed81a6a46128e4132cfeeedac8L243): \"Since we know we are not dealing with a reference here as this is the attribute final func, we can pass in None.\"\r\n\r\nTurns out that's wrong: we've pulled the attribute with the matching name (in this case, \"participant\"), but because we pass in None for the referenced entity, we end up skipping past [the branch we wanted to go into](https://github.com/broadinstitute/rawls/blob/8114dfef312fe18d555710f774dcb03820a968ab/src/main/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/AttributeComponent.scala#L348) and end up treating it as a value instead, ultimately [returning AttributeNull](https://github.com/broadinstitute/rawls/blob/8114dfef312fe18d555710f774dcb03820a968ab/src/main/scala/org/broadinstitute/dsde/rawls/dataaccess/slick/AttributeComponent.scala#L397).\r\n\r\nThe fix is to [distinguish between reference and value attributes](https://github.com/broadinstitute/rawls/pull/547/files#diff-5311caed81a6a46128e4132cfeeedac8R251) ahead of time, parse the value attributes nicely, and then [handle the entity references](https://github.com/broadinstitute/rawls/pull/547/files#diff-5311caed81a6a46128e4132cfeeedac8R257) by turning them into fake `AttributeEntityReference` objects that we can [catch and throw an exception in later](https://github.com/broadinstitute/rawls/pull/547/files#diff-5311caed81a6a46128e4132cfeeedac8R399).\r\n\r\n### Detour: Distinguishing between reference and value attributes\r\n\r\nWell, this is easy, right? Here are the fields of an AttributeRecord:\r\n\r\n```\r\ntrait AttributeRecord[OWNER_ID] {\r\n  val id: Long\r\n  val ownerId: OWNER_ID\r\n  val namespace: String\r\n  val name: String\r\n  val valueString: Option[String]\r\n  val valueNumber: Option[Double]\r\n  val valueBoolean: Option[Boolean]\r\n  val valueEntityRef: Option[Long]\r\n  val listIndex: Option[Int]\r\n  val listLength: Option[Int]\r\n}\r\n```\r\n\r\nIt's an entity reference if `valueEntityRef` is defined and a value otherwise. For lists, we populate the `listIndex` field to indicate which element of the list the record refers to, pull 'em all, and reassemble the list afterwards.\r\n\r\nFor empty lists, we just save a dummy record with `listIndex = -1` to indicate that the list is empty, and leave all the `valueFoo` fields blank.\r\n\r\nAh.\r\n\r\nNow we can't tell the difference between an empty reference list and an empty value list, because [they're saved to the database in the same way.](https://github.com/broadinstitute/rawls/pull/547/files#diff-37c2cc14b84b1a0ed1d99ba2adb74b18L234)\r\n\r\nGuess we have to fix that now.\r\n\r\n## Bug Two: Can't tell the difference between an empty value list and an empty reference list\r\n\r\nThis doesn't _seem_ to be a huge problem: the list is empty, who cares?\r\n\r\nWell, maybe you're a user who accidentally writes `this.samples` in your method config expression when you really mean `this.samples.name` (I've seen this happen), and it doesn't error the first time because your sample set was empty. Then you reuse the method config a second time and it suddenly stops working because we fixed Bug One and now throw errors when you end up with a ref at the end of a value expression.\r\n\r\nSo let's fix that. New rule! For empty value lists, [we jam a -1 in the `valueNumber` field](https://github.com/broadinstitute/rawls/pull/547/files#diff-37c2cc14b84b1a0ed1d99ba2adb74b18R199). For empty ref lists, we leave them all blank (because `valueEntityRef` is enforced with a foreign key constraint, so we can't put a dummy value in there). This also means we get rid of `AttributeEmptyList` in favour of [two new types](https://github.com/broadinstitute/rawls/pull/547/files#diff-37c2cc14b84b1a0ed1d99ba2adb74b18R248), `AttributeEntityReferenceEmptyList` and `AttributeValueEmptyList`.\r\n\r\n\r\nThen when we go to unmarshal the list from the database, if the `listIndex` is -1 we can [look at the `valueNumber` field and distinguish whether it's an empty ref or value list](https://github.com/broadinstitute/rawls/pull/547/files#diff-37c2cc14b84b1a0ed1d99ba2adb74b18R381).\r\n\r\nThis is great - we've now added types to empty lists (something even Scala can't do!) and we can distinguish between them.\r\n\r\nUnfortunately we're still not done yet.\r\n\r\n## Bug Three: JSON representation is still ambiguous\r\n\r\n`AttributeEmptyList` was serialized out to JSON as `[]`. Now we've ditched that in favour of two separate empty list types, how do we distinguish between them in JSON? Unless we do something about this, we can't roundtrip our data through JSON, as we'll have no idea how to interpret `[]` and _must_ error.\r\n\r\nWelp, there's only one thing we can do. Here's [how lists are represented now](https://github.com/broadinstitute/rawls/pull/547/files#diff-6242a09492d6dabd946ed5cadd93113aR68):\r\n\r\n```\r\n{\r\n    \"itemsType\" : \"AttributeValue\", //or \"EntityReference\"\r\n\t\"items\" : [1,2,3]\r\n}\r\n```\r\nEww.\r\n\r\nThat's what the changes around [AttributeFormat](https://github.com/broadinstitute/rawls/pull/547/files#diff-6242a09492d6dabd946ed5cadd93113aR39) are about.\r\n\r\nBut we only want to use this representation when talking to the outside world. Cromwell expects the old behaviour of lists being plain old JSON arrays, so we encapsulate that behaviour [in a new different WDLJsonSupport class](https://github.com/broadinstitute/rawls/pull/547/files#diff-527aaa28a026d49549dfda679d66e494R5).\r\n\r\n### Tweaking JsonSupport\r\n\r\nThe first version of this PR had an icky implementation of providing different AttributeList serialization strategies (object-y version vs. plain old lists). This has since been reworked thanks to a suggestion from Doge.\r\n\r\n`JsonSupport.AttributeFormat` now [has a mixin](https://github.com/broadinstitute/rawls/pull/547/files#diff-6242a09492d6dabd946ed5cadd93113aR97) called `AttributeListSerializer`. There are two versions of this mixin: `TypedAttributeListSerializer` and `PlainArrayAttributeListSerializer`, representing the object-y version and the plain old list version. [The default version of the implicit](https://github.com/broadinstitute/rawls/pull/547/files#diff-6242a09492d6dabd946ed5cadd93113aR128) uses the `TypedAttributeListSerializer`, but things that interact with Cromwell provide [their own JsonSupport implicit](https://github.com/broadinstitute/rawls/pull/547/files#diff-527aaa28a026d49549dfda679d66e494R6) that overrides the mixin.\r\n\r\n# Checklist time!\r\n\r\n- [ ] **Submitter**: Include the JIRA issue number in the PR description\r\n- [ ] **Submitter**: Make sure Swagger is updated if API changes\r\n- [ ] **Submitter**: If updating admin endpoints, also update [firecloud-admin-cli](https://github.com/broadinstitute/firecloud-admin-cli)\r\n- [ ] **Submitter**: Check documentation and code comments. Add explanatory PR comments if helpful.\r\n- [ ] **Submitter**: JIRA ticket checks:\r\n  * Acceptance criteria exists and is met\r\n  * Note any changes to implementation from the description\r\n  * Add notes on what you've tested\r\n- [ ] **Submitter**: Update RC_XXX release ticket with any config or environment changes necessary\r\n- [ ] **Submitter**: Database checks:\r\n  * If PR includes new or changed db queries, include the explain plans in the description\r\n  * Make sure liquibase is updated if appropriate\r\n  * If doing a migration, take a backup of the\r\n  [dev](https://console.cloud.google.com/sql/instances/terraform-qfarbdq3lrexxck5htofjs5z6m/backups?project=broad-dsde-dev&organizationId=548622027621)\r\n  and\r\n  [alpha](https://console.cloud.google.com/sql/instances/terraform-r4caezzc35c4tb7pgdhwkmme4y/backups?project=broad-dsde-alpha&organizationId=548622027621)\r\n  DBs in Google Cloud Console\r\n- [ ] **Submitter**: Update FISMA documentation if changes to:\r\n  * Authentication\r\n  * Authorization\r\n  * Encryption\r\n  * Audit trails\r\n- [ ] **Submitter**: If you're adding new libraries, sign us up to security updates for them\r\n- [ ] Tell ![](http://i.imgur.com/9dLzbPd.png) that the PR exists if he wants to look at it **(apply requires_doge label)**\r\n- [ ] Anoint a lead reviewer (LR). **Assign PR to LR**\r\n* Review cycle:\r\n  * LR reviews\r\n  * Rest of team may comment on PR at will\r\n  * **LR assigns to submitter** for feedback fixes\r\n  * Submitter rebases to develop again if necessary\r\n  * Submitter makes further commits. DO NOT SQUASH\r\n  * Submitter updates documentation as needed\r\n  * Submitter **reassigns to LR** for further feedback\r\n- [ ] ![](http://i.imgur.com/9dLzbPd.png) sign off\r\n- [ ] **LR** sign off\r\n- [ ] **Assign to submitter** to finalize\r\n- [ ] **Submitter**: Verify all tests go green, including CI tests\r\n- [ ] **Submitter**: Squash commits and merge to develop\r\n- [ ] **Submitter**: Delete branch after merge\r\n- [ ] **Submitter**: **Test this change works on dev environment after deployment**. YOU own getting it fixed if dev isn't working for ANY reason!\r\n- [ ] **Submitter**: Verify swagger UI on dev environment still works after deployment\r\n- [ ] **Submitter**: Inform other teams of any API changes via hipchat and/or email\r\n- [ ] **Submitter**: Mark JIRA issue as resolved once this checklist is completed\r\n"},"comment":{"url":"https://api.github.com/repos/broadinstitute/rawls/issues/comments/252122223","html_url":"https://github.com/broadinstitute/rawls/pull/547#issuecomment-252122223","issue_url":"https://api.github.com/repos/broadinstitute/rawls/issues/547","id":252122223,"user":{"login":"coveralls","id":2354108,"avatar_url":"https://avatars.githubusercontent.com/u/2354108?v=3","gravatar_id":"","url":"https://api.github.com/users/coveralls","html_url":"https://github.com/coveralls","followers_url":"https://api.github.com/users/coveralls/followers","following_url":"https://api.github.com/users/coveralls/following{/other_user}","gists_url":"https://api.github.com/users/coveralls/gists{/gist_id}","starred_url":"https://api.github.com/users/coveralls/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/coveralls/subscriptions","organizations_url":"https://api.github.com/users/coveralls/orgs","repos_url":"https://api.github.com/users/coveralls/repos","events_url":"https://api.github.com/users/coveralls/events{/privacy}","received_events_url":"https://api.github.com/users/coveralls/received_events","type":"User","site_admin":false},"created_at":"2016-10-07T00:08:04Z","updated_at":"2016-10-07T00:08:04Z","body":"\n[![Coverage Status](https://coveralls.io/builds/8225739/badge)](https://coveralls.io/builds/8225739)\n\nCoverage increased (+0.08%) to 77.62% when pulling **f3da65c55e1d7885dfffffabfd9ef229fd635073 on dodgy_attribute_exprs** into **44137c95d6000e39b57629cbc127f771b1e5164c on develop**.\n"}}
{"actor":{"display_login":"coveralls"}}