Correct resolver batching behavior #1495

zephraph · 2019-02-07T04:30:27Z

After enabling batching we found a few problems.

List endpoints return an array but single endpoints return an object. I was expecting all responses to be arrays and called Array.find on the results which caused metaphysics to crash on single responses
Dataloader wasn't properly deduping data. Turns out it does strict checking of the inputs to calculate a cache key (meaning objects are duplicated).
For any given endpoint "id" could also be a slug.
Not related to this implementation, but also a problem... list endpoints generally don't support slugs. Thankfully @mzikherman helped me out and updated the sales endpoint to do so.
We support historical slugs, so the slug passed into the dataloader might not be the slug returned to the response. Given that the order of results isn't guaranteed the dataloader needs some way to match the original key to the results, which it couldn't do if the key was a historical slug.

This PR resolves 1-3. 4 was fixed in @mzikherman's PR and 5 is still an open issue.

TODO

Test against a few queries to ensure behavior is as expected

src/lib/loaders/batchLoader.ts

zephraph · 2019-02-07T04:34:24Z

I'm fairly certain I need more coverage on error cases. Two things come to mind.

What happens when null is requested from the dataloader?
What happens when the gravity response fails?

It feels like for 1 the gravity loader should just return the default value as a response and for 2 we may need to resolve with an error object... I'll have to dig into how the gravity loader actually handles that.

src/lib/loaders/__tests__/batchLoader.test.ts

damassi · 2019-02-07T06:17:43Z

src/lib/loaders/batchLoader.ts

+ * Otherwise we stringify the object so it can be compared via strict comparison.
+ */
+export const cacheKeyFn = (key: Key): string => {
+ if (typeof key === "object" && key !== null) {


http://2ality.com/2013/04/quirk-undefined.html

I'm not sure what this relates to. If the key is undefined, I'm just returning it like any other value. I'm not sure how to do error handling here, because if I throw an exception it's going to fail the entire query... maybe that's what we want though? Hmmm...

src/lib/loaders/__tests__/batchLoader.test.ts

src/lib/loaders/batchLoader.ts

alloy · 2019-02-07T10:39:49Z

What happens when null is requested from the dataloader?

If you mean that the key is null then that constitutes an argument error, in my mind. I think the types should disallow it and your code should throw an error.

What happens when the gravity response fails?

Do you mean a non 2xx HTTP response? The promise will reject. (I’m going to change the way that check happens btw, no need to generate a string and compile a regex for each request.)

Co-Authored-By: zephraph <[email protected]>

zephraph · 2019-02-07T15:58:28Z

@alloy if the gravity request rejects I'll likely need to handle that separately or it'll cause the promise.all of the batch loader to fail which would cause all of the batched calls to not return.

Co-Authored-By: zephraph <[email protected]>

zephraph · 2019-04-08T16:05:11Z

@alloy I figured it out! It was a simple mistake on my part but the error wasn't very clear.

alloy · 2019-04-09T15:20:31Z

@zephraph Can you confirm if this is done and needs a final review + merge? (I’m slightly confused because it’s still self-assigned.)

zephraph · 2019-04-10T14:39:48Z

@alloy it is done and ready for review. Sorry about that!

alloy · 2019-04-10T14:45:38Z

src/lib/helpers.ts

+ : stringify(options, {
+ arrayFormat: "brackets",
+ sort: (a, b) => a.localeCompare(b),
+ })


TIL. Fancy ✨

src/lib/loaders/__tests__/batchLoader.test.ts

src/lib/loaders/batchLoader.ts

Co-Authored-By: zephraph <[email protected]>

alloy · 2019-04-10T20:06:13Z

src/lib/loaders/__tests__/batchLoader.test.ts

@@ -82,6 +82,19 @@ describe("batchLoader", () => {
 })
 })

+describe("serializeParams", () => {
+ const { serializeParams } = require("../batchLoader")


Btw, why are you loading these functions this way? If you would import them as normal, you could do describe(serializeParams, () => …) instead and have the name of the tests updated automatically if we’d ever rename the function (using the refactor tools).

That's a good tip!

I'm doing the imports this way because earlier in the tests I mock the dependency to override the feature flag.

https:/artsy/metaphysics/blob/batching-bugfix/src/lib/loaders/__tests__/batchLoader.test.ts#L10

Oh, but actually it doesn’t seem like you’d need to do any mocking for that, you can just change the value and reset it from an afterEach?

Anyways, not a blocker for this PR.

alloy · 2019-04-10T20:10:13Z

@zephraph FYI, re “arg names”–did you know ‘arguments’ are the values that are passed in, whereas ’parameters’ are the names of the variables in a function’s signature? Took me a while to realise this, but really helps.

zephraph · 2019-04-10T20:23:48Z

Ohh, hmmm... I haven't thought about that. JavaScript exposes arguments as a way to access parameters. That makes it weird.

In the context of this file params actually refer to query parameters, not function parameters. I probably haven't made that very clear.

alloy · 2019-04-10T20:26:22Z

Ohh, hmmm... I haven't thought about that. JavaScript exposes arguments as a way to access parameters. That makes it weird.

Well, not really, it’s a list of the values, so these are the arguments.

In the context of this file params actually refer to query parameters, not function parameters. I probably haven't made that very clear.

Nah, that was clear enough to me. I feel like params is well understood to refer to HTTP parameters 👍

alloy · 2019-04-11T10:50:45Z

As per artsy/README#183, may I squash and merge this one?

zephraph added 4 commits February 5, 2019 15:10

Fix issue where queriedGroup sometimes isn't an array

0409de1

Refactor batch loader to handle duplicates, slugs

64933a7

Remove unused imports

d4414df

Fix accidental import, typoes

61634d2

zephraph self-assigned this Feb 7, 2019

zephraph requested review from alloy, joeyAghion and mzikherman February 7, 2019 04:30

zephraph commented Feb 7, 2019

View reviewed changes

src/lib/loaders/batchLoader.ts Show resolved Hide resolved

zephraph changed the title ~~[WIP] Fix batching issues~~ [WIP] Correct resolver batching behavior Feb 7, 2019

damassi reviewed Feb 7, 2019

View reviewed changes

src/lib/loaders/__tests__/batchLoader.test.ts Show resolved Hide resolved

damassi reviewed Feb 7, 2019

View reviewed changes

alloy reviewed Feb 7, 2019

View reviewed changes

src/lib/loaders/__tests__/batchLoader.test.ts Outdated Show resolved Hide resolved

alloy reviewed Feb 7, 2019

View reviewed changes

src/lib/loaders/__tests__/batchLoader.test.ts Outdated Show resolved Hide resolved

alloy reviewed Feb 7, 2019

View reviewed changes

src/lib/loaders/batchLoader.ts Outdated Show resolved Hide resolved

alloy reviewed Feb 7, 2019

View reviewed changes

src/lib/loaders/batchLoader.ts Outdated Show resolved Hide resolved

alloy reviewed Feb 7, 2019

View reviewed changes

src/lib/loaders/batchLoader.ts Show resolved Hide resolved

alloy reviewed Feb 7, 2019

View reviewed changes

src/lib/loaders/batchLoader.ts Outdated Show resolved Hide resolved

alloy reviewed Feb 7, 2019

View reviewed changes

src/lib/loaders/batchLoader.ts Outdated Show resolved Hide resolved

alloy reviewed Feb 7, 2019

View reviewed changes

src/lib/loaders/batchLoader.ts Outdated Show resolved Hide resolved

alloy reviewed Feb 7, 2019

View reviewed changes

src/lib/loaders/batchLoader.ts Outdated Show resolved Hide resolved

alloy and others added 3 commits February 7, 2019 10:47

Update src/lib/loaders/__tests__/batchLoader.test.ts

853ddfe

Co-Authored-By: zephraph <[email protected]>

Update src/lib/loaders/batchLoader.ts

e15e609

Co-Authored-By: zephraph <[email protected]>

Update src/lib/loaders/batchLoader.ts

f3e17b7

Co-Authored-By: zephraph <[email protected]>

alloy and others added 3 commits February 7, 2019 10:58

Update src/lib/loaders/batchLoader.ts

cfed7e8

Co-Authored-By: zephraph <[email protected]>

Add newline

1588751

Merge branch 'master' into batching-bugfix

3253bbe

artsy deleted a comment from peril-staging bot Apr 6, 2019

zephraph changed the title ~~[WIP] Correct resolver batching behavior~~ Correct resolver batching behavior Apr 8, 2019

zephraph added 2 commits April 8, 2019 11:12

Add missing async

b063d48

Remove unused type

a9db97b

zephraph assigned alloy and unassigned zephraph Apr 10, 2019

alloy reviewed Apr 10, 2019

View reviewed changes

src/lib/helpers.ts

: stringify(options, {

arrayFormat: "brackets",

sort: (a, b) => a.localeCompare(b),

})

Copy link

Contributor

alloy Apr 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL. Fancy ✨

alloy reviewed Apr 10, 2019

View reviewed changes

src/lib/loaders/__tests__/batchLoader.test.ts Show resolved Hide resolved

alloy reviewed Apr 10, 2019

View reviewed changes

src/lib/loaders/batchLoader.ts Outdated Show resolved Hide resolved

alloy reviewed Apr 10, 2019

View reviewed changes

src/lib/loaders/batchLoader.ts Outdated Show resolved Hide resolved

alloy reviewed Apr 10, 2019

View reviewed changes

src/lib/loaders/batchLoader.ts Outdated Show resolved Hide resolved

alloy reviewed Apr 10, 2019

View reviewed changes

src/lib/loaders/batchLoader.ts Outdated Show resolved Hide resolved

alloy reviewed Apr 10, 2019

View reviewed changes

src/lib/loaders/batchLoader.ts Outdated Show resolved Hide resolved

alloy reviewed Apr 10, 2019

View reviewed changes

src/lib/loaders/batchLoader.ts Outdated Show resolved Hide resolved

alloy and others added 5 commits April 10, 2019 14:38

Update src/lib/loaders/batchLoader.ts

aeba69f

Co-Authored-By: zephraph <[email protected]>

Update src/lib/loaders/batchLoader.ts

028ee26

Merge branch 'master' into batching-bugfix

9d1507f

Fix up docs, arg names

5e807a1

Add tests for serializeParams

15ee783

alloy reviewed Apr 10, 2019

View reviewed changes

Update src/lib/loaders/batchLoader.ts

8c55da8

alloy merged commit 6e55642 into master Apr 11, 2019

alloy deleted the batching-bugfix branch April 11, 2019 13:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correct resolver batching behavior #1495

Correct resolver batching behavior #1495

zephraph commented Feb 7, 2019 •

edited

Loading

zephraph commented Feb 7, 2019

damassi Feb 7, 2019

zephraph Feb 7, 2019

alloy commented Feb 7, 2019

zephraph commented Feb 7, 2019

zephraph commented Apr 8, 2019

alloy commented Apr 9, 2019

zephraph commented Apr 10, 2019

alloy Apr 10, 2019

alloy Apr 10, 2019

zephraph Apr 10, 2019

alloy Apr 11, 2019

alloy commented Apr 10, 2019

zephraph commented Apr 10, 2019

alloy commented Apr 10, 2019

alloy commented Apr 11, 2019

Correct resolver batching behavior #1495

Correct resolver batching behavior #1495

Conversation

zephraph commented Feb 7, 2019 • edited Loading

zephraph commented Feb 7, 2019

damassi Feb 7, 2019

Choose a reason for hiding this comment

zephraph Feb 7, 2019

Choose a reason for hiding this comment

alloy commented Feb 7, 2019

zephraph commented Feb 7, 2019

zephraph commented Apr 8, 2019

alloy commented Apr 9, 2019

zephraph commented Apr 10, 2019

alloy Apr 10, 2019

Choose a reason for hiding this comment

alloy Apr 10, 2019

Choose a reason for hiding this comment

zephraph Apr 10, 2019

Choose a reason for hiding this comment

alloy Apr 11, 2019

Choose a reason for hiding this comment

alloy commented Apr 10, 2019

zephraph commented Apr 10, 2019

alloy commented Apr 10, 2019

alloy commented Apr 11, 2019

zephraph commented Feb 7, 2019 •

edited

Loading