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.

Sign up for GitHub

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

Jump to bottom

Accessing Nested Array Without .get() Adds Empty Object to Change Object #624

Open
kdagnan opened this issue Nov 10, 2021 · 6 comments
Open

Accessing Nested Array Without .get() Adds Empty Object to Change Object #624

kdagnan opened this issue Nov 10, 2021 · 6 comments

Comments

@kdagnan
Copy link

kdagnan commented Nov 10, 2021 •
edited
Loading

Version

3.13.6

Steps to reproduce

  1. Create a Changeset (from Ember-Data object in my case) with a nested array:
    { name: "Example, address: { residentIds: ["abc123"] } }
  2. Access the nested array in Javascript :
    const residentIds = changeset.address.residentIds
  3. Edit any other (non-object) attribute value

Expected Behavior

The change {} object and the changes [] array on the changeset should be uneffected:
changeset: { changes: [], change: {} }

Actual Behavior

The change {} object on the changeset has an empty value for the high-level key ("address" in above example), and changes [] array does not reflect it:
changeset: { changes: [], change: { address: {} } }

If you use .get() syntax, this behavior does not occur: const residentIds = changeset.get('address.residentIds')
@snewcomer
Copy link
Collaborator

Thanks Kyle for the detailed issue! Is there a test you can in this project and reproduces this issue?

@kdagnan
Copy link
Author

kdagnan commented Nov 10, 2021 •
edited
Loading

Thanks Kyle for the detailed issue! Is there a test you can in this project and reproduces this issue?

https://github.com/kdagnan/Ember-Changeset-Issue-624

Was also able to confirm a few more things:

  1. This occurs when accessing ANY nested value on a high-level object on the ember-data model (not just arrays as originally indicated)
  2. This occurs whether the object is an @attr on the ember-data model, or a result of a get()
  3. This occurs when accessing the nested values in both handlebars & Javascript
  4. For this to happen you MUST also edit ANY other attribute on the model first.

All of these are demonstrated in the linked project. You may want to run it & comment things out to see everything I'm talking about.

@jelhan
Copy link
Contributor

jelhan commented Nov 28, 2021

I'm facing the same bug. For some reason I missed this GitHub issue when scanning over reported bugs. I came up with another reproduction, which shows the same: https://github.com/jelhan/reproduce-invalid-change-in-ember-changeset-if-used-together-with-ember-bootstrap-power-select

To recreate the reproduction yourself, you can follow these steps:

  1. Install Ember Changeset v3.15.0, which is the latest version currently.
  2. Create an index route.
  3. Create the following controller for index route: 
    import Controller from '@ember/controller';
import { action } from '@ember/object';

export default class IndexController extends Controller {
  authors = [
    { name: 'Jane Doe' },
    { name: 'John Doe' },
    { name: 'Max Mustermann' },
    { name: 'Erika Mustermann' },
  ];
  post = {
    title: 'My post',
    author: this.authors[2],
  };
  
  @action
  handleSubmit(changeset, event) {
    event.preventDefault();
    
    console.log(changeset.get('change'), changeset.get('changes'));
  }
}
  4. Add the following template in index route: 
    {{#let (changeset this.post) as |changeset|}}
  <form
    {{on 'submit' (fn this.handleSubmit changeset)}}
  >
    <div>
      <label for='title-input'>Title</label>
      <Input
        @value={{changeset.title}}
        id='title-input'
      />
    </div>
    <p>
      Author: {{changeset.author.name}}
    </p>
    <button type='submit'>
      submit
    </button>
  </form>
{{/let}}
  5. Start development server, open http://localhost:4200 in a browser of your choice.
  6. Change the title input to foo.
  7. Hit the submit button.
  8. Inspect the change and changes properties of the changeset, which have been logged to console.

Expected

  • The change object should have only one key, which is title.
  • The change object should deep equal { title: 'foo' }.
  • The changes array should only have one item, which deep equals: { key: 'title', value: 'foo' }.

Actual

  • The change object has two keys: author and title.
  • The change object deep equals { author: {}, title: 'foo' }.
  • The changes property is as expected.

P.S.: Please ignore the name of the reproduction repository. I first thought that it might be related to Ember Bootstrap or Ember Power Select.

@jelhan
Copy link
Contributor

jelhan commented Nov 28, 2021

I debugged a little bit further. Let me share my findings. Hopefully they are helpful for other to debug further. All findings use my reproduction given above.

  1. this[CHANGES] contains author key regardless if there has been any change. The object looks like this from the very beginning: { author: {}}. I haven't found out yet when that author key is set.
  2. There must be another change to trigger the bug because of this if guard: https://github.com/validated-changeset/validated-changeset/blob/6c1346ed111b63dfc2f3e5bf9d526951e2add79a/src/index.ts#L233-L235 hasChanges() returns false even if this[CHANGES] deep equals { author: {}}. So without any other changes it uses this branch: return {}.
  3. The normalizeObject function used by change getter and the getKeyValues function treat key-value pairs for which isChange(value) is false differently. 
    JSON.stringify(this[CHANGES]);
// '{"author":{},"title":{}}'
JSON.stringify(getKeyValues(this[CHANGES]));
// '[{"key":"title","value":"My post a"}]'
JSON.stringify(normalizeObject(this[CHANGES]));
// '{"author":{},"title":"My post a"}'
    In both cases they are calling themselves recursively if value is an object and isChange(value) is false. https://github.com/validated-changeset/validated-changeset/blob/6c1346ed111b63dfc2f3e5bf9d526951e2add79a/src/utils/get-key-values.ts#L20-L24 https://github.com/validated-changeset/validated-changeset/blob/6c1346ed111b63dfc2f3e5bf9d526951e2add79a/src/utils/normalize-object.ts#L42-L51This is the case in our scenario: 
    isObject(this[CHANGES].author);
// true
isChange(this[CHANGES].author);
// true
    The difference is how they handle the empty object this[CHANGES].author. getKeyValues returns an empty array, while normalizeObject returns an empty object: 
    isChange(this[CHANGES].author);
// false
isObject(this[CHANGES].author);
// true
getKeyValues(this[CHANGES].author);
// []
normalizeObject(this[CHANGES].author);
// {}
normalizeObject(this[CHANGES].author) === this[CHANGES].author;
// false

Summary

change getter includes key-value pairs isChange(value) is false for them and all their nested objects. I guess that shouldn't be the case.

I don't have enough insights if this should be changed in normalizeObject utility function or if change getter should filter it out itself. The normalizeObject utility function is used at many places. Not only in the change getter.

The bug seems to be present in validated-changeset library, which is used by ember-changeset. It seems to be the same bug as already reported in validated-changeset repository in February: adopted-ember-addons/validated-changeset#102

@kdagnan
Copy link
Author

kdagnan commented Nov 28, 2021 via email

@jelhan
Copy link
Contributor

jelhan commented Nov 29, 2021

Since submitting this issue, a co-worker has pointed out to me this section of the README: https://github.com/poteto/ember-changeset#changeset-template-helpers I wonder if this is the reason and/or related to why they say to use {{changeset-get}}. Similar result/issue occurs if accessing directly in JavaScript (ie. Changeset.author.name), rather than using Changeset.get(“author.name”).

I can confirm that I don't see the issue anymore if applying this small change to my reproduction:

diff --git a/app/templates/index.hbs b/app/templates/index.hbs
index 39c4ed1..2e99109 100644
--- a/app/templates/index.hbs
+++ b/app/templates/index.hbs
@@ -10,7 +10,7 @@
       />
     </div>
     <p>
-      Author: {{changeset.author.name}}
+      Author: {{changeset-get changeset "author.name"}}
     </p>
     <button type='submit'>
       submit

I would still consider this a bug as change and changes getter should return a consistent value. And they don't do in my reproduction. Further changes getter returns what I would expect - even if not using {{changeset-get}} helper.

Enforcing to use {{changeset-get}} helper seems a very problematic limitation in perspective. I interact with the changeset through different libraries. In my case it's a combination of Ember Bootstrap and Ember Power Select. I can not alter them to use {{changeset-get}}.

That limitation does not seem to be necessary in my case as changes getter returns the expected value.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@jelhan @snewcomer @kdagnan