ItemFileWriteStore: automatically remove references to item when deleting the item

[guest]

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.

[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?

[bill]

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?)

[jaredj]

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.

[bill]

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.

[jaredj]

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.

[jaredj]

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

[dmachi]

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.

[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.

[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.

[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 :)

[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

[guest]

I forgot. The patch is for r11619

Thomas

[jaredj]

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.

[dmachi]

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.

[jaredj]

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.

[jaredj]

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

[jaredj]

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.

[jaredj]

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

[dmachi]

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.

2. 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

[jaredj]

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)

[jaredj]

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

[jaredj]

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

[jaredj]

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

[jaredj]

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