Item10520: QuerySearch for \"SomeText\" ONLY refers to a FieldName, and no longer also a FormName.
Priority: Enhancement
Current State: Closed
Released In: 2.0.0
Target Release: major
Applies To: Engine
Component: SEARCH
Branches:
QuerySearch is quite explicit about
SEARCH{"Sometext" type=query
, in that it says this
refers to the field named X, with an example of
LastName
however,
Fn_SEARCH::verify_formQuery2
has a test that
succeeds despite there being no field of that name:
The test succeeds on a topic that contains a form of that name, but this does not gel with the docco, and is really really hard to hoist to, as it means that a scalar string could refer to either a field name, or a form name.
I would prefer to stick with what is doccoed - but clearly need feedback on that. (oh, and some docco in the evaluate methods that might shed light on the issue.
trunkwork atm.
--
SvenDowideit - 22 Mar 2011
Fix it to align with the docco... sounds way too sticky otherwise. Unless there's a good reason not to (I don't know why you'd search for form usage this way).
In fact I don't think I've seen a "Blah" type="query" at all (but I only have 3 users that have ever written
QuerySearch, so...
--
PaulHarvey - 22 Mar 2011
of course, I would also rather the parser would turn this into a real query, rather than leaving us hanging on another exception case.
--
SvenDowideit - 22 Mar 2011
It
is a real query, roughly equivalent to
form.name='TestForm'
. Note that you can see this using %QUERY{"ItemTemplate"}% -> ==
To explain. All calls to
evaluate
return a result which is one of an array, a hash, a scalar, or undef. A form name used in isolation is spotted by
getField
, which maps it to an array of the fields in the form. This is context dependent behaviour, because
getField
depends on being able to find out the name of the form in the queried topic. In this case,
TestForm
is resolved by
getField
to an array of form fields, which is non-null and therefore true. Unfortunately the parser cannot rewrite this query because the context information is only available when evaluating the query against a specific topic, so it has no way to know if it is a form name, or a field name.
IMHO making the code fit the docco is not an option (not only is it complex to do in the code, it is inconsistent and potentially very confusing to have form names as "second class citizens" that sometimes resolve, and sometimes do not, and it also does nothing to resolve the context dependency issue). Because of the problem in resolving the context dependency during hoisting, one alternative is to entirely remove support for the form name. This is a major change that might potentially break end user applications, but without it I can't see how we can ever support full hoisting.
--
CrawfordCurrie - 06 Apr 2011
Dropping support for
FooForm.field accessors.. Hmm. Perhaps there's another way. Could we geek up the syntax a little...
-
MyForm:
- referring to form.name='MyForm'
-
MyForm:MyField
- referring to fields[name='MyField' AND form='MyForm'].value
I'm quite keen to get multi dataforms support into Foswiki...
--
PaulHarvey - 07 Apr 2011
Geeking up the syntax is one option, though it's kinda like a logical next step after breaking everything as I described.
I'm going to try to write a BNF for the query language that resolves the context of a name in such a way as it is unambiguous whether it is a form name or a field name.
--
CrawfordCurrie - 07 Apr 2011
Cool!
--
PaulHarvey - 07 Apr 2011
as discussed with Crawford on IRC
It
is not a real query, in that
SEARCH{"Sometext" type=query
is roughly equivalent to
form.name='Sometext' OR field.name='Sometext'
.
and that in many ways, its way too vague a 'statement' to really be allowed to be this way.
its error prone, as the user might have meant a word or regex search - without any logic, its vauge, and its not a query for a single thing - what about a form that has a field of the same name??
additionally, it makes it even harder for users to grok - as the
QUERY
macro does not in fact return what
SEARCH is evaluating.
and lastly - this kind of vaguery makes it much more difficult for search backends to generate queries.
imo there are (again) 2 issues -
- the query language needs to be simple and unambiguous, and attempts need to be made to recognise fat-finger-errors
- the output parse tree should be simplified and rigourously defined, documented and tested, to ensure that writers of backends have a snowballs hope.
wrt multi-forms - eeeek
--
SvenDowideit - 07 Apr 2011
Arguing about what constitutes a "real query" isn't helpful, so I'm not going to.
After writing a BNF and giving it some serious thought, I have tightened up the rules regarding where a name can be interpreted as a form name, and where it must be a field (or subscript). This has led to pulling a bunch of functionality back into Node.pm from QueryAlgorithm.pm, and adding a
getForm
method to QueryAlgorithm. This way the query algorithm is in no doubt as to whether it is being asked for a form or a field.
--
CrawfordCurrie - 07 Apr 2011
well. humpf.
your commit has broken all the hoisters, including the hoistRE one - trunk.f.o is broken.
its a pain that your commit isn't just a fix for this task, but also a much bigger change to the parse tree from binary to n-ary (as that is what has broken the hoisters).
and an additional pain that i was thinking about doing this too, but didn't want to break everyones work without talking about it first.
given that the
HoistRE tests are failing, and that trunk is broken, this piggback isn't complete yet - please use
Item10612: make OP_and, OP_or and OP_comma parse tree nodes n-ary and commit those things to it.
(yes, I've been spending time getting mongodb to work with your deep modification, so i'm not going to revert it.)
--
SvenDowideit - 08 Apr 2011
I disabled folding in this checkin; will check it in under
Item10612 when I'm sure the hoisting works with it.
--
CrawfordCurrie - 08 Apr 2011
Calling this closed
--
SvenDowideit - 07 Jun 2011