Partial Key Queries #33

cjoelrun · 2020-06-08T23:40:28Z

Looking to get some feedback on approach and thoughts about how to make partialKey queries safer.

cjoelrun · 2020-06-08T23:41:48Z

src/main/dynamo/Model.ts

+  }
+
+  partialKey(
+    params: AtLeastOne<{ [X in U]: string }> & AtLeastOne<{ [Y in V]: string }>


This is where I think I could possibly use a generated type of only valid combinations of keys.

Hmm... I think the types we'd want to express here would be a union like:

{ country: string } | { country: string, state: string } | { country: string, state: string, city: string }

This is tricky to do as is since:

We have a generic model class

All it knows inside here is that partitionKeys / sortKeys are a union type.

Afaik there's not a way to extract keys of a union type in-order and re-combine them to what we'd want (maybe there is, I just don't know about it).

This would be easier to express if we explicitly codegenned each model vs having a generic model class as we do today. Since then, we could just codegen the right types directly

jcarver989 · 2020-06-10T22:15:13Z

Nice @cjoelrun ! Going to try to get to reviewing this ~soon.

cjoelrun · 2020-06-10T22:17:01Z

I think there's still some work to be done, just wanted your opinion before I continue in this or another direction.

jcarver989

Nice @cjoelrun !

Sorry it took so long to get to this PR. But really exciting to see!

I think the overall approach here is a good short-term solution, in that we can check-in an initial version of complex / partial key support that doesn't enforce needing correct combinations of partial keys. As a "v1 / mvp" of this feature we could just allow a Partial<T> where T is the "full key".

Then, the long-term approach I'd want to take is to refactor our Model class / codegen to make expressing the types needed to enforce correct partial key combinations easier. I think this will basically result in axe-ing our generic Model class and generating specific classes for each model instead.

So, for now:

I'd like to DRY-up a lot of the isArray / isNotArray checks that are a result of threading the string | string[] type throughout the code. I think we could do this pretty easily by just normalizing non-array types to arrays near the "top" of where they appear.
There's a few small things related to style/naming that I've left comments on.

jcarver989 · 2020-06-09T16:19:17Z

src/main/codegen/generateGSIs.ts

@@ -1,4 +1,5 @@
 import { Fields, Table } from "./types"
+import {buildKey, replaceKeyDollar} from "./util"


These imports seem not be used?

jcarver989 · 2020-06-11T15:32:40Z

src/main/codegen/generateTables.ts

-      encryptionBlacklistSet.add(pk.replace("$", ""))
-      encryptionBlacklistSet.add(sk.replace("$", ""))
+
+      const pks = buildKeyArray(model.keys.partitionKey[1])


Looks like you can just use the pk and sk variables above as they're already bound to model.keys.partitionKey[1] and model.keys.sortKey[1]

jcarver989 · 2020-07-06T12:55:15Z

src/main/codegen/generateTables.ts

    })

  table.gsis.forEach(({ name, partitionKey, sortKey }) => {
-    encryptionBlacklistSet.add(partitionKey.replace("$", ""))
-    encryptionBlacklistSet.add(sortKey.replace("$", ""))
+    const pk = buildKeyArray(partitionKey)


Nit: would just inline to: buildKeyArray(partitionKey).forEach(key => encryptionBlacklist.add(key))

jcarver989 · 2020-07-06T13:00:36Z

src/main/codegen/util.ts

@@ -11,3 +11,27 @@ export function groupBy<T extends { [key: string]: any }, U extends keyof T>(

  return Object.entries(groupedItems)
 }
+
+export function replaceKeyDollar(key: string | string[]) {


Hmm...we're threading this string | string[] type through multiple places in the code. I'd prefer we only do this check once at the "top level" and just transform string types into string[]. That way, everything that comes after can just assume the type is string[]

jcarver989 · 2020-07-06T13:05:30Z

src/main/codegen/util.ts

@@ -11,3 +11,27 @@ export function groupBy<T extends { [key: string]: any }, U extends keyof T>(

  return Object.entries(groupedItems)
 }
+
+export function replaceKeyDollar(key: string | string[]) {


Style: all function types outside of tests should explicitly specify their return types. This avoids accidentally getting the wrong type inferred.

For inline lambdas, e.g. const f = () => { ... }, it's fine to leave the return type off since they're usually very simple .

jcarver989 · 2020-07-06T13:31:42Z

src/main/dynamo/Model.ts

-  private buildKey(prefix: string, key: string): string {
+  /* TODO: Had issue mapping unioned fields of T & V */
+  private _buildPartitionKey(model: AtLeastOne<{ [X in U]: string }>, fields: U | U[]): string | string[] {
+    if (Array.isArray(fields)) {


I think this is another place we're we'd benefit from normalizing non-array keys into arrays. For example, we could transform keys to arrays internally in the model's constructor.

jcarver989 · 2020-07-06T13:40:30Z

src/main/dynamo/Model.ts

+  }
+
+  partialKey(
+    params: AtLeastOne<{ [X in U]: string }> & AtLeastOne<{ [Y in V]: string }>


Hmm... I think the types we'd want to express here would be a union like:

{ country: string } | { country: string, state: string } | { country: string, state: string, city: string }

This is tricky to do as is since:

We have a generic model class

All it knows inside here is that partitionKeys / sortKeys are a union type.

Afaik there's not a way to extract keys of a union type in-order and re-combine them to what we'd want (maybe there is, I just don't know about it).

This would be easier to express if we explicitly codegenned each model vs having a generic model class as we do today. Since then, we could just codegen the right types directly

jcarver989 · 2020-07-06T13:57:40Z

src/test/codegen/generateCode.test.ts

@@ -322,3 +322,49 @@ Tables:

  expect(lines).toContainEqual(`name: BestNameEvah`)
 })
+
+it("should generate a complex key model", () => {


Nice test, thanks!

jcarver989 · 2020-07-06T13:58:07Z

src/test/codegen/generateCode.test.ts

+    Partitions:
+      ComplexAuthors:
+        ComplexAuthor:
+          partitionKey: [Author, $id]


Would like to add a test for partition keys as well, since it looks like this PR supports complex keys for both.

jcarver989 · 2020-07-06T14:01:08Z

src/main/dynamo/types.ts

@@ -11,3 +11,5 @@ export type ExtractKeyType<T> = T extends PartitionAndSortKey<infer U>
 export type GroupedModels<T extends TaggedModel> = {
  [K in T["model"]]: T extends { model: K } ? T[] : never
 }
+
+export type AtLeastOne<T, U = { [K in keyof T]: Pick<T, K> }> = Partial<T> & U[keyof U]


Hmm... This ends up generating a pretty crazy type that's hard to read. And it appears to be basically equivalent to Partial<{ [X in U]: string }> -- i.e. you can specify one or more of the key parts.

cjoelrun added 2 commits June 5, 2020 12:23

array key

96afe58

tests

90c450c

cjoelrun requested a review from jcarver989 June 8, 2020 23:40

cjoelrun commented Jun 8, 2020

View reviewed changes

cjoelrun marked this pull request as draft June 8, 2020 23:47

jcarver989 suggested changes Jul 6, 2020

View reviewed changes

jcarver989 mentioned this pull request Jul 17, 2020

[Beyonce]: Allow Compound Partition/Sort Keys #34

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Partial Key Queries #33

Partial Key Queries #33

cjoelrun commented Jun 8, 2020

cjoelrun Jun 8, 2020

jcarver989 Jul 6, 2020

jcarver989 commented Jun 10, 2020

cjoelrun commented Jun 10, 2020

jcarver989 left a comment •

edited

Loading

jcarver989 Jun 9, 2020

jcarver989 Jun 11, 2020

jcarver989 Jul 6, 2020

jcarver989 Jul 6, 2020

jcarver989 Jul 6, 2020

jcarver989 Jul 6, 2020

jcarver989 Jul 6, 2020

jcarver989 Jul 6, 2020

jcarver989 Jul 6, 2020

jcarver989 Jul 6, 2020

		@@ -1,4 +1,5 @@
		import { Fields, Table } from "./types"
		import {buildKey, replaceKeyDollar} from "./util"

Partial Key Queries #33

Are you sure you want to change the base?

Partial Key Queries #33

Conversation

cjoelrun commented Jun 8, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcarver989 commented Jun 10, 2020

cjoelrun commented Jun 10, 2020

jcarver989 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcarver989 left a comment •

edited

Loading