fix for bug #394508: queries that use maxResults can return incorrect results due to post query filtering. initial patch=Colin Walters <walters@verbum.org> r=dietrich, a=mconnor

This commit is contained in:
sspitzer@mozilla.org 2007-09-28 16:36:03 -07:00
parent bf8fe50fb6
commit a67ceea715
2 changed files with 68 additions and 50 deletions

View File

@ -1558,7 +1558,7 @@ nsNavHistory::EvaluateQueryForNode(const nsCOMArray<nsNavHistoryQuery>& aQueries
nsCOMArray<nsNavHistoryQuery> queries;
queries.AppendObject(query);
nsCOMArray<nsNavHistoryResultNode> filteredSet;
nsresult rv = FilterResultSet(nsnull, inputSet, &filteredSet, queries);
nsresult rv = FilterResultSet(nsnull, inputSet, &filteredSet, queries, aOptions);
if (NS_FAILED(rv))
continue;
if (! filteredSet.Count())
@ -2144,6 +2144,32 @@ PRBool IsHistoryMenuQuery(const nsCOMArray<nsNavHistoryQuery>& aQueries, nsNavHi
return PR_TRUE;
}
static
PRBool NeedToFilterResultSet(const nsCOMArray<nsNavHistoryQuery>& aQueries,
nsNavHistoryQueryOptions *aOptions)
{
// optimize the case where we just want a list with no grouping: this
// directly fills in the results and we avoid a copy of the whole list
PRUint32 groupCount;
const PRUint16 *groupings = aOptions->GroupingMode(&groupCount);
if (groupCount != 0 || aOptions->ExcludeQueries())
return PR_TRUE;
PRInt32 i;
for (i = 0; i < aQueries.Count(); i ++) {
if (aQueries[i]->Folders().Length() != 0) {
return PR_TRUE;
} else {
PRBool hasSearchTerms;
nsresult rv = aQueries[i]->GetHasSearchTerms(&hasSearchTerms);
if (NS_FAILED(rv) || hasSearchTerms)
return PR_TRUE;
}
}
return PR_FALSE;
}
nsresult
nsNavHistory::ConstructQueryString(const nsCOMArray<nsNavHistoryQuery>& aQueries,
nsNavHistoryQueryOptions *aOptions,
@ -2155,6 +2181,11 @@ nsNavHistory::ConstructQueryString(const nsCOMArray<nsNavHistoryQuery>& aQueries
return NS_ERROR_INVALID_ARG;
}
// determine whether we can push maxResults constraints
// into the queries as LIMIT, or if we need to do result count clamping later
// using FilterResultSet()
PRBool canLimitInSQL = !NeedToFilterResultSet(aQueries, aOptions);
// for the very special query for the history menu
// we generate a super-optimized SQL query
if (IsHistoryMenuQuery(aQueries, aOptions)) {
@ -2169,8 +2200,11 @@ nsNavHistory::ConstructQueryString(const nsCOMArray<nsNavHistoryQuery>& aQueries
"(h.id IN (SELECT DISTINCT h.id FROM moz_historyvisits, "
" moz_places h WHERE place_id = "
" h.id AND hidden <> 1 AND visit_type <> 4 AND visit_type <> 0 "
" ORDER BY visit_date DESC LIMIT ");
queryString.AppendInt(aOptions->MaxResults());
" ORDER BY visit_date DESC");
if (canLimitInSQL) {
queryString += NS_LITERAL_CSTRING(" LIMIT ");
queryString.AppendInt(aOptions->MaxResults());
}
queryString += NS_LITERAL_CSTRING(")) GROUP BY h.id ORDER BY 6 DESC"); // v.visit_date
return NS_OK;
}
@ -2238,8 +2272,7 @@ nsNavHistory::ConstructQueryString(const nsCOMArray<nsNavHistoryQuery>& aQueries
"LEFT OUTER JOIN moz_historyvisits v ON b.fk = v.place_id "
"LEFT OUTER JOIN moz_favicons f ON h.favicon_id = f.id ");
groupBy = NS_LITERAL_CSTRING(" GROUP BY b.id");
}
else {
} else {
// XXX: implement support for nsINavHistoryQueryOptions::QUERY_TYPE_UNIFIED
return NS_ERROR_NOT_IMPLEMENTED;
}
@ -2334,8 +2367,8 @@ nsNavHistory::ConstructQueryString(const nsCOMArray<nsNavHistoryQuery>& aQueries
NS_NOTREACHED("Invalid sorting mode");
}
// limit clause if there are 'maxResults'
if (aOptions->MaxResults() > 0) {
// limit clause if there are 'maxResults' and can use limit in our SQL
if (canLimitInSQL && aOptions->MaxResults() > 0) {
queryString += NS_LITERAL_CSTRING(" LIMIT ");
queryString.AppendInt(aOptions->MaxResults());
queryString.AppendLiteral(" ");
@ -2390,55 +2423,30 @@ nsNavHistory::GetQueryResults(nsNavHistoryQueryResultNode *aResultNode,
// bind parameters
PRInt32 numParameters = 0;
// optimize the case where we just want a list with no grouping: this
// directly fills in the results and we avoid a copy of the whole list
PRBool resultAsList = PR_TRUE;
PRUint32 groupCount;
const PRUint16 *groupings = aOptions->GroupingMode(&groupCount);
if (groupCount != 0 || aOptions->ExcludeQueries()) {
resultAsList = PR_FALSE;
}
PRInt32 i;
for (i = 0; i < aQueries.Count(); i ++) {
PRInt32 clauseParameters = 0;
rv = BindQueryClauseParameters(statement, numParameters,
aQueries[i], aOptions, &clauseParameters);
NS_ENSURE_SUCCESS(rv, rv);
numParameters += clauseParameters;
if (resultAsList) {
if (aQueries[i]->Folders().Length() != 0) {
resultAsList = PR_FALSE;
} else {
PRBool hasSearchTerms;
rv = aQueries[i]->GetHasSearchTerms(&hasSearchTerms);
if (hasSearchTerms)
resultAsList = PR_FALSE;
NS_ENSURE_SUCCESS(rv, rv);
}
}
}
if (resultAsList) {
rv = ResultsAsList(statement, aOptions, aResults);
NS_ENSURE_SUCCESS(rv, rv);
} else {
// optimize the case where we just use the results as is
// and we don't need to do any post-query filtering
if (NeedToFilterResultSet(aQueries, aOptions)) {
// generate the toplevel results
nsCOMArray<nsNavHistoryResultNode> toplevel;
rv = ResultsAsList(statement, aOptions, &toplevel);
NS_ENSURE_SUCCESS(rv, rv);
PRUint32 groupCount;
const PRUint16 *groupings = aOptions->GroupingMode(&groupCount);
if (groupCount == 0) {
FilterResultSet(aResultNode, toplevel, aResults, aQueries);
FilterResultSet(aResultNode, toplevel, aResults, aQueries, aOptions);
} else {
nsCOMArray<nsNavHistoryResultNode> filteredResults;
FilterResultSet(aResultNode, toplevel, &filteredResults, aQueries);
FilterResultSet(aResultNode, toplevel, &filteredResults, aQueries, aOptions);
rv = RecursiveGroup(aResultNode, filteredResults, groupings, groupCount,
aResults);
NS_ENSURE_SUCCESS(rv, rv);
}
}
} else {
rv = ResultsAsList(statement, aOptions, aResults);
NS_ENSURE_SUCCESS(rv, rv);
}
return NS_OK;
}
@ -4127,16 +4135,22 @@ nsNavHistory::URIHasTag(nsIURI* aURI, const nsAString& aTag)
// nsNavHistory::FilterResultSet
//
// This does some post-query-execution filtering:
// - searching on title & url
// - parent folder (recursively)
// - excludeQueries
// This does some post-query-execution filtering:
// - searching on title & url
// - parent folder (recursively)
// - excludeQueries
// - tags
// - limit count
//
// Note: changes to filtering in FilterResultSet()
// may require changes to NeedToFilterResultSet()
nsresult
nsNavHistory::FilterResultSet(nsNavHistoryQueryResultNode* aQueryNode,
const nsCOMArray<nsNavHistoryResultNode>& aSet,
nsCOMArray<nsNavHistoryResultNode>* aFiltered,
const nsCOMArray<nsNavHistoryQuery>& aQueries)
const nsCOMArray<nsNavHistoryQuery>& aQueries,
nsNavHistoryQueryOptions *aOptions)
{
nsresult rv;
@ -4260,6 +4274,9 @@ nsNavHistory::FilterResultSet(nsNavHistoryQueryResultNode* aQueryNode,
}
if (appendNode)
aFiltered->AppendObject(aSet[nodeIndex]);
if (aOptions->MaxResults() > 0 && aFiltered->Count() >= aOptions->MaxResults())
break;
}
// de-allocate the matrixes

View File

@ -516,7 +516,8 @@ protected:
nsresult FilterResultSet(nsNavHistoryQueryResultNode *aParentNode,
const nsCOMArray<nsNavHistoryResultNode>& aSet,
nsCOMArray<nsNavHistoryResultNode>* aFiltered,
const nsCOMArray<nsNavHistoryQuery>& aQueries);
const nsCOMArray<nsNavHistoryQuery>& aQueries,
nsNavHistoryQueryOptions* aOptions);
// observers
nsMaybeWeakPtrArray<nsINavHistoryObserver> mObservers;