Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#17836 closed defect (duplicate)

Problems with dijit/Tree with a ObjectStoreModel using dojo/store/JsonRest

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

Description

This lists all current problems with the "new" ObjectStoreModel? and the Tree widget. This use case uses Persevere 2.0 as remote store. As Persevere sails under the Dojo foundation flag, I see no problem to assume this would be a likely scenario.

To setup Persevere:

To use the test:

  • point the store target to your setup
  • To create / delete tree nodes, select a node and right/ctrl-click for context menu

Problems found:

  • Store doesn't observe items being added
  • Dragging/dropping items causes children to be removed

Attachments (3)

test.html (2.5 KB) - added by Wouter Hager 6 years ago.
test html file
sandbox.js (648 bytes) - added by Wouter Hager 6 years ago.
sandbox model
sandbox.2.js (723 bytes) - added by Wouter Hager 6 years ago.
sandbox facet

Download all attachments as: .zip

Change History (34)

Changed 6 years ago by Wouter Hager

Attachment: test.html added

test html file

Changed 6 years ago by Wouter Hager

Attachment: sandbox.js added

sandbox model

Changed 6 years ago by Wouter Hager

Attachment: sandbox.2.js added

sandbox facet

comment:1 Changed 6 years ago by Wouter Hager

Related: #17783

comment:2 Changed 6 years ago by Wouter Hager

Related?: #16692

comment:3 Changed 6 years ago by Wouter Hager

Related: #17837

comment:4 Changed 6 years ago by bill

Component: GeneralDijit
Owner: set to bill

Is this ticket just a duplicate of those others?

comment:5 Changed 6 years ago by bill

Anyway, I agree that Persevere, JsonRest?, and dijit/Tree could be a common combination, but to be honest it will be hard to fix such problems since they span across three components. The challenge is finding out which component is broken and getting an isolated test.

comment:6 Changed 6 years ago by Wouter Hager

I thought it useful to create a global ticket. You can take Persevere out of the equation any time. I've created isolated tests and referenced them here. If you disagree, just close it.

comment:7 Changed 6 years ago by Wouter Hager

When #17783 is fixed, a new error emerges down the line:

The observe function on objectStoreModel::getChildren will just callback for all parentItems that have had their children loaded. The only way to prevent all this happening is simply to add a queryEngine to the store.

Since I don't know what is causing this I created a new ticket: #17848

comment:8 Changed 6 years ago by Wouter Hager

There's another problem, the "before" option will only update the newParentItem, but not the oldParentItem. I don't understand how this is supposed to work. Say I have some order property in my child items, and update them when the position within the parent changes, it's ok, but when I change it to another item, the position in the old parent changes, so I want to have that updated. Or do I misunderstand "before"? I can't find any documentation. Should I just use the id of the sibling before which this child comes?

edit: documentation @ http://livedocs.dojotoolkit.org/dojo/store#methods

id is indeed used as "before" property.

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

comment:9 Changed 6 years ago by Wouter Hager

Also when I would use a "before" id, I should update something from my previous location. I get the impression this was not thought through well enough. Any docs to refute this?

comment:10 Changed 6 years ago by Wouter Hager

Related ticket: #15660

comment:11 Changed 6 years ago by Wouter Hager

Related ticket: #17642

Observable has no knowledge of "before", which is another case for handling relational data through other means (like hypermedia api)

comment:12 Changed 6 years ago by bill

There's another problem, the "before" option will only update the newParentItem, but not the oldParentItem.

I'm not sure if you think there's a bug in the Tree code or just dojo/store. The new dojo/stores (specifically Json and Memory) don't support inserting nodes into a position at all, as I already wrote in #15660, so you need to have your own store that supports it. But if there's a problem with Tree itself then please file a ticket with a minimal test case, or just instructions for reproducing against test_Tree_DnD.html.

comment:13 Changed 6 years ago by Wouter Hager

Well in that case there's no problem with that, but there's no documentation that says how to use before with all the components involved here, which seems vital for anyone who wants to use a Tree with JsonRest?. I can't imagine this is the kind of setup where everyone needs to reinvent the wheel for themselves, and judging from the other tickets that's what's happening now.

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

comment:14 Changed 6 years ago by bill

I agree, that's why I filed #15660.

comment:15 Changed 6 years ago by Wouter Hager

JsonRest? store with before bookkeeping

on insert item with parent and before:

  • changingItem = get item where parentId=parent.id and beforeId = before.id
  • update beforeId of changingItem to inserted.id

on insert item parent and before=null (append at end):

  • changingItem = get item where parentId=parent.id and beforeId = null
  • update beforeId of changingItem to inserted.id

on update item with parent and before:

  • changingItem1 = get item where parentId=parent.id and beforeId = before.id
  • changingItem2 = get item where parentId=parent.id and beforeId = updating.id
  • update beforeId of changingItem1 to updated.id
  • update beforeId of changingItem2 to updated.beforeId

comment:16 Changed 6 years ago by Wouter Hager

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

comment:17 Changed 6 years ago by Wouter Hager

Client-side sorting algorithm based on beforeId (perhaps not the most efficient or safe). Any other ideas very welcome.

var sorted = [];
// iterate through results and move correct item to sorted
var beforeId;
while(results.length) {
	var item = results.pop();
	if(item.beforeId==beforeId) {
		// found, update search and put in sorted
		beforeId = item.id;
		sorted.unshift(item);
	} else {
		// not found, put back in front
		results.unshift(item);
	}
}

comment:18 Changed 6 years ago by Wouter Hager

I still believe the proposed implementation is too tightly coupled with a very specific relational model. It will be more future proof if the client reads the relationships from a hyperschema or some meta source that can be used to create a more generic solution. I'll work on that after this specific implementation is entirely sorted out.

comment:19 Changed 6 years ago by Wouter Hager

Obviously, the before bookkeeping needs to take into account when the parent changes, so then it becomes oldParent.id instead of parent.id, which makes it quite a list of things to do...

comment:20 Changed 6 years ago by dylan

Cc: Kris Zyp added

comment:21 Changed 6 years ago by Wouter Hager

I've decided it's much simpler if the store just receives the oldParent. I'd also like to keep things clean, but to be alter the beforeId of the old sibling I have to be sure that I refer to the correct parent.

In some cases, the old parentId of the updating item may already be changed. In these cases it's necessary to get fresh data from the remote store.

There also may be other kinds of stores (e.g. Neo4j) that demand that the old parent is provided, because there is no other way to find out the starting point of the incoming relationship to the updating item.

Perhaps these examples are too specific already to consider modifying ObjectStoreModel? instead of just extending it, but oldParentItem is just sitting there, so why not send it with the store options anyway?

I'll create a patch for ObjectStoreModel? once we all agree on this minor API change.

comment:22 Changed 6 years ago by Wouter Hager

This could be the wrapper for the JsonRest? store when using oldParent. The changing branches should get re-queried and hence -ordered and -rendered through getChildren.observe, but I'm not yet convinced it won't get messy. I'll test this to see how it goes.

aspect.around(store,"put",function(put){
	return function(item,options){
		options = options || {};
		if(options.parent) {
			// item gets new parent
			item.parentId = store.getIdentity(options.parent);
			function updateBefore(parent,id,beforeId) {
				var parentId = store.getIdentity(parent);
				// query the store for the changing sibling
				return when(store.query({parentId:parentId,beforeId:id}),function(items){
					if(items && items.length>0) {
						var item = items[0];
						if(beforeId) {
							item.beforeId = beforeId;
						} else {
							delete item.beforeId;
						}
						// calling original put
						return put.call(store,item,options);
					}
					// what to do when not found?
					return new Deferred().reject();
				}
			}
			// if oldParent is sent with the options it means there's a move
			if(options.oldParent) {
				var promises = [];
				if(options.oldParent!=options.parent) {
					// old changing before: item.id => item.beforeId
					promises.push(updateBefore(options.oldParent,store.getIdentity(item),item.beforeId));
				} else {
					// old changing before: item.id => item.beforeId
					promises.push(updateBefore(options.parent,store.getIdentity(item),item.beforeId));
				}
				// new changing before: before.id => item.id
				promises.push(updateBefore(options.parent,options.before ? store.getIdentity(options.before) : null,store.getIdentity(item)));
				// update item before
				if(options.before) {
					item.beforeId = store.getIdentity(options.before);
				} else {
					delete item.beforeId;
				}
				promises.push(put.call(store,item,options));
				return all(promises);
			} else {
				// assume this is new data
				return when(put.call(store,item,options)),function(item){
					return updateBefore(options.parent,options.before ? store.getIdentity(options.before) : null,store.getIdentity(item));
				});
			}
		} else {
			// just another update
			return put.call(store,item,options);
		}
	};
});
Last edited 6 years ago by Wouter Hager (previous) (diff)

comment:23 Changed 6 years ago by Wouter Hager

I've updated the sample, all updating branches should be re-rendered by loading fresh data in ObjectStoreModel?.pasteItem...

comment:24 Changed 6 years ago by Wouter Hager

Ok I've had it. Because getChildren uses a cache, Observable will never work for ObjectStoreModel?.pasteItem and JsonRest?. Even if the parent is updated, getChildren is not called again, so even without all the superfluous state, observing getChildren would be useless. The only solution is ignoring Observable in pasteItem, and just call (a cacheless) getChildren for newParent and oldParent.

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

comment:25 Changed 6 years ago by bill

I've decided it's much simpler if the store just receives the oldParent.... I'll create a patch for ObjectStoreModel? once we all agree on this minor API change.

IIUC, you want to modify dijit/tree/ObjectStoreModel.pasteItem()'s call to dojo/store.put(), to additionally pass in the oldParentItem.

It's a harmless change, but since passing the old parent item isn't in the spec for stores (https://dojotoolkit.org/reference-guide/1.9/dojo/store.html#methods), I'd lean to having the application extend dijit/tree/ObjectStoreModel, rather than changing dijit/tree/ObjectStoreModel itself.

Because getChildren uses a cache, Observable will never work for ObjectStoreModel?.pasteItem and JsonRest?. Even if the parent is updated, getChildren is not called again, so even without all the superfluous state, observing getChildren would be useless. The only solution is ignoring Observable in pasteItem, and just call (a cacheless) getChildren for newParent and oldParent.

It does look like dijit/tree/ObectStoreModel.childrenCache[] can get stale. I'm not sure why I added that cache in the first place. But in any case, feel free to file a ticket for that issue with a simple test case, and then write the ticket number here.

If you have enough time, it would be great if you could update dijit/tests/tree/Tree_ObjectStoreModel.html with an automated test case. Theoretically you can demonstrate the problem by a call to getChildren() followed by a store update, then another call to getChildren(). In other words the problem can be reproduced w/just using the Memory store.

Could be done in conjunction with #17837 since they are obviously related.

PS: I'm not sure if Tree ever calls getChildren() twice on the same node? If you have a test case where Tree does that (and gets the error) that would be good. Otherwise its more of a corner-case bug.

Last edited 6 years ago by bill (previous) (diff)

comment:26 Changed 6 years ago by Wouter Hager

With the childrencache taken out of ObjectStoreModel?, I should have been able to use Observable to update the parent, but there seems to be a bug in Persevere that I overlooked earlier. Problem is that when updating an object with links (Json Reference), the result contains the $refs from the PUT request, not the actual resolved links. I will address this issue at github.com/persvr. I'm getting very close now, I'll add the test you mentioned asap.

comment:27 Changed 6 years ago by bill

Regarding tests, dijit has a Tree_with_JRS.html test that runs against dijit.tree.ForestStoreModel? and dojox.data.JsonRestStore?. We should use that as a template to make a test running against ObjectStoreModel? and dojo/store/JsonRest. Hopefully that's good enough to reproduce the bugs you are seeing.

PS: You can't update the data since it's just sitting in the filesystem. But dojo/store/Cache will temporarily store updates. However, it passes all queries through to the underlying store. Probably a test where each store item has a children[] attribute would be possible.

Last edited 6 years ago by bill (previous) (diff)

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

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:29 Changed 6 years ago by bill

Resolution: duplicate
Status: newclosed

I just checked in a fix for #16692. I'm going to close this meta-ticket now. I agree that supporting Tree against JsonRest? is important, but it's just too hard to follow such a long ticket mentioning bugs across multiple components owned by different people.

comment:30 Changed 6 years ago by Wouter Hager

Agreed. Perhaps we could setup a cloudant sandbox for testing purposes.

comment:31 Changed 6 years ago by bill

Maybe... the long term goal would be to convert all the tests to run under Intern instead of DOH, at which point IIUC it could fire up a Persevere as part of the test run. But that's a lot of work.

Note: See TracTickets for help on using tickets.