Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#17837 closed defect (fixed)

Tree: exeception ObjectStoreModel.pasteItem() with asynchronous stores

Reported by: Wouter Hager Owned by: bill
Priority: undecided Milestone: 1.10
Component: Dijit Version: 1.9.3
Keywords: Cc:
Blocked By: Blocking:

Description

When using dijit/Tree + DnD + ObjectStoreModel? with a betweenThreshold AND an async store, childrenCache contains Promises.

This can be prevented by calling the getChildren method, as per API spec.

Change History (20)

comment:1 Changed 6 years ago by bill

Component: GeneralDijit
Owner: set to bill
Summary: pasteItem method in dijit/tree/ObjectStoreModel accesses childrenCache directlyTree: pasteItem method in dijit/tree/ObjectStoreModel accesses childrenCache directly

Presumably this has nothing to do with betweenThreshold?

comment:2 Changed 6 years ago by Wouter Hager

betweenThreshold is causing the problem. To reproduce, change the position of a treenode by dragging it before/after a sibling.

That is because the children are not loaded once the target will call expandNode. The target, in this case, is the sibling.

comment:3 Changed 6 years ago by Wouter Hager

I mean the target in dndSource.onDndDrop is the sibling. If the children aren't loaded, this target won't be available. Sorry I messed up the causality chain in my ticket.

comment:4 Changed 6 years ago by bill

When you file tickets, you need to explain what the bug is. Here you've said "pasteItem method in dijit/tree/ObjectStoreModel accesses childrenCache directly", but you didn't explain what problem that causes or how to reproduce that problem. Try to write the (1) steps to reproduce, (2) expected behavior, and (3) actual behavior.

comment:5 Changed 6 years ago by Wouter Hager

Problem: ObjectStoreModel?.childrenCache contains Promises when using an async store. In ObjectStoreModel?.pasteItem (called when DndDrop?) childrenCache entries are treated as values.

1) To reproduce, change the position of a treenode by dragging it before/after a sibling.

2) Expected behavior: tree nodes should be rearranged

3) Actual behavior: the parent node is empty.

The Dnd target expandNode method throws an error, because it's not loaded. This target is the before/after sibling of the dragged node.

Solution: call ObjectStoreModel?.getChildren.

Finally, IMO: 1) getChildren should not use caching 2) the store should manage the children in oldParentItem, not the model.

comment:6 Changed 6 years ago by bill

I agree there's a bug here.

FYI, the reason ObjectStoreModel? caches the results of getChildren() is because (when possible) it observes the children list, listening for updates. In other words, it sets up to be notified when the node's list of children changes:

// Setup listener in case children list changes, or the item(s) in the children list are
// updated in some way.
if(res.observe){
	res.observe(lang.hitch(this, function(obj, removedFrom, insertedInto){
	...

If the observer is working, there's no reason to requery the children for a given node. More importantly, multiple calls to getChildren() for the same node should not set up multiple observers, or at least there should only be one observer at any given time, and it should be destroyed when the model itself is destroyed.

comment:7 Changed 6 years ago by Wouter Hager

Sorry, I don't understand. The childrenCache isn't managed. To prevent 'observe' being called again, I'd suggest using a pointer to the handle that 'observe' provides.

comment:8 Changed 6 years ago by bill

I don't know what you mean by "the childrenCache isn't managed", or "using a pointer", but the idea is that if you call getChildren() 1000 times on the same item, it shouldn't setup 1000 observers listening for changes to the query results. I suppose "shouldn't" is debatable.

comment:9 Changed 6 years ago by Wouter Hager

I mean I don't see any updates to the cache, so it will get stale, no matter what. Or am I missing something?

Furthermore, I suggested adding an 'observe' handle property to the model:

if(res.observe && !this._childrenHandle) {
    this._childrenHandle = res.observe ...
destroy: function(){
    this._childrenHandle && this._childrenHandle.remove();

comment:10 Changed 6 years ago by bill

I mean I don't see any updates to the cache, so it will get stale, no matter what. Or am I missing something?

Yes, you are missing something subtle. Observable updates the cached value behind the scenes by using Array.splice().

About your suggestion to have a this._childrenHandle property, it doesn't make sense because ObjectStoreModel? needs to keep track of many handles, not just one.

In any case, I'm going to check in an update now that tests and fixes ObjectStoreModel? for asynchronous stores. There are likely still issues with JsonRest? itself, but AFAIK this fixes the issues in the Tree code.

comment:11 Changed 6 years ago by Bill Keese <bill@…>

Resolution: fixed
Status: newclosed

In 302c1fe542b646a07dfce94172e4a61345768354/dijit:

Error: Processor CommitTicketReference failed
Unsupported version control system "git": Can't find an appropriate component, maybe the corresponding plugin was not enabled? 

comment:12 Changed 6 years ago by bill

Milestone: tbd1.10

comment:13 Changed 6 years ago by Wouter Hager

So, as I understand it you're relying on Observable to change the state. I don't think observing should involve state changes, but that's another discussion. However, I would welcome some comments about that.

Considering handles, perhaps ObjectStoreModel? could inherit from Destroyable?

comment:14 Changed 6 years ago by bill

Yeah, it's weird that observing has that side effect. It is efficient though; it avoid having to make a copy of every array returned from query().

I agree, probably it makes sense for ObjectStoreModel? to inherit from Destroyable; I'll try that.

comment:15 Changed 6 years ago by Bill Keese <bill@…>

In 71c34ca3d7486d77c4bac7e06eaff4595699d2a6/dijit:

Error: Processor CommitTicketReference failed
Unsupported version control system "git": Can't find an appropriate component, maybe the corresponding plugin was not enabled? 

comment:16 Changed 6 years ago by bill

Summary: Tree: pasteItem method in dijit/tree/ObjectStoreModel accesses childrenCache directlyTree: exeception ObjectStoreModel.pasteItem() with asynchronous stores

Note that 71c34ca3d7486d77c4bac7e06eaff4595699d2a6 requires #17680.

comment:17 Changed 6 years ago by Bill Keese <bill@…>

In 37ff15fd02928e813db7b18a1cfe5926a9857e04/dijit:

Error: Processor CommitTicketReference failed
Unsupported version control system "git": Can't find an appropriate component, maybe the corresponding plugin was not enabled? 

comment:18 Changed 6 years ago by Wouter Hager

I'm sorry, but I disagree with this fix. I thought about it for a while, and I came to the conclusion that Observable is a piece of over-engineering that does a lot to obfuscate the workings of the ObjectStoreModel?. In contrast with the JsonRest? store, which is very concise, it requires additional code (SimpleQueryEngine?) that repeats functionality that should have been taken care of server-side.

I vote for removing the Observable requirement from ObjectStoreModel? and skimming the code as much as possible.

Last edited 6 years ago by Wouter Hager (previous) (diff)

comment:19 Changed 6 years ago by bill

You're welcome to have your own opinion but let's limit use of the bug database to reporting bugs.

Also though, it sounds like you are just talking about #15893.

comment:20 Changed 6 years ago by Wouter Hager

It's certainly a related concern: Observable may work fine for Memory, but it has been known to create false expectations towards JsonRest? (indeed, comet).

Note: See TracTickets for help on using tickets.