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

NEW
Unassigned

Status

()

Firefox
Bookmarks & History
--
minor
10 years ago
6 years ago

People

(Reporter: Erwan Loisant, Unassigned)

Tracking

(Depends on: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [3.1])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

10 years ago
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
Whiteboard: [3.1]

Comment 1

10 years ago
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
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: Places Query: Combination of "annotation" and "RESULTS_AS_TAG_QUERY" creates a broken SQL query → Places Query: Combination of nsINavHistoryQuery Attributes and "RESULTS_AS_TAG_QUERY" creates a broken SQL query

Comment 2

10 years ago
Created attachment 318818 [details]
Test case demonstrating the problem
(Reporter)

Comment 3

10 years ago
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

10 years ago
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.
(Reporter)

Comment 5

10 years ago
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.
(Reporter)

Comment 6

10 years ago
Created attachment 321653 [details] [diff] [review]
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

10 years ago
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.

Updated

8 years ago
Blocks: 509868

Updated

8 years ago
Attachment #321653 - Attachment is obsolete: true
Attachment #321653 - Flags: review-
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.

Updated

8 years ago
Depends on: 511890
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
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.