Success! Looks like we've fixed this one. According to our records the fix was applied for TOUCH-3500 in a recent build.
  1. #1
    Ext JS Premium Member stever's Avatar
    Join Date
    Mar 2007
    Posts
    1,407
    Vote Rating
    6
    stever will become famous soon enough stever will become famous soon enough

      0  

    Default Store.load() ... Store.sync() <-- this tells the server to delete EVERYTHING

    Store.load() ... Store.sync() <-- this tells the server to delete EVERYTHING


    REQUIRED INFORMATION


    Ext version tested:
    • Sencha Touch 2.0.1

    Browser versions tested against:
    • Chrome
    • FF (firebug 1.9.1 installed)

    Description:
    • Load and then sync should be a no-op but instead tells server to erase all your data.

    Steps to reproduce the problem:
    • Go to the Kiva example
    • console.log(Ext.data.StoreManager.get("Loans").removed;
    • console.log(Ext.data.StoreManager.get("Loans").load();console.log(Ext.data.StoreManager.get("Loans").removed)
    • Ext.data.StoreManager.get("Loans").sync() // This will find all the items there are marked to be destroyed

    The result that was expected:
    • load() then sync() should not delete all your data

    The result that occurs instead:
    • data is deleted on server

    Test Case:

    Code:
        <<insert working code to reproduce the report >>


    HELPFUL INFORMATION


    See this URL for live test case: http://docs.sencha.com/touch/2-0/tou...iva/index.html


    Debugging already done:
    I have narrowed down a case where a store will tell the server to delete all the records. In Ext 4.0.7 this does not happen, but in ST2.0.1 is does.

    Store.filter()
    ...
    Store.load()
    ...
    Store.sync()
    ...
    all items GONE!!!

    Internally, it looks like load() leads to doDataRefresh() which does a removeAll() but first joins all the records to a temp store. This joining is not removing their association with the original store and they are marked to get removed, despite the comment to the contrary in the code. Thus when the next sync comes around... everything gets deleted.

    Possible fix:
    • not provided

    Additional CSS used:
    • only default ext-all.css
    • custom css (include details)

  2. #2
    Sencha User
    Join Date
    Mar 2007
    Location
    Haarlem, Netherlands
    Posts
    1,243
    Vote Rating
    10
    TommyMaintz will become famous soon enough TommyMaintz will become famous soon enough

      0  

    Default


    As a temporary workaround try setting syncRemovedRecords: false on your store. Obviously that's not a good solution but thought it might help you in the meantime.

  3. #3
    Ext JS Premium Member stever's Avatar
    Join Date
    Mar 2007
    Posts
    1,407
    Vote Rating
    6
    stever will become famous soon enough stever will become famous soon enough

      0  

    Default


    Quote Originally Posted by TommyMaintz View Post
    As a temporary workaround try setting syncRemovedRecords: false on your store. Obviously that's not a good solution but thought it might help you in the meantime.
    Yes... it won't accidentally delete everything then. But it won't delete anything I want to delete either.

  4. #4
    Ext JS Premium Member stever's Avatar
    Join Date
    Mar 2007
    Posts
    1,407
    Vote Rating
    6
    stever will become famous soon enough stever will become famous soon enough

      0  

    Default


    Here is one possible fix. I don't have confidence in either my fix or the Sencha code dealing with data elsewhere (what about tree stores?)
    Code:
        doDataRefresh: function(store, data, operation) {
            var records = operation.getRecords(),
                me = this,
                original_records = me.data.all,
                ln = original_records.length,
                i, record;
    
    
            if (operation.getAddRecords() !== true) {
                for (i = 0; i < ln; i++) {
                    record = original_records[i];
                    record.unjoin(me);
                }
                me.data.clear();
                // This means we have to fire a clear event though
                me.fireEvent('clear', this);
            }
    
            if (records && records.length) {
                // Now lets add the records without firing an addrecords event
                me.suspendEvents();
                me.add(records);
                me.resumeEvents();
            }
    
            this.fireEvent('refresh', this, this.data);
        },

  5. #5
    Sencha - Community Support Team edspencer's Avatar
    Join Date
    Jan 2009
    Location
    Palo Alto, California
    Posts
    1,939
    Vote Rating
    9
    edspencer is a jewel in the rough edspencer is a jewel in the rough edspencer is a jewel in the rough

      0  

    Default


    Hey Steve,

    Sorry we released such an egregious bug with 2.0.1, we're treating it as a top priority internally and hope to have a public fix out quickly. Stay tuned
    Ext JS Senior Software Architect
    Personal Blog: http://edspencer.net
    Twitter: http://twitter.com/edspencer
    Github: http://github.com/edspencer

  6. #6
    Sencha User
    Join Date
    Oct 2011
    Location
    Germany
    Posts
    146
    Vote Rating
    10
    Möhre will become famous soon enough

      0  

    Default


    Quality management has to get much better, now that Sencha has teamed up with SAP

  7. #7
    Ext JS Premium Member stever's Avatar
    Join Date
    Mar 2007
    Posts
    1,407
    Vote Rating
    6
    stever will become famous soon enough stever will become famous soon enough

      0  

    Default


    Quote Originally Posted by edspencer View Post
    Hey Steve,

    Sorry we released such an egregious bug with 2.0.1, we're treating it as a top priority internally and hope to have a public fix out quickly. Stay tuned
    The fix above works, though I guess I should have created an override for people. The original logic was overly complicated (and wrong). Didn't look like your style Ed. Considering all the testing you guys have, this shows a potential weak spot. Seeing data getting deleted from the database is less than ideal way to find it. :p Though Illuminations did show a list of removed records attached to the store before syncing which is how I tracked down the bug.

  8. #8
    Sencha User
    Join Date
    Mar 2007
    Location
    Haarlem, Netherlands
    Posts
    1,243
    Vote Rating
    10
    TommyMaintz will become famous soon enough TommyMaintz will become famous soon enough

      0  

    Default


    The reason that function became so "ugly" is because of the memory leaks we were having. Basically if you keep loading a store with new records, the old ones never get destroyed and thus stay in memory. removeAll has the logic to determine wether or not a record should be destroyed (is it still in joined to another store?). The problem then became that when you added records to the store that already existed in the store then they would be destroyed and then added again, which would obviously break. Thats why I had the records we are about to add to the store joined to a fake temp store before removing all the existing ones. That way the ones you were about to readd would not be destroyed.

    I agree the logic is ugly, and I'll clean it up. The fix you posted looks clean (and is actually exactly the same as what the code was in 2.0.0) but it reintroduces all the memory leaks again.

    I think it would be probably much cleaner to duplicate some of the removeAll functionality in the doDataRefresh method instead of trying to have that function do something it wasnt designed for. I'll provide an override shortly that hopefully fixes these issues once and for all and makes the code clean again :p.

    P.s. The unit tests testing this part of the Store API are not yet detailed enough to catch all these different variations, which is something else we'll make sure is done.

  9. #9
    Sencha User
    Join Date
    Mar 2007
    Location
    Haarlem, Netherlands
    Posts
    1,243
    Vote Rating
    10
    TommyMaintz will become famous soon enough TommyMaintz will become famous soon enough

      0  

    Default


    @stever

    Could you try the following override and see if it solves your case? I'm gonna write some more tests to cover all these cases but it would be useful for me to hear if it at least solved the issues you were having.

    Code:
    Ext.define('Ext.data.StoreRefreshFix', {
        override: 'Ext.data.Store',
    
        doDataRefresh: function(store, data, operation) {
            var records = operation.getRecords(),
                me = this,
                destroyRemovedRecords = me.getDestroyRemovedRecords(),
                syncRemovedRecords = me.getSyncRemovedRecords(),
                currentRecords = data.all.slice(),
                removed = me.removed,
                ln = currentRecords.length,
                i, record;
    
            if (operation.getAddRecords() !== true) {
                for (i = 0; i < ln; i++) {
                    record = currentRecords[i];
                    record.unjoin(me);
    
                    // If the record we are removing is not part of the records we are about to add to the store then handle
                    // the destroying or removing of the record to avoid memory leaks.
                    if (records.indexOf(record) === -1) {
                        if (syncRemovedRecords && record.phantom !== true) {
                            removed.push(record);
                        }
                        else if (destroyRemovedRecords && !syncRemovedRecords && !record.stores.length) {
                            record.destroy();
                        }
                    }
                }
                data.clear();
                // This means we have to fire a clear event though
                me.fireEvent('clear', me);
            }
    
            if (records && records.length) {
                // Now lets add the records without firing an addrecords event
                me.suspendEvents();
                me.add(records);
                me.resumeEvents();
            }
    
            me.fireEvent('refresh', me, data);
        }
    });
    Thanks,
    Tommy

  10. #10
    Ext JS Premium Member stever's Avatar
    Join Date
    Mar 2007
    Posts
    1,407
    Vote Rating
    6
    stever will become famous soon enough stever will become famous soon enough

      0  

    Default


    Quote Originally Posted by TommyMaintz View Post
    I think it would be probably much cleaner to duplicate some of the removeAll functionality in the doDataRefresh method instead of trying to have that function do something it wasnt designed for. I'll provide an override shortly that hopefully fixes these issues once and for all and makes the code clean again :p.
    Yes, I did notice that I wasn't using all the logic from removeAll, mostly because I was unfamiliar and wanted to err on the side of not destroying stuff. I did not notice that what I suggested was the same as v2.0.0. I don't like the temp store solution. What happens if an error gets thrown? And _temp doesn't seem like a very unique name either. I would go for copying code from removeAll and putting that in a comment (such that a grep for removeAll would also show something in doDataRefresh). Or you could factor it out, if it is worth it. But the current version is hard to read... leading me to a "what the ?#$% is going on here" moment.

    That said... I don't like how refresh is happening anyhow. Let's say I refresh the content with identical content. Why create new records at all? All the references to records will then point to the right ones, etc. (as I doubt people that make such references are using ES6 weak references). Maybe people shouldn't keep such references, but that is another issue. In fact, reusing records is a bigger issue altogether.

    Quote Originally Posted by TommyMaintz View Post
    P.s. The unit tests testing this part of the Store API are not yet detailed enough to catch all these different variations, which is something else we'll make sure is done.
    I'm surprised, as you guys have some really extensive unit tests!

    I suppose Sencha could use some more fleshed out samples to be used for integration testing. Maybe release that TouchForum app under MIT and I can help out.