Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

A 'deleteObject' from a hasMany relation with an object not included in the collection, deletes another object within the collection #8318

Closed
eodb opened this issue Nov 24, 2022 · 4 comments
Labels
good-first-issue 🏷️ bug This PR primarily fixes a reported issue

Comments

@eodb
Copy link

eodb commented Nov 24, 2022

Reproduction

Following straight forward qunit test can be copied to quickly reproduce the issue:

import { run } from '@ember/runloop';
import Model, { attr, belongsTo, hasMany } from '@ember-data/model';
import { setupTest } from 'ember-qunit';
import { module, test } from 'qunit';

let store;

module('integration/relationship/deleteObject from hasMany relation', function (hooks) {
  setupTest(hooks);

  hooks.beforeEach(function () {
    class Job extends Model {
      @attr name;
      @hasMany('message', { async: true, inverse: 'job' }) messages;
    }

    class Message extends Model {
      @attr msg;
      @belongsTo('job', { async: true, inverse: 'messages' }) job;
    }

    store = this.owner.lookup('service:store');
    this.owner.register('model:job', Job);
    this.owner.register('model:message', Message);
  });

  test('deleteObject of hasMany relation', async function (assert) {
    let job;
    let msg1;
    let msg2;
    let msg3;

    run(() => {
      job = store.createRecord('job', { name: 'my job' });
    });
    assert.ok(job, 'job created');

    run(() => {
      msg1 = store.createRecord('message', { job: job, msg: '1st msg' });
      msg2 = store.createRecord('message', { job: job, msg: '2nd msg' });
      msg3 = store.createRecord('message', { msg: '3rd msg' }); // 3rd msg, not part of the job
    });

    await run(() => {
      return job.messages.then((msgs) => {
        assert.equal(msgs.length, 2, 'job has 2 messages');
        assert.ok(msgs.includes(msg1), 'msg1 included');
        assert.ok(msgs.includes(msg2), 'msg2 included');
        // try to delete an object that is not included in the collection
        assert.notOk(msgs.includes(msg3), 'msg3 is not included');
        msgs.removeObject(msg3);
      });
    });

    run(() => {
      assert.equal(
        job.messages.length,
        2,
        'job still has 2 messages after an attempt to delete an object that is not part of the collection'
      );
    });
  });
});

Description

One expects that an attempt to delete an object from a hasMany relationship that is not included in the collection, should have no effect. Instead some other object from the collection seems to be deleted (see last failing assert: the resulting collection only contains one element anymore instead of the original two elements).

I tested this with an AsyncHasMany relation (async=true), but it also fails when the relation is defined as synchronous (async=false).

Versions

This scenario did work in ember-data 4.4.0 but not anymore with 4.8.3.
Tests were done with:

DEBUG: Ember      : 4.8.2
DEBUG: Ember Data : 4.8.3
@runspired
Copy link
Contributor

Probably the shim to deprecate removeObject allows negative indexes as native splice allows negative. Should be an easy guard to add here:

objs.forEach((obj) => this.splice(this.indexOf(obj), 1));

@runspired
Copy link
Contributor

@jrjohnson this could be an easy one for you <3

@rossketron
Copy link
Contributor

Is this issue currently being worked on by anyone? If not, I'd like to work on it!

@jrjohnson
Copy link
Contributor

Go for it @rossketron!

rossketron added a commit to rossketron/ember-data that referenced this issue Nov 28, 2022
runspired pushed a commit to rossketron/ember-data that referenced this issue Dec 7, 2022
runspired added a commit that referenced this issue Dec 7, 2022
…8323)

* fix: Only remove from record array if in collection #8318

* test: Update tests to use hasMany relationship and add deprecatedTests

* chore: Seperate tests for sync/async

* update tests post-review

Co-authored-by: Chris Thoburn <[email protected]>
@runspired runspired added 🏷️ bug This PR primarily fixes a reported issue and removed Bug labels Sep 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good-first-issue 🏷️ bug This PR primarily fixes a reported issue
Projects
None yet
Development

No branches or pull requests

4 participants