vuex-orm / vuex-orm

The Vuex plugin to enable Object-Relational Mapping access to the Vuex Store.
https://vuex-orm.org
MIT License
2.36k stars 166 forks source link

ORM inserting pivot with inconsistent IDs #636

Open justsanjit opened 4 years ago

justsanjit commented 4 years ago

On my morphToMany relationship. Orm is saving duplicate pivot record under

Here is the screenshot of vuex store:

Capture

This is how my model looks like:

import Model from '@/models/Model'

export default class EmployeeHasRoles extends Model {
  static primaryKey = ['role_id', 'model_id', 'model_type']

  static fields () {
    return {
      role_id: this.attr(''),
      model_id: this.attr(''),
      model_type: this.attr(''),
    }
  }

    static entity = 'employee_has_roles';
}

I am wondering, If I am using composite keys property. Please help!

Correction:

There are actually two different endpoints that insert pivot record.

Capture

Somehow they are using different keys,

kiaking commented 4 years ago

Ah OK I think this is the nasty defect. Could you give us the data structure you tried to insert?

justsanjit commented 4 years ago

Hi @kiaking, Sorry for delay in response.

This is the data structure that I am trying to insert


{
  "data": [
    {
      "id": 852,
      "name": "Sanjit Singh",
      "email": "ssingh@example.com",
      "avatar_link": "\/avatar\/anon-employee.jpg",
      "disabled_at": null,
      "disabled_by": null,
      "imported_at": null,
      "created_by": null,
      "created_at": "2019-12-02 14:54:39",
      "updated_at": "2019-12-02 14:54:39",
      "deleted_at": null,
      "roles": [
        {
          "id": 1,
          "name": "Admin",
          "guard_name": "web",
          "created_at": "2020-05-14 10:29:39",
          "updated_at": "2020-05-14 10:29:39",
          "pivot": {
            "model_id": 852,
            "role_id": 1,
            "model_type": "employee"
          }
        },
        {
          "id": 2,
          "name": "Technician",
          "guard_name": "web",
          "created_at": "2020-05-15 12:49:45",
          "updated_at": "2020-05-15 12:49:45",
          "pivot": {
            "model_id": 852,
            "role_id": 2,
            "model_type": "employee"
          }
        }
      ]
    }
  ],
}

Employee.js

export default class Employee extends Model {
  static fields () {
    return {
      id: this.number(null).nullable(),
      name: this.string(''),
      email: this.string(''),
      avatar_link: this.string(''),
      disabled_at: this.attr(null),
      abilities: this.attr(() => []),
      created_at: this.attr(null).nullable(),
      created_by: this.attr('null').nullable(),
      disabled_by: this.attr('null').nullable(),

      // Roles: this.belongsToMany(Role, EmployeeHasRoles, 'model_id', 'role_id'),
      roles: this.morphToMany(Role, EmployeeHasRoles, 'role_id', 'model_id', 'model_type'),
    }
  }
}

EmployeeHasRoles.js

export default class EmployeeHasRoles extends Model {
  static primaryKey = ['role_id', 'model_id', 'model_type']

  static fields () {
    return {
      role_id: this.attr(''),
      model_id: this.attr(''),
      model_type: this.attr(''),
    }
  }

    static entity = 'mainstay_employee_has_roles';
}

Role.js

export default class Role extends Model {
  static fields () {
    return {
      id: this.number(null),
      name: this.string(''),
      created_at: this.string(''),
      permissions: this.belongsToMany(Permission, RolePermission, 'role_id', 'permission_id'),
    }
  }
}

I am using axios plugin. Here is the code that inserts the data into the vuex store

export default {

// ...

beforeRouteEnter (to, from, next) {
  Employee.api().get('/api/employees?include=roles', {
    dataKey: 'data',
  })
    .then((result) => {
      console.log(result)
      Employee.commit((state) => {
        state.total = result.response.data.total
        state.order = result.response.data.data.map((i) => i.id)
        state.lastPage = result.response.data.last_page
      })
      next()
    })
},

methods: {

  fetch (resetPage = false) {
    this.loading = true
    if (resetPage) {
      this.page = 1
    }

    Employee.api().get('/api/employees?include=roles', {
      dataKey: 'data',
    })
      .then((result) => {
        Employee.commit((state) => {
          state.total = result.response.data.total
          state.order = result.response.data.data.map((i) => i.id)
          state.lastPage = result.response.data.last_page
        })
        this.loading = false
      })
  },
}
kiaking commented 4 years ago

Thanks a lot! Currently, we're building the next iteration of Vuex ORM. It will be announced publicly soon with the draft version. We'll make sure to look into this one and won't let that happen in the Vuex ORM Next.

Sorry for taking time but please be patient 🙏

cuebit commented 4 years ago

@sanjitkung the polymorphic relations behave differently to other relations. Also, for some reason, intermediate models for poly relations do not adhere to composite keys as it will create its own combination for indexing which is contrary to what you’d expect.

That being said, just remove your composite primary key on your intermediate model, specifically this line:

static primaryKey = ['role_id', 'model_id', 'model_type']
justsanjit commented 4 years ago

Hi @cuebit Thanks for the response. I removed the primary key.

Now I get this one entry with null ID Capture

Null entry show up when I try to add the same record twice.

cuebit commented 4 years ago

@sanjitkung what is the side-effect to removing the pivot attribute altogether from your response? This adds little value since you have the primary and foreign keys in your response for the respective models for Vuex ORM to generate the pivot record.

mantaskon commented 3 years ago

I have the same issue... removing primaryKey from model definition doesn't work. Interestingly, morphToMany sets $id incorrectly (strings) initially (using "insert"), but uses the correct primary key definition when updating records ("insertOrUpdate").

@sanjitkung did you ever find a temp fix for this?

ldiebold commented 3 years ago

Having the same issue, yet understand that we may need to find a workaround until NEXT release. My current strategy is to just query for the models I want to update (by their modelable_id and modelable_type). However, this will have a performance hit since we're not longer fetching it directly by key!

adm-bome commented 3 years ago

Hi,

I also have the same same issue: But maybe i'm missing something. (any advice is welcome!)

Just tried the example in the docs for a Many To Many (Polymorphic) Tag / Tagging relation.

I noticed a difference in the normalization keys. between an Many to Many or a Many To Many (Polymorphic) the normalizr has different keys, so for example in the method updateIndexes() the comparison between key and id are never false.

As seen from the note above: The issues are only visible when you update the records. (ex. when you update a address with tags: all tags are added as new and you have doubled your relations)

I feel the issue #668 and PR #669 (as a possible solution to that issue), has also something to do with this issue. When we have pivot data that specifies the record id in the pivot table, things start to work but you end up with records pointing to the latest modification of the tag. All other/previous pivotRecords are created (ex 200+) but pointless, because they are not bound to any tag anymore.

And there is another feeling... in the code base comparisons are made between a normalize key and an record id. my feeling is that when missing an id or the record has an auto generated id the records can't be found or be matched correctly. the record has also an $id field. but it never used to match the records. but in the situation a composed key is used the lookup should be on the composedKey and not on the id field. If this makes any sense. The composed Key is also a thingy... sometimes it is an Json.stringify() and on other places it is a String.join('_'). very different and when comparing the outcome is always 'NOT EQUAL' ex: ('[12,4 ,"address"]' === '12_4_address')

// Reponse Webservice
{
  "id": 1,
  "name": "Test",
  "emailAddresses": [
    {
      "id": 1,
      "email": "fake@email.com",
      "tags": [
        {
          "id": 1,
          "name": "Default",
          "slug": "default"
        },
        {
          "id": 2,
          "name": "Billing",
          "slug": "billing"
        }
      ]
    }
  ],
  "addresses": [
    {
      "id": 1,
      "street": "StreetName",
      "tags": [
        {
          "id": 1,
          "name": "Default",
          "slug": "default"
        },
        {
          "id": 2,
          "name": "Billing",
          "slug": "billing"
        },
        {
          "id": 3,
          "name": "Public",
          "slug": "public"
        },
        {
          "id": 4,
          "name": "Shipping",
          "slug": "shipping"
        }
      ]
    }
  ]
}

export default class Client extends Model {
  static entity = 'clients'

  static fields () {
    return {
      id: this.attr(null),
      name: this.string(null).nullable(),

      emailAddresses: this.morphMany(EmailAddress, 'mailableId', 'mailableType'),
      addresses: this.morphMany(Address, 'addressId', 'addressType'),
    }
  }
}

export default class Address extends Model {
  static entity = 'addresses'

  static fields () {
    return {
      id: this.attr(null),
      street: this.string(''),

      addressType: this.attr(null),
      addressId: this.attr(null),

      tags: this.morphToMany(Tag, Taggable, 'tagId', 'taggableId', 'taggableType'),

    }
  }
}

export default class EmailAddress extends Model {
  static entity = 'email_addresses'

  static fields () {
    return {
      id: this.attr(null),
      email: this.string(''),

      mailableType: this.attr(null),
      mailableId: this.attr(null),

      tags: this.morphToMany(Tag, Taggable, 'tagId', 'taggableId', 'taggableType'),

    }
  }
}

export default class Tag extends Model {
  static entity = 'tags'

  static fields () {
    return {
      id: this.uid(),
      name: this.string(''),
      slug: this.string(''),

      // Reversed Relations
      emailAddresses: this.morphedByMany(EmailAddress, Taggable, 'tagId', 'taggableId', 'taggableType'),
      addresses: this.morphedByMany(Address, Taggable, 'tagId', 'taggableId', 'taggableType')
    }
  }
}

export default class Taggable extends Model {
  static entity = 'taggables'

  static get primaryKey () {
    return ['taggableId', 'tagId', 'taggableType']
  }

  static fields () {
    return {
      id: this.uid(null),
      taggableType: this.string(''),
      taggableId: this.number(),
      tagId: this.number()
    }
  }
}
adm-bome commented 3 years ago

maybe not the best sollution, but for development and for now....:

I narrowed it down to the way keys are compared.

I made an addition... in the source file vuex-orm.esm.js at line 5090 of the compiled library. Maybe not the right place and not the best code but it works for me... maybe this is better in the method getIndexIdFromRecord and return the correct format.

The code below should be for the un-compiled lib

// file: query/Query.ts
export default class Query<T extends Model = Model> {
// ...
  private updateIndexes(instances: Data.Instances<T>): Data.Instances<T> {
    return Object.keys(instances).reduce<Data.Instances<T>>(
      (instances, key) => {
        const instance = instances[key]
        let id = String(this.model.getIndexIdFromRecord(instance))

       /** Added lines:
        * Transform id from stringified Array to snakeCase,  when key has snakeCase
        */
        const parsedId = JSON.parse(id)
        if (String(key).includes('_') && Array.isArray(parsedId)) {
          id = parsedId.join('_')
        }
        /** End **/

        if (key !== id) {
          instance.$id = id

          instances[id] = instance

          delete instances[key]
        }

        return instances
      },
      instances
    )
  }

// ...
}
adm-bome commented 3 years ago

Question: @cuebit @kiaking

Is there a reason pivotKey is build different ways in the Many relations?

file attributes/relations/BelongsToMany.ts see line 233 see

against: file attributes/relations/MorphToMany.ts see line 242 see file attributes/relations/MorphedByMany.ts see line 232 see

When I alter the MorphToMany and MorphedByMany relations to a JSON.stringify of an array like in the BelongsToMany relation, the compaired key are the same.

Model.getIndexIdFromRecord() and Query.normalizeIndexId() are array stringified and not snakeCase.

Possible solution:

    // file: MorphToMany.ts
      const pivotKey = JSON.stringify([
        parseInt(parentId),
        parseInt(id),
        parent.entity
      ])
   // file: MorphedByMany.ts
      const pivotKey = JSON.stringify([
        parseInt(id),
        parseInt(parentId),
        this.related.entity
      ])
steven-fox commented 2 years ago

I'm going to jot this down in case it helps any fellow Vuex-ORM users out there, but proceed with caution. Your mileage may vary on this "hack" of a solution (er, I can't even consider this a "solution" in good faith) and your peer will likely yell in agony as you include this code in a PR review. Alas, since it doesn't appear this problem will be fixed in the package, this is a 2 minute change that may be good enough for your use case.

In your model that represents the pivot table for a morph many relation (and I believe it'd work the same for a standard many2many pivot), you can tap into the beforeCreate(model) and beforeUpdate(model) lifecycle hooks to catch a "vuex-orm is about to insert/update a duplicated pivot record" situation and return false in that hook to abort the persist process.

Something like:

Note: it may be sufficient to run only 1 of the hooks, but haven't tested as of writing this. Update: it does indeed appear that only the beforeUpdate() hook is needed, but not 100% certain...

// Imagine a 'project', 'tags', and 'taggables' situation...

// In Taggable.js

export default class Taggable extends BaseModel {
  // ...

  static primaryKey = ['tag_id', 'taggable_id', 'taggable_type']

  /**
   * @param {Taggable} model
   * @returns {boolean|void}
   */
  static beforeCreate(model) {
    if (model.hasDuplicateRecord) {
      return false
    }
  }

  /**
   * @param {Taggable} model
   * @returns {boolean|void}
   */
  static beforeUpdate(model) {
    if (model.hasDuplicateRecord) {
      return false
    }
  }

  /**
   * @param {Taggable} model
   * @returns {?Taggable}
   */
  static findDuplicateRecordFor(model) {
    return (
      // The following commented search throws a "composite key is not an array" exception,
      //    which is a tad comical considering it's the id value in the store...
      // Taggable.query()
      //   .whereId(`${model.taggable_id}_${model.tag_id}_${model.taggable_type}`)
      //   .first() ??
      Taggable.query()
        .whereId([model.tag_id, model.taggable_id, model.taggable_type])
        .first() ??
      Taggable.query()
        .where('tag_id', model.tag_id)
        .where('taggable_id', model.taggable_id)
        .where('taggable_type', model.taggable_type)
        .first()
    )
  }

  /**
   * @returns {?Taggable}
   */
  get duplicateRecord() {
    return Taggable.findDuplicateRecordFor(this)
  }

  /**
   * @returns {boolean}
   */
  get hasDuplicateRecord() {
    return this.duplicateRecord !== null
  }
}

One could likely abstract this logic to share it across various pivot models, but this illustrates the idea.

Keep in mind, this may will lead to a significant performance hit if inserting/updating a bunch of records, as this is clearly an N+1 situation (that grows in time-to-search as the # of pivot records increases, especially since we can't do the string_composite_key id lookup).

Note: you can also approach this problem by only returning distinct pivot records in the beforeSelect(records) query hook, which is faster for inserts/updates but slower for selects. For example:

// Taggable.js

import { uniqWith } from 'lodash'

export default class Taggable extends BaseModel {
    ...

  static beforeSelect(taggables) {
    return uniqWith(
      taggables,
      (arrVal, otherVal) =>
        arrVal.tag_id === otherVal.tag_id &&
        arrVal.taggable_id === otherVal.taggable_id &&
        arrVal.taggable_type === otherVal.taggable_type
    )
  }

Apologies if you adopt this code and lose your job as a consequence. 🤣