431431 - Places Query: Combination of nsINavHistoryQuery Attributes and "RESULTS_AS_TAG_QUERY" creates a broken SQL query

Status: RESOLVED INACTIVE

(Firefox :: Bookmarks & History, defect)

Description

[Erwan Loisant]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.14) Gecko/20080414 Firefox/2.0.0.14 Flock/1.1.2
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9pre) Gecko/2008042913 Firefox/3.0pre

A query such as:
place:annotation=<something>&type=<CI.nsINavHistoryQueryOptions.RESULTS_AS_TAG_QUERY>

Generates this kind of SQL query:
SELECT null, 'place:folder=' || id || '&queryType=1&type=7', title, null, null, null, null, null, null, dateAdded, lastModified FROM   moz_bookmarks WHERE  parent = 4WHERE (   EXISTS (SELECT h.id FROM moz_annos anno JOIN moz_anno_attributes annoname ON anno.anno_attribute_id = annoname.id WHERE anno.place_id = h.id AND annoname.name =   :anno  ) )

2 problems here:
* There are 2 "WHERE" statements
* The annotation parameter should be propagated to the tag query

It doesn't really impact Firefox itself, because this kind of query is not used in Firefox. It's more for extensions or future features.


Reproducible: Always

Comment 1

[cmtalbert]

The cause is a little broader than this.  If you use any other nsINavHistoryQuery option that adds another WHERE clause, you're going to get this.  Right now, we're throwing because of the missing space between the "4" and second WHERE.

I'll attach a test that causes the same problem using only a searchterm and a minVisit query.

But, here's the odd thing: if you comment out the both the searchterm and the minVisits from this test, you get the following SQL query (which works):
SELECT null, 'place:folder=' || id || '&queryType=1&type=7', title, null, null, null, null, null, null, dateAdded, lastModified FROM   moz_bookmarks WHERE  parent = 4

And if you put the searchterm back in, and keep minVisits commented out, you get the same query:
SELECT null, 'place:folder=' || id || '&queryType=1&type=7', title, null, null, null, null, null, null, dateAdded, lastModified FROM   moz_bookmarks WHERE  parent = 4

However, with only the searchterm attribute specified, you don't get a valid result object back and the query fails.  There are no assertions fired, it just silently refuses to work.

I'll attach my test and change the summary to reflect the nature of the problem.

Tested on April 30, 2008 debug build. --> CONFIRMED

Comment 2

[cmtalbert]

Attachment: Test case demonstrating the problem

Comment 3

[Erwan Loisant]

The problem with 2 WHERE and a missing space is actually pretty easy to fix (I'll send a patch). All we need to do is add {ADDITIONAL_CONDITIONS} at the right place. When {ADDITIONAL_CONDITIONS} is present it gets replaced by AND... otherwise WHERE gets appended.

What's more complex is propagating the conditions to the tag query, especially in the case of 2 queries together (that end up in a "OR").

Comment 4

[Marco Bonardo [:mak]]

there's no point in adding condition to a query that is returning only "virtual" containers... conditions should be converted to inside places queries but that's quite complex.

Comment 5

[Erwan Loisant]

The patch I have (locally) is doing that: generating a query to get tags that have at least one bookmark that match the annotation, then propagates the annotation to the tag query.

The problem is that I'm not sure we can put a "or" in a serialized tag query, but that's what we should do to take into account the case where 2 queries are passed in the same call.

Comment 6

[Erwan Loisant]

Attachment: Fixes queries with annotation, results as tag query

Here is a first shot. It fixes queries having both annotation and RESULT_AS_TAG_QUERY by generating:
* A query that returns all tags having at least a "child" bookmark having the requested annotation
* The query associated to each tags returns bookmarks having the annotation, and tagged with the tag

Now, this is not perfect in the case you pass more than one query to executeQueries; it will only propagate the annotation from the first query. (As far as I understand, "multiple queries" are used to do a logical "OR" because a
nsINavHistoryQuery can only accumulate "AND" statements).

The reason is that the query must be serialized in a string, and I don't think we can do a "OR" in a serialized query.

Comment 7

[Marco Bonardo [:mak]]

in that way additional conditions would be applied only to tag containers rather than to their contents, and you only support annotations for contents, while there is a bunch of other possible fields we could query on (and finish up generating a wrong query). For example if i search for a term it should be applied to bookmarks contained in a tag and not to tags, searches should be always applied to contents while resultType should define a grouping way to return results.

Comment 8

[Marco Bonardo [:mak]]

So, the only way to fix this is to take all parameters from the original query and pass it down to the *TAG_CONTENTS query, through SQL. The problem is that *TAG_CONTENTS query could not be able to manage all parameters because it's a really special remapping query, that won't know about visits parameters. Better would be to change our tags implementation, and build a simpler query, then passing params down to descendant queries should just work.

Comment 9

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv

Comment 10

[BMO Automation]

Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.