fix(links): Fix issue with link resolution

Bad use of memoization which prevented it from being used correctly,
leading to issues with larger data structures

Author: trodrigues

lib/mixins/link-getters.js

@@ -9,89 +9,92 @@ import {partial, memoize} from 'lodash/function'
  * @param {Object} includes - Object with lists of Entry, Asset, DeletedEntry and DeletedAsset
  */
 export default function mixinLinkGetters (items, includes) {
-  each(items, item => setLocalizedFieldGetters(item.fields, includes, !!item.sys.locale))
-}
-
-/**
- * If a field does not have a locale defined in sys, the content of that field
- * is an object where the keys are each available locale, and we need to iterate
- * over each of those
- * @private
- * @param {Object} fields - Fields object
- * @param {Object} includes - Object with lists of Entry, Asset, DeletedEntry and DeletedAsset
- * @param {boolean} hasLocale - If entry has been requested with a locale
- */
-function setLocalizedFieldGetters (fields, includes, hasLocale) {
-  if (hasLocale) {
-    setFieldGettersForFields(fields, includes)
-  } else {
-    each(fields, localizedField => setFieldGettersForFields(localizedField, includes))
-  }
-}
+  const linkGetter = memoize(getLinksFromIncludes, memoizationResolver)
+  each(items, item => setLocalizedFieldGetters(item.fields, !!item.sys.locale))
 
-/**
- * Sets getters on each link field or list of link fields for each item
- * @private
- * @param {Object} fields - Fields object
- * @param {Object} includes - Object with lists of Entry, Asset, DeletedEntry and DeletedAsset
- */
-function setFieldGettersForFields (fields, includes) {
-  each(fields, (field, fieldKey) => {
-    if (Array.isArray(field)) {
-      addGetterForLinkArray(field, fieldKey, fields, includes)
+  /**
+   * If a field does not have a locale defined in sys, the content of that field
+   * is an object where the keys are each available locale, and we need to iterate
+   * over each of those
+   * @private
+   * @param {Object} fields - Fields object
+   * @param {boolean} hasLocale - If entry has been requested with a locale
+   */
+  function setLocalizedFieldGetters (fields, hasLocale) {
+    if (hasLocale) {
+      setFieldGettersForFields(fields)
     } else {
-      addGetterForLink(field, fieldKey, fields, includes)
+      each(fields, localizedField => setFieldGettersForFields(localizedField))
     }
-  })
-}
+  }
 
-/**
- * Sets a getter which resolves the link of the given fieldKey in fields
- * @private
- * @param {Object} field - Field object
- * @param {string} fieldKey
- * @param {Object} fields - Fields object
- * @param {Object} includes - Object with lists of Entry, Asset, DeletedEntry and DeletedAsset
- */
-function addGetterForLink (field, fieldKey, fields, includes) {
-  if (get(field, 'sys.type') === 'Link') {
-    Object.defineProperty(fields, fieldKey, {
-      get: memoize(partial(linkGetter, includes, field))
+  /**
+   * Sets getters on each link field or list of link fields for each item
+   * @private
+   * @param {Object} fields - Fields object
+   */
+  function setFieldGettersForFields (fields) {
+    each(fields, (field, fieldKey) => {
+      if (Array.isArray(field)) {
+        addGetterForLinkArray(field, fieldKey, fields)
+      } else {
+        addGetterForLink(field, fieldKey, fields)
+      }
     })
   }
-}
 
-/**
- * Sets a getter which resolves the array of links of the given fieldKey in fields
- * @private
- * @param {Array<Object>} field - List field array
- * @param {string} fieldKey
- * @param {Object} fields - Fields object
- * @param {Object} includes - Object with lists of Entry, Asset, DeletedEntry and DeletedAsset
- */
-function addGetterForLinkArray (listField, fieldKey, fields, includes) {
-  if (get(listField[0], 'sys.type') === 'Link') {
-    Object.defineProperty(fields, fieldKey, {
-      get: memoize(function () {
-        return map(listField, partial(linkGetter, includes))
+  /**
+   * Sets a getter which resolves the link of the given fieldKey in fields
+   * @private
+   * @param {Object} field - Field object
+   * @param {string} fieldKey
+   * @param {Object} fields - Fields object
+   */
+  function addGetterForLink (field, fieldKey, fields) {
+    if (get(field, 'sys.type') === 'Link') {
+      Object.defineProperty(fields, fieldKey, {
+        get: partial(linkGetter, field)
       })
-    })
+    }
   }
-}
 
-/**
- * Looks for given link field in includes.
- * If linked entity is not found, it returns the original link.
- * @private
- * @param {Object} field - Field object
- * @param {Object} includes - Object with lists of Entry, Asset, DeletedEntry and DeletedAsset
- * @return {Object} Field, or link if field not resolved
- */
-function linkGetter (includes, field) {
-  var link = find(includes[field.sys.linkType], ['sys.id', field.sys.id])
-  if (link && link.fields) {
-    setLocalizedFieldGetters(link.fields, includes, !!link.sys.locale)
-    return link
+  /**
+   * Sets a getter which resolves the array of links of the given fieldKey in fields
+   * @private
+   * @param {Array<Object>} field - List field array
+   * @param {string} fieldKey
+   * @param {Object} fields - Fields object
+   */
+  function addGetterForLinkArray (listField, fieldKey, fields) {
+    if (get(listField[0], 'sys.type') === 'Link') {
+      Object.defineProperty(fields, fieldKey, {
+        get: function () {
+          return map(listField, partial(linkGetter))
+        }
+      })
+    }
+  }
+
+  /**
+   * Looks for given link field in includes.
+   * If linked entity is not found, it returns the original link.
+   * This method shouldn't be used directly, and it's memoized whenever this
+   * module's main method is used.
+   * This is done to prevent the same link being resolved multiple times.
+   * @private
+   * @param {Object} field - Field object
+   * @return {Object} Field, or link if field not resolved
+   */
+  function getLinksFromIncludes (field) {
+    var link = find(includes[field.sys.linkType], ['sys.id', field.sys.id])
+    if (link && link.fields) {
+      setLocalizedFieldGetters(link.fields, !!link.sys.locale)
+      return link
+    }
+    return field
+  }
+
+  function memoizationResolver (link) {
+    return link.sys.id
   }
-  return field
 }

test/unit/entities/entry-test.js

@@ -37,7 +37,7 @@ test('Entry collection is wrapped', t => {
   t.end()
 })
 
-test('Entry collection links are resolved', t => {
+test.only('Entry collection links are resolved', t => {
   const entryCollection = {
     total: 1,
     skip: 0,
@@ -47,6 +47,7 @@ test('Entry collection links are resolved', t => {
       cloneDeep(entryMock)
     ],
     includes: {
+      Entry: [ cloneDeep(entryMock) ],
       Asset: [ cloneDeep(assetMock) ]
     }
   }
@@ -70,6 +71,21 @@ test('Entry collection links are resolved', t => {
   entryCollection.includes.Asset[0].sys.id = 'asset1'
   // setup second linked entry
   entryCollection.items[1].sys.id = 'entry3'
+  entryCollection.items[1].fields.linked1 = {
+    sys: {
+      id: 'entry4',
+      type: 'Link',
+      linkType: 'Entry'
+    }
+  }
+  entryCollection.includes.Entry[0].sys.id = 'entry4'
+  entryCollection.includes.Entry[0].fields.linked1 = {
+    sys: {
+      id: 'entry3',
+      type: 'Link',
+      linkType: 'Entry'
+    }
+  }
 
   const wrappedEntry = wrapEntryCollection(entryCollection, true).toPlainObject()
   // first linked entry resolved from includes
@@ -78,5 +94,7 @@ test('Entry collection links are resolved', t => {
   // second linked entry resolved from items list
   t.equals(wrappedEntry.items[0].fields.linked2.sys.type, 'Entry', 'second linked entity is resolved')
   t.ok(wrappedEntry.items[0].fields.linked2.fields, 'second linked entity has fields')
+  t.equals(wrappedEntry.items[1].fields.linked1.sys.type, 'Entry', 'third linked entity is resolved')
+  t.ok(wrappedEntry.items[1].fields.linked1.fields, 'third linked entity has fields')
   t.end()
 })