Correct resolver batching behavior (#1495)

* Fix issue where queriedGroup sometimes isn't an array

* Refactor batch loader to handle duplicates, slugs

* Resolve corner cases with batch loader

1. Normalizes the input to the loader
2. Removes the notion of defaults
3. Ensures list results return an array
4. Ensures multiple items are batched correctly with loadMany

* Update batching to work with gravity batch param

src/data/complete.queryMap.json

@@ -483,4 +483,4 @@
   "4534ea8e115e15baa7f77f2a652902a6": "query ShowTestsQuery {\n  show(id: \"anderson-fine-art-gallery-flickinger-collection\") {\n    ...Show_show\n    __id\n  }\n}\n\nfragment Show_show on Show {\n  ...Detail_show\n  ...MoreInfo_show\n  ...ShowArtists_show\n  ...ShowArtworks_show\n  __id\n}\n\nfragment Detail_show on Show {\n  _id\n  id\n  name\n  description\n  city\n  is_local_discovery\n  images {\n    id\n  }\n  ...ShowHeader_show\n  ...ShowArtworksPreview_show\n  ...ShowArtistsPreview_show\n  ...Shows_show\n  location {\n    ...LocationMap_location\n    openingHours {\n      __typename\n      ... on OpeningHoursArray {\n        schedules {\n          days\n          hours\n        }\n      }\n      ... on OpeningHoursText {\n        text\n      }\n    }\n    __id\n  }\n  artists_without_artworks {\n    id\n    __id\n  }\n  counts {\n    artworks\n    artists\n  }\n  status\n  partner {\n    __typename\n    ... on Partner {\n      name\n      type\n    }\n    ... on Node {\n      __id\n    }\n    ... on ExternalPartner {\n      __id\n    }\n  }\n  __id\n}\n\nfragment MoreInfo_show on Show {\n  _id\n  id\n  exhibition_period\n  pressReleaseUrl\n  openingReceptionText\n  partner {\n    __typename\n    ... on Partner {\n      website\n      type\n    }\n    ... on Node {\n      __id\n    }\n    ... on ExternalPartner {\n      __id\n    }\n  }\n  press_release\n  events {\n    ...ShowEventSection_event\n  }\n  __id\n}\n\nfragment ShowArtists_show on Show {\n  _id\n  id\n  artists_grouped_by_name {\n    letter\n    items {\n      ...ArtistListItem_artist\n      sortable_id\n      href\n      __id\n    }\n  }\n  __id\n}\n\nfragment ShowArtworks_show on Show {\n  __id\n  id\n  _id\n  filteredArtworks(size: 0, medium: \"*\", price_range: \"*-*\", aggregations: [MEDIUM, PRICE_RANGE, TOTAL]) {\n    ...FilteredInfiniteScrollGrid_filteredArtworks\n    __id\n  }\n}\n\nfragment FilteredInfiniteScrollGrid_filteredArtworks on FilterArtworks {\n  ...Filters_filteredArtworks\n  ...ArtworksGridPaginationContainer_filteredArtworks\n  __id\n}\n\nfragment Filters_filteredArtworks on FilterArtworks {\n  aggregations {\n    slice\n    counts {\n      id\n      name\n      __id\n    }\n  }\n  __id\n}\n\nfragment ArtworksGridPaginationContainer_filteredArtworks on FilterArtworks {\n  __id\n  artworks: artworks_connection(first: 10) {\n    pageInfo {\n      hasNextPage\n      startCursor\n      endCursor\n    }\n    edges {\n      node {\n        id\n        __id\n        image {\n          aspect_ratio\n        }\n        ...Artwork_artwork\n        __typename\n      }\n      cursor\n    }\n  }\n}\n\nfragment Artwork_artwork on Artwork {\n  title\n  date\n  sale_message\n  is_in_auction\n  is_biddable\n  is_acquireable\n  is_offerable\n  id\n  sale {\n    is_auction\n    is_live_open\n    is_open\n    is_closed\n    display_timely_at\n    __id\n  }\n  sale_artwork {\n    opening_bid {\n      display\n    }\n    current_bid {\n      display\n    }\n    bidder_positions_count\n    sale {\n      is_closed\n      __id\n    }\n    __id\n  }\n  image {\n    url(version: \"large\")\n    aspect_ratio\n  }\n  artists(shallow: true) {\n    name\n    __id\n  }\n  partner {\n    name\n    __id\n  }\n  href\n  __id\n}\n\nfragment ArtistListItem_artist on Artist {\n  __id\n  _id\n  id\n  name\n  is_followed\n  nationality\n  birthday\n  deathday\n  image {\n    url\n  }\n}\n\nfragment ShowEventSection_event on PartnerShowEventType {\n  event_type\n  description\n  start_at\n  end_at\n}\n\nfragment ShowHeader_show on Show {\n  id\n  _id\n  __id\n  name\n  press_release\n  is_followed\n  end_at\n  exhibition_period\n  status\n  isStubShow\n  partner {\n    __typename\n    ... on Partner {\n      name\n      id\n      href\n    }\n    ... on Node {\n      __id\n    }\n    ... on ExternalPartner {\n      __id\n    }\n  }\n  images {\n    url\n    aspect_ratio\n  }\n  followedArtists(first: 3) {\n    edges {\n      node {\n        artist {\n          name\n          href\n          id\n          _id\n          __id\n        }\n      }\n    }\n  }\n  artists {\n    name\n    href\n    id\n    _id\n    __id\n  }\n}\n\nfragment ShowArtworksPreview_show on Show {\n  __id\n  artworks(size: 6) {\n    ...GenericGrid_artworks\n    __id\n  }\n  counts {\n    artworks\n  }\n}\n\nfragment ShowArtistsPreview_show on Show {\n  _id\n  id\n  artists {\n    _id\n    id\n    href\n    ...ArtistListItem_artist\n    __id\n  }\n  artists_without_artworks {\n    _id\n    id\n    href\n    ...ArtistListItem_artist\n    __id\n  }\n  __id\n}\n\nfragment Shows_show on Show {\n  nearbyShows(first: 20) {\n    edges {\n      node {\n        __id\n        ...ShowItem_show\n      }\n    }\n  }\n  __id\n}\n\nfragment LocationMap_location on Location {\n  __id\n  id\n  city\n  address\n  address_2\n  postal_code\n  summary\n  coordinates {\n    lat\n    lng\n  }\n  day_schedules {\n    start_time\n    end_time\n    day_of_week\n  }\n  openingHours {\n    __typename\n    ... on OpeningHoursArray {\n      schedules {\n        days\n        hours\n      }\n    }\n    ... on OpeningHoursText {\n      text\n    }\n  }\n}\n\nfragment ShowItem_show on Show {\n  _id\n  id\n  name\n  exhibition_period\n  end_at\n  images {\n    url\n    aspect_ratio\n  }\n  partner {\n    __typename\n    ... on Partner {\n      name\n    }\n    ... on Node {\n      __id\n    }\n    ... on ExternalPartner {\n      __id\n    }\n  }\n  __id\n}\n\nfragment GenericGrid_artworks on Artwork {\n  __id\n  id\n  image {\n    aspect_ratio\n  }\n  ...Artwork_artwork\n}\n",
   "edb302bf331c6fc8a0c7ecaf3c107bcb": "query ShowsQuery(\n  $count: Int!\n  $cursor: String\n) {\n  me {\n    ...Shows_me_1G22uz\n    __id\n  }\n}\n\nfragment Shows_me_1G22uz on Me {\n  followsAndSaves {\n    shows(first: $count, after: $cursor) {\n      pageInfo {\n        startCursor\n        endCursor\n        hasPreviousPage\n        hasNextPage\n      }\n      edges {\n        node {\n          __id\n          ...ShowItemRow_show\n          __typename\n        }\n        cursor\n      }\n    }\n  }\n  __id\n}\n\nfragment ShowItemRow_show on Show {\n  id\n  _id\n  __id\n  is_followed\n  name\n  isStubShow\n  partner {\n    __typename\n    ... on Partner {\n      name\n      profile {\n        image {\n          url(version: \"square\")\n        }\n        __id\n      }\n    }\n    ... on Node {\n      __id\n    }\n    ... on ExternalPartner {\n      __id\n    }\n  }\n  href\n  exhibition_period\n  status\n  cover_image {\n    url\n    aspect_ratio\n  }\n  is_fair_booth\n  start_at\n  end_at\n}\n",
   "e5c878c2ad186621b8ed4f9737f81519": "query indexTestsQuery {\n  show(id: \"anderson-fine-art-gallery-flickinger-collection\") {\n    ...Show_show\n    __id\n  }\n}\n\nfragment Show_show on Show {\n  ...Detail_show\n  ...MoreInfo_show\n  ...ShowArtists_show\n  ...ShowArtworks_show\n  __id\n}\n\nfragment Detail_show on Show {\n  _id\n  id\n  name\n  description\n  city\n  is_local_discovery\n  images {\n    id\n  }\n  ...ShowHeader_show\n  ...ShowArtworksPreview_show\n  ...ShowArtistsPreview_show\n  ...Shows_show\n  location {\n    ...LocationMap_location\n    openingHours {\n      __typename\n      ... on OpeningHoursArray {\n        schedules {\n          days\n          hours\n        }\n      }\n      ... on OpeningHoursText {\n        text\n      }\n    }\n    __id\n  }\n  artists_without_artworks {\n    id\n    __id\n  }\n  counts {\n    artworks\n    artists\n  }\n  status\n  partner {\n    __typename\n    ... on Partner {\n      name\n      type\n    }\n    ... on Node {\n      __id\n    }\n    ... on ExternalPartner {\n      __id\n    }\n  }\n  __id\n}\n\nfragment MoreInfo_show on Show {\n  _id\n  id\n  exhibition_period\n  pressReleaseUrl\n  openingReceptionText\n  partner {\n    __typename\n    ... on Partner {\n      website\n      type\n    }\n    ... on Node {\n      __id\n    }\n    ... on ExternalPartner {\n      __id\n    }\n  }\n  press_release\n  events {\n    ...ShowEventSection_event\n  }\n  __id\n}\n\nfragment ShowArtists_show on Show {\n  _id\n  id\n  artists_grouped_by_name {\n    letter\n    items {\n      ...ArtistListItem_artist\n      sortable_id\n      href\n      __id\n    }\n  }\n  __id\n}\n\nfragment ShowArtworks_show on Show {\n  __id\n  id\n  _id\n  filteredArtworks(size: 0, medium: \"*\", price_range: \"*-*\", aggregations: [MEDIUM, PRICE_RANGE, TOTAL]) {\n    ...FilteredInfiniteScrollGrid_filteredArtworks\n    __id\n  }\n}\n\nfragment FilteredInfiniteScrollGrid_filteredArtworks on FilterArtworks {\n  ...Filters_filteredArtworks\n  ...ArtworksGridPaginationContainer_filteredArtworks\n  __id\n}\n\nfragment Filters_filteredArtworks on FilterArtworks {\n  aggregations {\n    slice\n    counts {\n      id\n      name\n      __id\n    }\n  }\n  __id\n}\n\nfragment ArtworksGridPaginationContainer_filteredArtworks on FilterArtworks {\n  __id\n  artworks: artworks_connection(first: 10) {\n    pageInfo {\n      hasNextPage\n      startCursor\n      endCursor\n    }\n    edges {\n      node {\n        id\n        __id\n        image {\n          aspect_ratio\n        }\n        ...Artwork_artwork\n        __typename\n      }\n      cursor\n    }\n  }\n}\n\nfragment Artwork_artwork on Artwork {\n  title\n  date\n  sale_message\n  is_in_auction\n  is_biddable\n  is_acquireable\n  is_offerable\n  id\n  sale {\n    is_auction\n    is_live_open\n    is_open\n    is_closed\n    display_timely_at\n    __id\n  }\n  sale_artwork {\n    opening_bid {\n      display\n    }\n    current_bid {\n      display\n    }\n    bidder_positions_count\n    sale {\n      is_closed\n      __id\n    }\n    __id\n  }\n  image {\n    url(version: \"large\")\n    aspect_ratio\n  }\n  artists(shallow: true) {\n    name\n    __id\n  }\n  partner {\n    name\n    __id\n  }\n  href\n  __id\n}\n\nfragment ArtistListItem_artist on Artist {\n  __id\n  _id\n  id\n  name\n  is_followed\n  nationality\n  birthday\n  deathday\n  image {\n    url\n  }\n}\n\nfragment ShowEventSection_event on PartnerShowEventType {\n  event_type\n  description\n  start_at\n  end_at\n}\n\nfragment ShowHeader_show on Show {\n  id\n  _id\n  __id\n  name\n  press_release\n  is_followed\n  end_at\n  exhibition_period\n  status\n  isStubShow\n  partner {\n    __typename\n    ... on Partner {\n      name\n      id\n      href\n    }\n    ... on Node {\n      __id\n    }\n    ... on ExternalPartner {\n      __id\n    }\n  }\n  images {\n    url\n    aspect_ratio\n  }\n  followedArtists(first: 3) {\n    edges {\n      node {\n        artist {\n          name\n          href\n          id\n          _id\n          __id\n        }\n      }\n    }\n  }\n  artists {\n    name\n    href\n    id\n    _id\n    __id\n  }\n}\n\nfragment ShowArtworksPreview_show on Show {\n  __id\n  artworks(size: 6) {\n    ...GenericGrid_artworks\n    __id\n  }\n  counts {\n    artworks\n  }\n}\n\nfragment ShowArtistsPreview_show on Show {\n  _id\n  id\n  artists {\n    _id\n    id\n    href\n    ...ArtistListItem_artist\n    __id\n  }\n  artists_without_artworks {\n    _id\n    id\n    href\n    ...ArtistListItem_artist\n    __id\n  }\n  __id\n}\n\nfragment Shows_show on Show {\n  nearbyShows(first: 20) {\n    edges {\n      node {\n        __id\n        ...ShowItem_show\n      }\n    }\n  }\n  __id\n}\n\nfragment LocationMap_location on Location {\n  __id\n  id\n  city\n  address\n  address_2\n  postal_code\n  summary\n  coordinates {\n    lat\n    lng\n  }\n  day_schedules {\n    start_time\n    end_time\n    day_of_week\n  }\n  openingHours {\n    __typename\n    ... on OpeningHoursArray {\n      schedules {\n        days\n        hours\n      }\n    }\n    ... on OpeningHoursText {\n      text\n    }\n  }\n}\n\nfragment ShowItem_show on Show {\n  _id\n  id\n  name\n  exhibition_period\n  end_at\n  images {\n    url\n    aspect_ratio\n  }\n  partner {\n    __typename\n    ... on Partner {\n      name\n    }\n    ... on Node {\n      __id\n    }\n    ... on ExternalPartner {\n      __id\n    }\n  }\n  __id\n}\n\nfragment GenericGrid_artworks on Artwork {\n  __id\n  id\n  image {\n    aspect_ratio\n  }\n  ...Artwork_artwork\n}\n"
-}
+}

src/data/vortex.graphql

@@ -173,7 +173,12 @@ type Query {
   """
   Pricing Context Histograms
   """
-  pricingContext(artistId: String!, category: PricingContextCategoryEnum!, heightCm: Int!, widthCm: Int!): PricingContext
+  pricingContext(
+    artistId: String!
+    category: PricingContextCategoryEnum!
+    heightCm: Int!
+    widthCm: Int!
+  ): PricingContext
 }
 
 enum QueryPeriodEnum {

src/lib/cache.ts

@@ -134,7 +134,9 @@ function _set(key, data, options: CacheOptions) {
   const timestamp = new Date().getTime()
   /* eslint-disable no-param-reassign */
   if (isArray(data)) {
-    data.forEach(datum => (datum.cached = timestamp))
+    data.forEach(datum => {
+      datum && (datum.cached = timestamp)
+    })
   } else {
     data.cached = timestamp
   }

src/lib/helpers.ts

@@ -78,10 +78,19 @@ export const truncate = (string, length, append = "…") => {
   return x.length > limit ? x.slice(0, limit) + append : x
 }
 export const toQueryString = (options = {}) =>
-  stringify(options, {
-    arrayFormat: "brackets",
-    sort: (a, b) => a.localeCompare(b),
-  })
+  /**
+   * In the case of batched requests we want to explicitly _not_ sort the
+   * params because the order matters to dataloader
+   */
+  // @ts-ignore
+  options.batched
+    ? stringify(options, {
+        arrayFormat: "brackets",
+      })
+    : stringify(options, {
+        arrayFormat: "brackets",
+        sort: (a, b) => a.localeCompare(b),
+      })
 export const toKey = (path, options = {}) => `${path}?${toQueryString(options)}`
 export const exclude = (values?: any[], property?: any) => xs =>
   reject(xs, x => includes(values, x[property]))

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

@@ -58,11 +58,11 @@ describe("batchLoader", () => {
       )
       const batch = batchLoader({ multipleLoader })
 
-      await batch({ id: "foo", param: "bar" })
+      await batch({ id: ["foo"], param: "bar" })
       expect(multipleLoader).toBeCalledWith({
+        batched: true,
         id: ["foo"],
         param: "bar",
-        size: 1,
       })
     })
 
@@ -79,55 +79,58 @@ describe("batchLoader", () => {
 
       expect(multipleLoader).toBeCalledTimes(1)
     })
+  })
+})
 
-    it("should return a default value when an id is missing", async () => {
-      const multipleLoader = jest.fn(({ id: ids }) =>
-        ids.slice(1).map(id => ({
-          _id: id,
-        }))
-      )
-      const batch = batchLoader({ multipleLoader, defaultResult: {} })
-
-      const results = await Promise.all([
-        batch("123"),
-        batch("456"),
-        batch("789"),
-      ])
-      expect(results).toEqual([
-        {},
-        {
-          _id: "456",
-        },
-        {
-          _id: "789",
-        },
-      ])
+describe("serializeParams", () => {
+  const { serializeParams } = require("../batchLoader")
 
-      expect(multipleLoader).toBeCalledWith({
-        id: ["123", "456", "789"],
-        size: 3,
-      })
-    })
+  it("should return an empty string for a param object with only id", () => {
+    expect(serializeParams({ id: "a" })).toBe("")
+  })
+  it("should return key=value for every entry in the params object", () => {
+    expect(serializeParams({ id: "a", foo: "bar", bleep: "bloop" })).toBe(
+      "bleep=bloop&foo=bar"
+    )
   })
 })
 
-const { groupKeys } = require("../batchLoader")
-describe("groupKeys", () => {
+describe("groupByParams", () => {
+  const { groupByParams } = require("../batchLoader")
   it("should handle a single key", () => {
-    expect(groupKeys(["a"])).toEqual([
-      {
-        id: ["a"],
-        size: 1,
-      },
+    expect(groupByParams([{ id: "a" }])).toEqual([
+      [""],
+      [
+        [
+          {
+            id: "a",
+          },
+        ],
+      ],
     ])
   })
 
   it("should group single keys together", () => {
-    expect(groupKeys(["a", "b", "c"])).toEqual([
+    const keys = [
       {
-        id: ["a", "b", "c"],
-        size: 3,
+        id: "a",
       },
+      {
+        id: "b",
+      },
+    ]
+    expect(groupByParams(keys)).toEqual([
+      [""],
+      [
+        [
+          {
+            id: "a",
+          },
+          {
+            id: "b",
+          },
+        ],
+      ],
     ])
   })
 
@@ -142,38 +145,75 @@ describe("groupKeys", () => {
         foo: "bar",
       },
     ]
-    expect(groupKeys(keys)).toEqual([
-      {
-        id: ["a", "b"],
-        foo: "bar",
-        size: 2,
-      },
+    expect(groupByParams(keys)).toEqual([
+      ["foo=bar"],
+      [
+        [
+          {
+            id: "a",
+            foo: "bar",
+          },
+
+          {
+            id: "b",
+            foo: "bar",
+          },
+        ],
+      ],
     ])
   })
 
   it("should separate keys with different parameters", () => {
     const keys = [
-      "a",
+      { id: "a" },
       { id: "b", foo: "bar" },
       { id: "c", foo: "bar", boo: "baz" },
     ]
 
-    expect(groupKeys(keys)).toEqual([
-      {
-        id: ["a"],
-        size: 1,
-      },
-      {
-        id: ["b"],
-        foo: "bar",
-        size: 1,
-      },
-      {
-        id: ["c"],
-        foo: "bar",
-        boo: "baz",
-        size: 1,
-      },
+    expect(groupByParams(keys)).toEqual([
+      ["", "foo=bar", "boo=baz&foo=bar"],
+      [
+        [
+          {
+            id: "a",
+          },
+        ],
+        [
+          {
+            id: "b",
+            foo: "bar",
+          },
+        ],
+        [
+          {
+            id: "c",
+            foo: "bar",
+            boo: "baz",
+          },
+        ],
+      ],
     ])
   })
 })
+
+describe("cacheKeyFn", () => {
+  const { cacheKeyFn } = require("../batchLoader")
+  it("should not treat two objects with the same id but different params as different", () => {
+    expect(cacheKeyFn({ id: "123" })).not.toEqual(
+      cacheKeyFn({ id: "123", foo: "bar" })
+    )
+  })
+
+  it("should treat two objects with the same params and id as equal", () => {
+    expect(cacheKeyFn({ id: "123", foo: "bar" })).toEqual(
+      cacheKeyFn({ id: "123", foo: "bar" })
+    )
+  })
+
+  it("should treat two objects with different ids as different", () => {
+    expect(cacheKeyFn({ id: "456", foo: "bar" })).not.toEqual({
+      id: "567",
+      foo: "bar",
+    })
+  })
+})