Nodes may be ranked on the same position in queue [#1168228]

I am extensively using the "Add to nodequeue" and "Remove from nodequeue" actions. Somehow positions may get out of sync, e. g. resulting in two nodes with the same position (cause unkown yet):

qid sqid nid position timestamp
7 20 76477 5 1302096639
7 20 75636 1 1297930240
7 20 76134 3 1300352414
7 20 76258 3 1301414845
7 20 76290 4 1301567390

I discovered that the re-index algorithm in nodequeue_subqueue_remove() cannot fix positions which have gotten out of sync as it only decrements higher positions by one on node removal.

The attached patch reorders the positions of all nodes. Be aware that it introduces n + 1 (n = number of nodes in queue) db queries instead of 1. I don't know a smarter way to do this, but maybe we can eliminate the cause.

Comment	File	Size	Author
#10	1168228-followup-prevent_duplicate_positions-10-D6.patch	820 bytes	amateescu
#10
#10	1168228-followup-prevent_duplicate_positions-10.patch	895 bytes	amateescu
#10
#8	1168228-prevent_duplicate_positions-8-D6.patch	850 bytes	amateescu
#8
#8	1168228-prevent_duplicate_positions-8.patch	983 bytes	amateescu
#8
#4	1168228-nodequeue_prevent_duplicate_positions.patch	1.06 KB	zyxware
#4
	nodequeue-unique-position-order.patch	1.19 KB	gnindl

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

amateescu CreditAttribution: amateescu commented 11 June 2011 at 23:58

Closed a duplicate: #1171198: Nodequeue allows more nodes than "Max Nodes" to be added to queue.

Comment #2

zyxware CreditAttribution: zyxware commented 13 June 2011 at 05:36

@amateescu - Is it required for the positions to be continuous on the number line?

Would a subquery to set the position work when you insert the node? Wouldn't that eliminate the duplicate node position issue?

Comment #3

druninja CreditAttribution: druninja commented 13 June 2011 at 06:27

Subscribing

Comment #4

zyxware CreditAttribution: zyxware commented 14 June 2011 at 22:01

File	Size
1168228-nodequeue_prevent_duplicate_positions.patch	1.06 KB

I think transaction is one way and another would be a subquery to get the max+1 position. Here is a patch for getting the position as a subquery and using that in the insert query in the nodequeue_subqueue_add function. It is a bit ugly with the subquery but I think this would work until (unless) another solution is found.

Comment #5

ezra-g CreditAttribution: ezra-g commented 14 June 2011 at 23:06

I don't see a problem with doing a subquery, so would be fine committing this with folks saying it resolves the issue.

Comment #6

amateescu CreditAttribution: amateescu commented 15 June 2011 at 07:08

I also like the subquery fix, but why is there an extra select? Shouldn't it be: SELECT MAX(position) + 1 FROM {nodequeue_nodes} WHERE sqid = %d ?

Comment #7

zyxware CreditAttribution: zyxware commented 15 June 2011 at 15:02

MySQL doesn’t allow selecting and inserting into the same table. The sub sub query works around this.

Comment #8

amateescu CreditAttribution: amateescu commented 19 June 2011 at 21:18

Status:

Needs review

» Fixed

File	Size
1168228-prevent_duplicate_positions-8.patch	983 bytes

1168228-prevent_duplicate_positions-8-D6.patch	850 bytes

@zyxware: your patch had some extra code in there that wasn't part of nodequeue and had some problems when inserting the first node in a queue so I added a ISNULL check in there. Commited attached patches to 6.x and 7.x.

http://drupalcode.org/project/nodequeue.git/commit/f5a9807
http://drupalcode.org/project/nodequeue.git/commit/4fcf255

Comment #9

zyxware CreditAttribution: zyxware commented 28 June 2011 at 00:18

Status:

Fixed

» Active

There is still an issue here. When multiple nodes get added into a nodequeue simultaneously the patch will ensure correct positions but the subqueue size is handled via variables in the page and this might have changed during the page load via another add request. So the count will have to be queried back again after the insert. Otherwise the spill over code will not be triggered during simultaneous inserts and nodequeue could grow larger than the limit.

$subqueue->count++;

should be

$subqueue->count = db_result(db_query("SELECT count(nid) FROM {nodequeue_nodes} WHERE sqid = %d", $subqueue->sqid));

(not sure if this had to be added as a new issue or hitchhike with this one)