Opened 12 years ago

Closed 12 years ago

#4552 closed enhancement (fixed)

ItemFileWriteStore: automatically remove references to item when deleting the item

Reported by: guest Owned by: Jared Jurkiewicz
Priority: high Milestone: 1.1
Component: Data Version: 0.9
Keywords: dijit tree store Cc: bill, alex, Dustin Machi
Blocked By: Blocking:

Description

After deleting an item from the store, I was using the ItemFileWriteStore? the dijit.Tree handles the onDelete correctly and will delete any node that is visible, but the node is not removed from being a child of the node in the store. This causes problems if something is deleted from the store before the node is visible. When dijit tries to render the children, then it errors out because one of the children has been deleted and is therefore missing.

Any refrences in the store to anything that is deleted need to also be deleted in the event of a delete.

Attachments (6)

Tree.patch (949 bytes) - added by guest 12 years ago.
dojo.data_ItemFileStores_RefIntegrity_20071218.patch (31.9 KB) - added by Jared Jurkiewicz 12 years ago.
Patch to include efficent reference integrity code into the ItemFile?*Stores
demo_ReferenceIntegrityTree.html (2.6 KB) - added by Jared Jurkiewicz 12 years ago.
Demo with tree. Just drop this as a peer file to dojo/, dijit/, dojox/
dojo.data.ItemFileStore_refIntegrity_20080103.patch (34.3 KB) - added by Jared Jurkiewicz 12 years ago.
Updated patch. Condensed a few sections, moved code from Read into Write (made read a stub). Fixed a UT problem, added a new test, and fixed a problem with child items (not specifically _referenced items)
dojo.data.ItemFileStore_refIntegrity_alternate_20080103.patch (32.2 KB) - added by Jared Jurkiewicz 12 years ago.
Alternate ref integrity approach (ref information is attached to each item instead of a global map).
dojo.data.ItemFileStore_refIntegrity_20080107.patch (37.8 KB) - added by Jared Jurkiewicz 12 years ago.
Updated reference integrity patch with optimizations suggested by Bill Keese + a couple more corner case fixes.

Download all attachments as: .zip

Change History (27)

comment:1 Changed 12 years ago by bill

I didn't understand your description. Sounds like you are talking about a problem with dojo.data.ItemFileWriteStore? Please attach a testcase *using the "Attach File" button*. And also, in your description above do "node" and "item" mean the same thing?

comment:2 Changed 12 years ago by bill

Component: DijitData
Owner: set to Jared Jurkiewicz

Hmm if anything this is a dojo.data bug AFAICT. Jared, can you reproduce a problem where deleting a node doesn't delete references to that node? (And also, what's our whole story on that? Is it supposed to delete references or is that something the user has to do manually?)

comment:3 Changed 12 years ago by Jared Jurkiewicz

The ItemFileRead/Write? store doesn't do referential integrity checking. So, you would have had to remove the references before deleting it. Could it be done? Maybe. Concerns on how efficient it would be to do so, though.

comment:4 Changed 12 years ago by bill

severity: criticalnormal
Summary: Dijit.Tree removeNodeItemFileWriteStore: automatically remove references to item when deleting the item
Type: defectenhancement

OK, not sure what to say here. I guess it's "expected behavior"; if you delete an item you have to manually delete references to it, or otherwise the datastore becomes inconsistent which then messes up Tree. I'm changing the bug summary to be more descriptive, and changing type to an enhancement. I'll leave it to Jared to schedule if/when this behavior should be changed. (Since there is a workaround to this "bug" I don't think it needs to be fixed for 1.0, but maybe it's worth a discussion in the dojo.data meeting.

comment:5 Changed 12 years ago by Jared Jurkiewicz

I agree its worth discussion. there are two approaches for dealing with it. One is like O(m*n*x), the other would be O(n). But the O(n) would require significant changes to parts of the store, which I'm leery of doing unless I have significant time to retest it.

comment:6 Changed 12 years ago by Jared Jurkiewicz

Milestone: 1.01.1

comment:7 Changed 12 years ago by Jared Jurkiewicz

Since people have not been attending the meetings to discuss this issue, this is being tabled out to 1.1.

comment:8 Changed 12 years ago by Dustin Machi

I hope its not me that anyone has been waiting on to come to the meeting to discuss this. I likely will never be able to attend this meeting as I can't get away on tuesdays and thursdays at this time of day. Anyway, I'll put my two cents in here. I think this is very very important (and that isn't to say that you are the person that needs to implement it Jared). I also understand that it is difficult to implement in a performant fashion for some stores. IMHO, it seems like something that should be added to the spec. Perhaps there should be a method for adding references to an item so that the store can keep track of references for easy deltion, or this could potentially be done by analyzing data that gets added as a property to another item to see if it isItem() (though this seems like it could potentially be slow in some cases). Anyway, I guess if we aren't going to fix this for 1.0, we should probably put an experimental note in the dojo.notification of widgets like tree as they basically don't work correctly right now until we come up with a solution for this. They will work in fairly simple cases, but get messed up easily afterwards. I guess also technically if we decide to add this to the api, then we should either do so before 1.0 (even if it isn't implemented..perhaps as an additional feature or something?) since the api is supposed to be locked for that. Of course, this is an addition so maybe people dont' care as much and we can be a little less strict.

comment:9 Changed 12 years ago by bill

Yeah I agree. Although there is an alternative, that Tree removes the reference to a item when deleting the item. I'm not sure what to think... in my world of databases the child contains a reference to the parent, rather than vice-versa, so this whole idea is foreign to me.

comment:10 Changed 12 years ago by guest

We also need this fix badly.

We've tried to delete the references manually but it doesn't seem to work - we don't have enough internal datastore knownledge apparantly.

The fact that deleting a node and then rerendering the tree with the same store makes it crash is a severe bug that should be given more attention. I find it unacceptable for this ticket to be postponed to 1.1.

Sure, with enough time maybe we can hack it ourselves and make it work, but it's in the best interest of all dijit.Tree users that this ugly bug is fixed once and for all.

comment:11 Changed 12 years ago by guest

I second that opinion. This bug basically prevents us from using the store like a store is supposed to be used, a storage for several interfaces.

Right now there is no difference in having the data stored in the dijit.Tree directly or using a store, since there is no point in having the store when it's not possible to reuse it.

We have also tried to 'hack' the tree/store to make it delete the references, but without success.

This bug can be compared to a grid deleting a row from a database, and then creating a new grid from the same database would fail.

Please resolve this bug :)

comment:12 Changed 12 years ago by guest

We have patched the dijit.Tree to remove the deleted items returned by the store. See the attached patch.

Regards, Thomas

thomas(at)webhuset.no

Changed 12 years ago by guest

Attachment: Tree.patch added

comment:13 Changed 12 years ago by guest

I forgot. The patch is for r11619

Thomas

comment:14 Changed 12 years ago by Jared Jurkiewicz

Just a note that this enhancement request isn't dead. I'm working on some changes to ItemFile?*Store to handle reference integrity such that if an item is deleted and other items hold reference to it, it will clean those up. The problem is doing it in a performant manner. Working slowly through it and doing a lot of testing in order to ensure it works decently.

comment:15 Changed 12 years ago by Dustin Machi

Jared, When you get a chance we should discuss this. I have this working and references working in jsonPathStore, and perhaps we could discuss this and make sure we do things in a way that doesn't muddy the waters or we could even further refine/define this portion of the spec to state the expected behavior.

comment:16 Changed 12 years ago by Jared Jurkiewicz

dmachi,

Sounds good to me. It has been a bit of a pain to implement efficiently in ItemFileWriteStore?. I would love it if you could review my approach. I'll be attaching a patch with my approach + a bunch of testcases when I'm happy I'm testing it enough. Effectively, for ItemFile?*Store, I'm keeping a reverse reference map so that for any item, I can access in O(1) time the list of items that have references to it. That then allows me to clean them up very, very, quickly. The only real fun is making sure the map stays consistent between deletes, news, setValue/setValues, etc, which is where I have been spending most of the time making sure it's keeping the map correct.

Changed 12 years ago by Jared Jurkiewicz

Patch to include efficent reference integrity code into the ItemFile?*Stores

comment:17 Changed 12 years ago by Jared Jurkiewicz

Cc: bill alex added

Okay, Dustin, others! I've added a patch to this tracker that updates both ItemFileReadStore? and ItemFileWriteStore? to do reference tracking and reference integrity (if you delete an item that is referenced by other items, it cleans up those references and calls onSet events for each item touched.) I've added 10 unit tests to test the various functions, but I would really like a review of the code and concepts behind the implementation. It should be fast, as the lookup for who references what is an O(1) operation. Then it's just a matter of iterating over all the items known to have references and invoking cleanup and change events. I've also tested it with Tree and it seems to *most* work with dijit.Tree. It actually looks like dijit.Tree may not handle the events quite right, depending on what is expanded in view in the HTML. I'll be attaching a simple test file that demos that as well (Just a tree with a button that deletes Item 10, an item with lots of references inside it, as well as references to it. If nothing was expanded, the delete works fine and the entire tree updates right. Now, if some were expanded, it only half-cleans up the tree view. (The events fired from the store don't change), so all I can figure is that it is a problem in Tree.

Bill, I've CCed you on too, to take a look at Tree and see if you can explain how tree is handing the deletes/updates and why it doesn't always seem to apply them.

Changed 12 years ago by Jared Jurkiewicz

Demo with tree. Just drop this as a peer file to dojo/, dijit/, dojox/

comment:18 Changed 12 years ago by Jared Jurkiewicz

Cc: Dustin Machi added

comment:19 Changed 12 years ago by Dustin Machi

Jared,

Looked quickly at your code and I think it looks fine. I haven't run it yet to see how it all plays out but the map seems fine. I did notice that one places does a clone by doing dojo.toJson(dojo.fromJson)) which I'm not sure is the most efficient way to do that, but perhaps it was being used to truncate things? If not, I would think dojo.clone() would perform better for you.

I'm not sure I'll get to look at the tree before I get back from Christmas break, but Bills comment on DnD was kind of confusing to me. I'm not sure why DnD is even in play in this problem that you are describing. The notification stuff is really only a few lines of code, and in my previous testing was working for referential integrity with jsonPathStore, but I will try to test again some more when i get back if Bill hasn't already figured it out.

All that said, the main thing I wanted to discuss was not how it was implemented but rather how it developers work with it. For jsonPathStore here is how references and referential integrity plays out.

  1. Adding refrenced items in the initial data set. In jsonPathStore, since it can take regular js objects, js referenced items are automatically setup as references in the store. I still need to add in a {_reference} type object for use when you are using JSON instead of js object literals, but was really hoping we could add some reference spec to the api so we could be consistent with that across stores.
  1. Once the store is created and items are in it, adding an item that is a reference to a new item is done with newItem as well. basically something like this:
    //create an item
    var item = {name: "foo", id: 1};
    var newItem = store.newItem(i,pInfo);
    
    //add a reference to this item somewhere else
    var secondItem = store.newItem(newItem, pInfo2);
    

So as you can see to add a reference to an item, we are just adding it again (the store item not the original object).

When you remove items there are two different things you can do and/or might want to accomplish.

  • delete the item and all references
  • delete a single reference

In jsonPathStore, this the deleteItem() method will do the former. It will remove the item and any references to that item and should trigger the update trigger for each of the parents that have an item or reference deleted. If, on the other hand, you wanted to remove a single reference, then that would be done by setValue() on the parent item. Simple remove that item from whatever property it is in.

This seems to be easy to use and understand and has worked out so far in what we've been using jsonPathStore for.

Have you had a chance to look at jsonPathStore. I'm interested in your comments on how I've done things and/or things i've added/further defined.

Dustin

Changed 12 years ago by Jared Jurkiewicz

Updated patch. Condensed a few sections, moved code from Read into Write (made read a stub). Fixed a UT problem, added a new test, and fixed a problem with child items (not specifically _referenced items)

Changed 12 years ago by Jared Jurkiewicz

Alternate ref integrity approach (ref information is attached to each item instead of a global map).

Changed 12 years ago by Jared Jurkiewicz

Updated reference integrity patch with optimizations suggested by Bill Keese + a couple more corner case fixes.

comment:20 Changed 12 years ago by Jared Jurkiewicz

The latest patch has been tested successfully against:

IE 6.

IE 7

Firefox 2.0.0.11

Firefox 1.5.0.12

Safari B3

Opera 9.2

SeaMonkey? 1.1.2

DOH Commandline unit test.

All passed. No pre-existing UT had to be changed to enable this function, so previous tests (1.0), should continue to work.

From the DOH commandline, results were: 335 tests to run in 25 groups


GROUP "tests.date.locale" has 9 tests to run


GROUP "tests.data.readOnlyItemFileTestTemplates, with datastore dojo.data.ItemFileReadStore?" has 56 tests to run


GROUP "tests._base._loader.hostenv_rhino" has 1 test to run


GROUP "tests._base.declare" has 12 tests to run


GROUP "tests.i18n" has 9 tests to run


GROUP "tests.data.readOnlyItemFileTestTemplates, with datastore dojo.data.ItemFileWriteStore?" has 56 tests to run


GROUP "tests.data.utils" has 18 tests to run


GROUP "tests._base._loader.loader" has 3 tests to run


GROUP "tests._base.array" has 13 tests to run


GROUP "tests.data.ItemFileWriteStore?" has 34 tests to run


GROUP "tests.number" has 26 tests to run


GROUP "tests.string" has 3 tests to run


GROUP "tests._base.lang" has 13 tests to run


GROUP "tests.cldr" has 1 test to run


GROUP "tests.date.math" has 12 tests to run second


GROUP "tests.currency" has 1 test to run


GROUP "tests._base.json" has 1 test to run


GROUP "tests.date.util" has 3 tests to run


GROUP "tests._base.Color" has 12 tests to run


GROUP "tests.date.stamp" has 2 tests to run


GROUP "tests.AdapterRegistry?" has 5 tests to run


GROUP "tests._base.connect" has 10 tests to run


GROUP "tests._base.Deferred" has 4 tests to run debug from dojo.Deferred callback


GROUP "tests._base._loader.bootstrap" has 5 tests to run


GROUP "tests.colors" has 26 tests to run


| TEST SUMMARY:


335 tests in 25 groups 0 errors 0 failures

comment:21 Changed 12 years ago by Jared Jurkiewicz

Resolution: fixed
Status: newclosed

(In [11972]) Updated to ItemFileReadStore? and ItemFileWriteStore? to enable reference integrity. fixes #4552 !strict

Note: See TracTickets for help on using tickets.