Item10703: something broke the query parser

Running QueryTests
*** Running 2 tests matching your pattern (verify_meta_dot)
   QueryTests::verify_meta_dot_BruteForceQuery
Parse: 'AnotherTopic'/META:TOPICINFO.date
Operand: qs 'AnotherTopic'
Operator: Foswiki::Query::OP_ref=HASH(0x41d8c10)
Operand: word 'META:TOPICINFO'
Operator: Foswiki::Query::OP_dot=HASH(0x4229ac8)
Apply /('AnotherTopic', META:TOPICINFO)
Operand: word 'date'
Apply .(/{'AnotherTopic',META:TOPICINFO}, date)
Return .{/{'AnotherTopic',META:TOPICINFO},date}
Parse: 'AnotherTopic'/META:TOPICINFO.date
Operand: qs 'AnotherTopic'
Operator: Foswiki::Query::OP_ref=HASH(0x42223d8)
Operand: word 'META:TOPICINFO'
Operator: Foswiki::Query::OP_dot=HASH(0x420c440)
Apply /('AnotherTopic', META:TOPICINFO)
Operand: word 'date'
Apply .(/{'AnotherTopic',META:TOPICINFO}, date)
Return .{/{'AnotherTopic',META:TOPICINFO},date}
hoistEQ  .{'HASH(0x421c3b0)',date}   FAILED
hoistAND  .{'HASH(0x421c3b0)',date}  FAILED

there seem to be a number of places where the output has changed, but we don't have unit tests for the parse tree

-- SvenDowideit - 04 May 2011

this kills the MongoDB hoister even worse.

-- SvenDowideit - 04 May 2011

tbh, the log above looks reasonable to me, the problem is something changed, and i've not worked out what yet.

-- SvenDowideit - 04 May 2011

ok, a possible lead

$node->stringify() outputs:

HoistMongoDB::hoist from: and{not{({or{={name,'WebPreferences'},={name,'TestTopicSEARCH'},={name,'AnotherTopic'}}}},.{'HASH(0x467b458)',date}}

so it doesn't seem to expect a hash there either.

-- SvenDowideit - 04 May 2011

erm, ok, Crawford, this feels somewhat serious to me:

I added the ASSERT below - something (i think recently) has changed so that the parse tree contains a non-STRING where it used to be a STRING, and the 'type' hasn't changed - or of course, something that didn't used to be marked as a STRING, because its not, is now marked as one..

=begin TML

---++ ObjectMethod stringify() -> $string

Generate a string representation of the subtree, for reporting. The representation
generated by this function should be parseable, but is not guaranteed to be.


=cut

sub stringify {
    my $this = shift;

    unless ( ref( $this->{op} ) ) {
        if ( $this->{op} == STRING ) {
            ASSERT(ref($this->{params}[0]) eq '') if DEBUG;
            return "'$this->{params}[0]'";
        }
        else {
            return $this->{params}[0];
        }
    }
    if ( $this->{op}->{arity} ) {
        use Foswiki::Query::Node;
        return
          $this->{op}->{name} . '{'
          . join( ',', map { 
                        ref($_).' - '.stringify($_) 
                     } @{ $this->{params} } ) . '}';
    }
    else {
        $this->{op}->{name};
    }
}

-- SvenDowideit - 04 May 2011

and then i turn off evaluatesToConstant and the tests succeed again

-- SvenDowideit - 04 May 2011

ah

um, evaluatesToConstant modifies the parse tree, even when the QUERY isn't a boolean (and so doesn't evaluate to a Constant in the end), and in the process is breaking the tree.

-- SvenDowideit - 04 May 2011

so the root cause of this, is that Node::simplify() converted an evaluatesToConstant to a STRING element without bothering to test if that element was indeed a scalar - resulting in a Node that was marked as a STRING, but was in fact a HASH.

this was masked by the REHoister simply failing as expected, but in a different place in the code.

After fixing that, I also found another issue, where BruteForce was simplifying the tree with no context - which causes OP_ref to fail, and then there was something else .

fundamentally, without a set of tests ensuring that the parse tree doesn't change unintentionally, we're working in a skyscraper built on sand.

-- SvenDowideit - 04 May 2011

Is this trunk only?

You did not set the TargetRelease so it looks like a 1.1.4 release blocker right now.

-- KennethLavrsen - 08 May 2011

I've set TargetRelease to minor. Sven, please fix it if I'm wrong (the trees coming out of the QuerySearch parser on 1.1 shouldn't be changing).

-- PaulHarvey - 08 May 2011

yup, definitely just trunk - we're not going anywhere near the released parse tree unless there's a serious user facing issue.

-- SvenDowideit - 08 May 2011

QueryTests::test_match_lc_field also now fails and works if i turn off the simplifier.

it goes from

query: 'TemporarySEARCHTestWebSEARCH.HitTopic'/fields

before simplification: /{'TemporarySEARCHTestWebSEARCH.HitTopic',fields}

after simplification: ,{,{},,{,{},,{,{},,{,{},,{,{},,{,{},,{}}}}}}}

the broken parse tree is because fields is the topics META::FIELDS array, not and array of OP nodeable things

Node::_freeze seems to think it knows what everything is, and gets it wrong, and because its done inplace, you can't even unroll if you detect an issue midway

in this case,

-- SvenDowideit - 12 May 2011

I have a change that I might commit just to get Crawford to see.

-- SvenDowideit - 12 May 2011

Sven, you said you had a patch for this. Or are you waiting for me? The report is in state New, but appears to have been analysed? Confused.

-- CrawfordCurrie - 05 Jul 2011

I commited the changes and hoped you'd look at what i did in case i misunderstood.

given the time passing, closing as 'ok'

-- SvenDowideit - 06 Jul 2011

Commenting is disabled while not logged in