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

Implement dirty tracking and rollback of belongsTo relationships. #3698

Closed

Conversation

arenoir
Copy link
Contributor

@arenoir arenoir commented Aug 25, 2015

Issues #3696, #3045

@fivetanley
Copy link
Member

This looks good to me. Would prefer for @wecc or @igorT to review though. Great work!

@fivetanley fivetanley assigned wecc and igorT and unassigned wecc Aug 26, 2015
@arenoir arenoir force-pushed the belongs-to-relationship-improvements branch 2 times, most recently from b6612ce to aa04fcf Compare September 16, 2015 01:21
@svox1
Copy link
Contributor

svox1 commented Sep 22, 2015

Any news on this? Would be great if releationships can rollback again.

@arenoir arenoir force-pushed the belongs-to-relationship-improvements branch from aa04fcf to febb27b Compare October 15, 2015 22:51
@ptgamr
Copy link

ptgamr commented Dec 14, 2015

+1

@fivetanley
Copy link
Member

@arenoir sorry for the delay, I'll make sure it gets discussed at this week's meeting.

@fivetanley
Copy link
Member

@igorT has agreed to write an RFC for relationship tracking.

@eshtadc
Copy link
Contributor

eshtadc commented Feb 12, 2016

+1 on this issue

@arenoir arenoir force-pushed the belongs-to-relationship-improvements branch 2 times, most recently from 03fb8b1 to 75e9f62 Compare February 23, 2016 23:30
@arenoir
Copy link
Contributor Author

arenoir commented Feb 23, 2016

@fivetanley any word on this? fyi I rebased with master.

@nicksteffens
Copy link

👍

@Keeo
Copy link

Keeo commented Mar 6, 2016

Please 👍

@thedrew12
Copy link

+1

@fivetanley
Copy link
Member

@arenoir I pinged @igorT about this yesterday. He said he should be able to write an RFC for an alternative approach. Hopefully he can fill in here more when he has a chance...

@fivetanley
Copy link
Member

I apologize for the long delays

@arenoir
Copy link
Contributor Author

arenoir commented Mar 8, 2016

No worries @fivetanley, I look forward to the RFC. Perhaps I am beating a dead horse but @igorT would you take a look at the conversation I started regarding the difference between hasOne and belongsTo. I think it relates to the RFC you may be putting together.

@aaronbhansen
Copy link

👍 this would be useful

@fivetanley
Copy link
Member

We all agree it would be useful. ;)

@nanuxbe
Copy link

nanuxbe commented Mar 19, 2016

👍 I've been implementing an re-implementing this for ages every time ember-data changes. It would definitely be useful.

@bmac
Copy link
Member

bmac commented Mar 27, 2016

I'm going to close this pr. @igorT is working on an RFC to describe what needs to happen to for Ember Data to support relationship rollback for both hasMany and belongsTo.

@bmac bmac closed this Mar 27, 2016
@epyx25
Copy link

epyx25 commented Apr 8, 2016

I've been implementing an re-implementing this for ages every time ember-data changes.

@nanuxbe could you share to us in a gist?

@xomaczar
Copy link
Contributor

Any news on the before mentioned RFC?

@abhilashlr
Copy link

👍 Any news on the RFC? Would love to see this rollback for belongsTo resolved.

@erkie
Copy link

erkie commented Jan 11, 2017

How can we help get this issue fixed? What's the blocker?

@malatin3
Copy link

This is a huge pain point with ember data for us. Would love to see this merged.

@XaserAcheron
Copy link
Contributor

At risk of being "that guy": I'm struggling through this at present, and it feels a lot like I drove a car into a parking space only to find out the next day that the car can't reverse.

I get that this is a hard feature to add, but I'm un-thrilled about the radio silence as of late.

@erkie
Copy link

erkie commented May 10, 2017

One solution for certain cases is to use ember-data-model-fragments.

I found that to implement arrays of plain objects or primitive types I was modeling them as ember-data relationships, when I didn't really want or need the overhead of the ember-data object.

@broerse
Copy link

broerse commented Aug 20, 2017

Should this be called isDirty ? Perhaps more logical:

hasDirtyFields: Ember.computed.or('hasDirtyAttributes', 'hasDirtyRelationships')

@workmanw
Copy link

This was marked "Needs Team Discussion" nearly two years ago. Was there a discussion on this? Is there anything that can be shared from the core team about this? As a long time ember user, it's just down right frustrating to see PRs with a lot of support sit without any feedback.

mmpestorich added a commit to swarmbox/data that referenced this pull request Feb 4, 2018
This commit:

1. Allows one to rollback belongsTo and hasMany relationships.
2. Reintroduces dirtyRecordFor*Change hooks on the adapter that allow
   one to customize when a record becomes dirty.
3. Added 'removeDeletedFromRelationshipsPriorToSave' flag to Adapter
   that allows one to opt back into the old deleted record from many
   array behavior (pre emberjs#3539).

Known issues:

1. Rolling back a hasMany relationship from the parent side of the
   relationship does not work (doing the same from the child side works
   fine). See test that is commented out below as well as the discussion
   at the end of emberjs#2881#issuecomment-204634262

   This was previously emberjs#2881 and is related to emberjs#3698
mmpestorich added a commit to swarmbox/data that referenced this pull request Feb 4, 2018
This commit:

1. Allows one to rollback belongsTo and hasMany relationships.
2. Reintroduces dirtyRecordFor*Change hooks on the adapter that allow
   one to customize when a record becomes dirty.
3. Added 'removeDeletedFromRelationshipsPriorToSave' flag to Adapter
   that allows one to opt back into the old deleted record from many
   array behavior (pre emberjs#3539).

Known issues:

1. Rolling back a hasMany relationship from the parent side of the
   relationship does not work (doing the same from the child side works
   fine). See test that is commented out below as well as the discussion
   at the end of emberjs#2881#issuecomment-204634262

   This was previously emberjs#2881 and is related to emberjs#3698
mmpestorich added a commit to swarmbox/data that referenced this pull request Feb 4, 2018
This commit:

1. Allows one to rollback belongsTo and hasMany relationships.
2. Reintroduces dirtyRecordFor*Change hooks on the adapter that allow
   one to customize when a record becomes dirty.
3. Added 'removeDeletedFromRelationshipsPriorToSave' flag to Adapter
   that allows one to opt back into the old deleted record from many
   array behavior (pre emberjs#3539).

Known issues:

1. Rolling back a hasMany relationship from the parent side of the
   relationship does not work (doing the same from the child side works
   fine). See test that is commented out below as well as the discussion
   at the end of emberjs#2881#issuecomment-204634262

   This was previously emberjs#2881 and is related to emberjs#3698
mmpestorich added a commit to swarmbox/data that referenced this pull request Feb 4, 2018
This commit:

1. Allows one to rollback belongsTo and hasMany relationships.
2. Reintroduces dirtyRecordFor*Change hooks on the adapter that allow
   one to customize when a record becomes dirty.
3. Added 'removeDeletedFromRelationshipsPriorToSave' flag to Adapter
   that allows one to opt back into the old deleted record from many
   array behavior (pre emberjs#3539).

Known issues:

1. Rolling back a hasMany relationship from the parent side of the
   relationship does not work (doing the same from the child side works
   fine). See test that is commented out below as well as the discussion
   at the end of emberjs#2881#issuecomment-204634262

   This was previously emberjs#2881 and is related to emberjs#3698
mmpestorich added a commit to swarmbox/data that referenced this pull request Feb 4, 2018
This commit:

1. Allows one to rollback belongsTo and hasMany relationships.
2. Reintroduces dirtyRecordFor*Change hooks on the adapter that allow
   one to customize when a record becomes dirty.
3. Added 'removeDeletedFromRelationshipsPriorToSave' flag to Adapter
   that allows one to opt back into the old deleted record from many
   array behavior (pre emberjs#3539).

Known issues:

1. Rolling back a hasMany relationship from the parent side of the
   relationship does not work (doing the same from the child side works
   fine). See test that is commented out below as well as the discussion
   at the end of emberjs#2881#issuecomment-204634262

   This was previously emberjs#2881 and is related to emberjs#3698
mmpestorich added a commit to swarmbox/data that referenced this pull request Feb 4, 2018
This commit:

1. Allows one to rollback belongsTo and hasMany relationships.
2. Reintroduces dirtyRecordFor*Change hooks on the adapter that allow
   one to customize when a record becomes dirty.
3. Added 'removeDeletedFromRelationshipsPriorToSave' flag to Adapter
   that allows one to opt back into the old deleted record from many
   array behavior (pre emberjs#3539).

Known issues:

1. Rolling back a hasMany relationship from the parent side of the
   relationship does not work (doing the same from the child side works
   fine). See test that is commented out below as well as the discussion
   at the end of emberjs#2881#issuecomment-204634262

   This was previously emberjs#2881 and is related to emberjs#3698
@Ramblurr
Copy link

I just watched a coworker spend over an hour working to debug an issue that turned out to be caused by inadvertently checking hasDirtyAttributes when the user changed a belongsTo value.

This exactly describes my afternoon. Incredibly frustrating!

@runspired
Copy link
Contributor

runspired commented Apr 22, 2018

@workmanw the short answer is that there have been many tried and failed attempts at resolving this by the core team. Discussions, spiked implementations, concepts. Ultimately it comes down to this being one of the hardest concepts to solve generally because there is simply so much variation in API design (even tying to json-api spec doesn't solve this issue for the most part).

This was one of the first things I looked to "fix" when I began working on ember-data two years ago. It took me a few weeks to understand the full scope of the issue and quite obviously nothing ever came of that. It's a problem that seems simple at first and quickly becomes the most complex issue any ORM can possibly face.

The above said, over the past few years we've identified a few areas that if-improved would improve our ability to potentially, maybe, eventually rollback relationships.

There's also a few ideas that still have merits but don't solve the problem the way they first appear to:

  • single source of truth for relationships

And some ideas that just won't work due to API complexity

  • in-flight relationship data

You'll maybe note that in-flight is in the "just wont work" category here. Ultimately this is the core reason why rollback has never successfully landed. It's also a much harder problem to solve for many relationships than it is for belongsTo, and the solution here does not address that, leaving many pitfalls and bugs for users to still encounter. In the general case, here's a simple fail scenario for 1:1 relationships to illustrate the difficulty of rollback during a save:

  • A 1:1 B
  • A sets B
  • B saves (which in this API does save the new relationship)
  • A rollback of B before B save completes
  • B resolves with no new relationship info (APIs in the wild often will not)
  • A:B linkage does not exist though it should

Ultimately, the store is incapable of storing in-flight state for relationships. However, adapters are well suited for this, and if we improve usage patterns in the request layer and improve our ability to correctly respond to changes in canonical state we will unlock the ability for users to implement relationship rollback locally as well.

Bear in mind, all of this discussion is only for the viewpoint that rollback and dirty tracking of relationships is for the linkage, not the state of the related records themselves. That's a very different concept, and not one that rollback should cover on the record level.

mmpestorich added a commit to swarmbox/data that referenced this pull request Aug 8, 2018
This commit:

1. Allows one to rollback belongsTo and hasMany relationships.
2. Reintroduces dirtyRecordFor*Change hooks on the adapter that allow
   one to customize when a record becomes dirty.
3. Added 'removeDeletedFromRelationshipsPriorToSave' flag to Adapter
   that allows one to opt back into the old deleted record from many
   array behavior (pre emberjs#3539).

Known issues:

1. Rolling back a hasMany relationship from the parent side of the
   relationship does not work (doing the same from the child side works
   fine). See test that is commented out below as well as the discussion
   at the end of emberjs#2881#issuecomment-204634262

   This was previously emberjs#2881 and is related to emberjs#3698
mmpestorich added a commit to swarmbox/data that referenced this pull request Aug 8, 2018
This commit:

1. Allows one to rollback belongsTo and hasMany relationships.
2. Reintroduces dirtyRecordFor*Change hooks on the adapter that allow
   one to customize when a record becomes dirty.
3. Added 'removeDeletedFromRelationshipsPriorToSave' flag to Adapter
   that allows one to opt back into the old deleted record from many
   array behavior (pre emberjs#3539).

Known issues:

1. Rolling back a hasMany relationship from the parent side of the
   relationship does not work (doing the same from the child side works
   fine). See test that is commented out below as well as the discussion
   at the end of emberjs#2881#issuecomment-204634262

   This was previously emberjs#2881 and is related to emberjs#3698
@runspired
Copy link
Contributor

Closing this in favor of addons like ember-data-changetracker and use of a RecordData based approach. That said improved relationship primitives are in the works for RecordData authors to make use of, and would include tracked changes for easier rollback and diffing.

@runspired runspired closed this Aug 24, 2019
mmpestorich added a commit to swarmbox/data that referenced this pull request Oct 24, 2019
This commit:

1. Allows one to rollback belongsTo and hasMany relationships.
2. Reintroduces dirtyRecordFor*Change hooks on the adapter that allow
   one to customize when a record becomes dirty.
3. Added 'removeDeletedFromRelationshipsPriorToSave' flag to Adapter
   that allows one to opt back into the old deleted record from many
   array behavior (pre emberjs#3539).

Known issues:

1. Rolling back a hasMany relationship from the parent side of the
   relationship does not work (doing the same from the child side works
   fine). See test that is commented out below as well as the discussion
   at the end of emberjs#2881#issuecomment-204634262

   This was previously emberjs#2881 and is related to emberjs#3698
mmpestorich added a commit to swarmbox/data that referenced this pull request Sep 18, 2020
This commit:

1. Allows one to rollback belongsTo and hasMany relationships.
2. Reintroduces dirtyRecordFor*Change hooks on the adapter that allow
   one to customize when a record becomes dirty.
3. Added 'removeDeletedFromRelationshipsPriorToSave' flag to Adapter
   that allows one to opt back into the old deleted record from many
   array behavior (pre emberjs#3539).
4. Adds bin/build.js to build a standalone version of ember-data.

Known issues:

1. Rolling back a hasMany relationship from the parent side of the
   relationship does not work (doing the same from the child side works
   fine). See test that is commented out below as well as the discussion
   at the end of emberjs#2881#issuecomment-204634262

   This was previously emberjs#2881 and is related to emberjs#3698
mmpestorich added a commit to swarmbox/data that referenced this pull request Jan 1, 2021
This commit:

1. Allows one to rollback belongsTo and hasMany relationships.
2. Reintroduces dirtyRecordFor*Change hooks on the adapter that allow
   one to customize when a record becomes dirty.
3. Added 'removeDeletedFromRelationshipsPriorToSave' flag to Adapter
   that allows one to opt back into the old deleted record from many
   array behavior (pre emberjs#3539).
4. Adds bin/build.js to build a standalone version of ember-data.

Known issues:

1. Rolling back a hasMany relationship from the parent side of the
   relationship does not work (doing the same from the child side works
   fine). See test that is commented out below as well as the discussion
   at the end of emberjs#2881#issuecomment-204634262

   This was previously emberjs#2881 and is related to emberjs#3698
mmpestorich added a commit to swarmbox/data that referenced this pull request Oct 31, 2022
This commit:

1. Allows one to rollback belongsTo and hasMany relationships.
2. Added 'shouldRemoveDeletedFromRelationshipsPriorToSave' flag
   to Adapter that allows one to opt back into the old deleted
   record from many array behavior (pre emberjs#3539).
3. Adds bin/build.js to build a standalone version of ember-data.

Known issues:

1. Rolling back a hasMany relationship from the parent side of that
   relationship does not work (doing the same from the child side works
   fine). See test that is commented out below as well as the discussion
   at the end of emberjs#2881#issuecomment-204634262

This was previously emberjs#2881 and is related to emberjs#3698
mmpestorich added a commit to swarmbox/data that referenced this pull request Oct 31, 2022
This commit:

1. Allows one to rollback belongsTo and hasMany relationships.
2. Added 'shouldRemoveDeletedFromRelationshipsPriorToSave' flag
   to Adapter that allows one to opt back into the old deleted
   record from many array behavior (pre emberjs#3539).
3. Adds bin/build.js to build a standalone version of ember-data.

Known issues:

1. Rolling back a hasMany relationship from the parent side of that
   relationship does not work (doing the same from the child side works
   fine). See test that is commented out below as well as the discussion
   at the end of emberjs#2881#issuecomment-204634262

This was previously emberjs#2881 and is related to emberjs#3698
jghansell pushed a commit to swarmbox/data that referenced this pull request Apr 13, 2023
This commit:

1. Allows one to rollback belongsTo and hasMany relationships.
2. Added 'shouldRemoveDeletedFromRelationshipsPriorToSave' flag
   to Adapter that allows one to opt back into the old deleted
   record from many array behavior (pre emberjs#3539).
3. Adds bin/build.js to build a standalone version of ember-data.

Known issues:

1. Rolling back a hasMany relationship from the parent side of that
   relationship does not work (doing the same from the child side works
   fine). See test that is commented out below as well as the discussion
   at the end of emberjs#2881#issuecomment-204634262

This was previously emberjs#2881 and is related to emberjs#3698
mmpestorich added a commit to swarmbox/data that referenced this pull request Apr 14, 2023
This commit:

1. Allows one to rollback belongsTo and hasMany relationships.
2. Added 'shouldRemoveDeletedFromRelationshipsPriorToSave' flag
   to Adapter that allows one to opt back into the old deleted
   record from many array behavior (pre emberjs#3539).
3. Adds bin/build.js to build a standalone version of ember-data.

Known issues:

1. Rolling back a hasMany relationship from the parent side of that
   relationship does not work (doing the same from the child side works
   fine). See test that is commented out below as well as the discussion
   at the end of emberjs#2881#issuecomment-204634262

This was previously emberjs#2881 and is related to emberjs#3698
jghansell pushed a commit to swarmbox/data that referenced this pull request Jun 13, 2024
This commit:

1. Allows one to rollback belongsTo and hasMany relationships.
2. Added 'shouldRemoveDeletedFromRelationshipsPriorToSave' flag
   to Adapter that allows one to opt back into the old deleted
   record from many array behavior (pre emberjs#3539).
3. Adds bin/build.js to build a standalone version of ember-data.

Known issues:

1. Rolling back a hasMany relationship from the parent side of that
   relationship does not work (doing the same from the child side works
   fine). See test that is commented out below as well as the discussion
   at the end of emberjs#2881#issuecomment-204634262

This was previously emberjs#2881 and is related to emberjs#3698
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.