Bug Hunting Session
Bug 67647 - SQL/SingleSelectQueryComposer::getStructuredFilter/HavingClause returns comparison operator in value
Summary: SQL/SingleSelectQueryComposer::getStructuredFilter/HavingClause returns compa...
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Base (show other bugs)
Version:
(earliest affected)
unspecified
Hardware: All All
: medium major
Assignee: Not Assigned
URL:
Whiteboard: target:5.3.0
Keywords: difficultyInteresting, easyHack, skillCpp, skillJava, skillSql
Depends on:
Blocks:
 
Reported: 2013-08-02 05:12 UTC by Lionel Elie Mamane
Modified: 2017-02-14 08:57 UTC (History)
3 users (show)

See Also:
Crash report or crash signature:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Lionel Elie Mamane 2013-08-02 05:12:09 UTC
Let cmp be a SQLQueryComposer or SingleSelectQueryComposer that gives a query like:

  SELECT foo, bar FROM qux WHERE baz > 5;

cmp.getStructuredFilter() returns something like:

 [[ {Name=baz, Handle=GREATER, Value="> 5"} ]]

while it should return something like:

 [[ {Name=baz, Handle=GREATER, Value="5"} ]]

The latter (not the former!) is the format expected by setStructuredFilter. Which means that (in Basic):

 cmp.StructuredFilter = cmp.StructuredFilter

will fail.

This has been noticed several times through the codebase, but instead of fixing getStructuredFilter(), each caller works around this.

IMHO we should fix it.

Anyway, encoding the operator in PropertyValue.Handle is IMHO an abuse of PropertyValue. This should have its own datatype (struct).

So instead of just fixing getStructuredFilter, we could just deprecate it and replace it by something better, e.g. along the lines of the suggestion in dbaccess/source/ui/browser/unodatbr.cxx (or something better):

// check if the columns participating in the filter refer to existing tables
// TODO: there's no API at all for this. The method which comes nearest to what we need is
// "getStructuredFilter", but it returns pure column names only. That is, for a statement like
// "SELECT * FROM <table> WHERE <other_table>.<column> = <value>", it will return "<column>". But
// there's no API at all to retrieve the information about  "<other_table>" - which is what would
// be needed here.
// That'd be a chance to replace getStructuredFilter with something more reasonable. This method
// has at least one other problem: For a clause like "<column> != <value>", it will return "<column>"
// as column name, "NOT_EQUAL" as operator, and "!= <value>" as value, effectively duplicating the
// information about the operator, and beding all clients to manually remove the "!=" from the value
// string.
// So, what really would be handy, is some
//   XNormalizedFilter getNormalizedFilter();
// with
//   interface XDisjunctiveFilterExpression
//   {
//     XConjunctiveFilterTerm getTerm( int index );
//   }
//   interface XConjunctiveFilterTerm
//   {
//     ComparisonPredicate getPredicate( int index );
//   }
//   struct ComparisonPredicate
//   {
//     XComparisonOperand   Lhs;
//     SQLFilterOperator    Operator;
//     XComparisonOperand   Rhs;
//   }
//   interface XComparisonOperand
//   {
//     SQLFilterOperand Type;
//     XPropertySet     getColumn();
//     string           getLiteral();
//     ...
//   }
//   enum SQLFilterOperand { Column, Literal, ... }
//
// ... or something like this ....



Notable caller: wizards/com/sun/star/wizards/db/SQLQueryComposer.java

See in particular the function getNormalizedStructuredFilter()
Comment 1 Björn Michaelsen 2013-10-04 18:46:20 UTC
adding LibreOffice developer list as CC to unresolved EasyHacks for better visibility.

see e.g. http://nabble.documentfoundation.org/minutes-of-ESC-call-td4076214.html for details
Comment 2 Alex Thurgood 2015-01-03 17:38:07 UTC Comment hidden (no-value)
Comment 3 Robinson Tryon (qubit) 2015-12-13 13:24:13 UTC Comment hidden (obsolete)
Comment 4 Robinson Tryon (qubit) 2016-02-18 14:51:28 UTC Comment hidden (obsolete)
Comment 5 kerem 2016-03-21 08:59:05 UTC
I sent following for this patch;

https://gerrit.libreoffice.org/#/c/23392/
Comment 6 Lionel Elie Mamane 2016-03-21 10:38:06 UTC
To summarise and make things clear:

The EasyHack part of this bug is:

1) Take the implementation of "getStructuredFilter" in file dbaccess/source/core/api/SingleSelectQueryComposer.cxx and change it to not include the operator in the "Value" member, so that it produces the format that is expected by setStructuredFilter. This will require:

1.1) Going into the getStructuredCondition function
1.2) See what other callers getStructuredCondition has, and check if these other callers should also not get the operator in the value member (probably so...).

2) Check all callers of getStructuredFilter and remove the kludges they do to remove the operator themselves. For that, do a "git grep getStructuredFilter". There is currently , C++ code but also Java code.

3) "Optiona" but would make the work really complete: make a unittest that tests that setStructuredFilter(getStructuredFilter()) is a no-op.

Sorry I rambled in description (comment 0) about an eventual new API. Anybody wanting to propose (and implement) a new API is most welcome too, but this is beyond the scope of an EasyHack.
Comment 7 Commit Notification 2016-08-10 14:34:36 UTC
Fabio Buso committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=2c76fd1e04441889fbc416d4e5815ef31b474193

tdf#67647 getStructuredFilter returns operator

It will be available in 5.3.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.