476722 - Refactor Search.pm's funcdefs into a series of constants

Status: RESOLVED FIXED

(Bugzilla :: Query/Bug List, enhancement, P1)

Description

[Max Kanat-Alexander]

Right now it's very difficult to extend funcdefs, and it matches using a regular expression with a comma, which is a problem.

Instead, we should have two constants. One constant contains all search operators by themselves, and points to subs that describe the default behavior for those search operators.

The second constant describes overrides for certain fields. That is, it overrides the constant above, for those fields.

It would look something like this:

use constant OPERATORS => {
    equals         => \&_equals,
    allwordssubstr => \&_allwordssubstr,
    (etc.)
};

use constant OPERATOR_OVERRIDE => {
    assigned_to => { 
        notequals => \&_something,
    }
};

And then a function like:

sub function_for {
   my ($self, $field, $operator) = @_;
}

That returns the right function. function_for can be hooked, and then extensions can easily modify Search.pm.

Comment 1

[Michael Leupold]

I've been looking at this issue a bit and came up with the following ideas:
- add a NORMALIZED_FIELDS that maps fields like long_desc which are kept for backwards-compatibility to a unique internal fieldname so we don't need that many special cases.
- @funcdefs currently also lists functions for joining with additional tables. Adding JOIN_FIELDS to map normalized field names to functions which join those tables (like _long_desc or _attachments) is likely to reduce OPERATORS_OVERRIDE by a lot and making search functionality easier to maintain.

Comment 2

[Max Kanat-Alexander]

(In reply to comment #1)
> - add a NORMALIZED_FIELDS that maps fields like long_desc which are kept for
> backwards-compatibility to a unique internal fieldname so we don't need that
> many special cases.

  Good idea! We can call it FIELD_MAP, since that's what we call it in other places (like the WebService).

> - @funcdefs currently also lists functions for joining with additional tables.
> Adding JOIN_FIELDS to map normalized field names to functions which join those
> tables (like _long_desc or _attachments) is likely to reduce OPERATORS_OVERRIDE
> by a lot and making search functionality easier to maintain.

  Oh, that sounds like it could be a good idea too! In any case, all of these constants should be used through a method on $self that can be hooked. (So you'd have $self->map_field, $self->do_join, etc. probably.)

Comment 3

[Max Kanat-Alexander]

Attachment: Work In Progress

Here's what I have so far. This shows the data structures for OPERATORS and OPERATOR_FIELD_OVERRIDE. I still have to decide what I'm going to do about the value-based overrides, though. The ones that we have are so specific that I'm not sure if I'm going to do a constant for them, or what.

Comment 4

[Max Kanat-Alexander]

Attachment: v1

Okay, here we go. I have tested this EXTENSIVELY. I tested every search function that I modified, and every search function that has an override.

This changes @funcdefs into a series of constants, and has a function go use those constants in a logical manner (instead of just running a bunch of regexes with commas in them).

I have also started to use a few new variables names in a few places--ones that are clearer than "f", "ff", "t", and "v".

I discovered that $f and $ff were getting mixed up--$ff is supposed to be what goes into the SQL, while $f is supposed to remain the name of the field from fielddefs. This wasn't affecting us before this refactor, but it started to when I refactored, so I fixed it now.

I refactored the code that handles pronouns so that all of the non-group pronoun stuff is handled directly by a single function for each type of field (as you'll see if you see what I removed and replaced with functions with "pronoun" in the name).

Every use of %funcsbykey within search functions was actually just a way of getting the default function for a particular search type (which I now call an "operator"). So, I replaced that functionality with a function called _do_operator_function.

_attach_data_thedata_changed seems to no longer be necessary with the new structure, so I have removed it.

I now for the first time think that I actually fully understand how the search functions in Search.pm work. :-)

I'm requesting a general review from jjclark, because I think he's the only person around and available who understands this code enough to review it. However, I'd take review from justdave or joel if they want to review it, also.

It's important that this get reviewed relatively quickly, because it touches a lot of Search.pm, it's very complex, and I don't want it to bitrot.

Comment 5

[Jesse Clark]

Comment on attachment 444652 [details] [diff] [review]
v1

This is a good encapsulation of the current search function determination logic, without all those confusing regular expressions. When I attempted this I tried to model the "changed" control flow differently and couldn't express it with a reasonable topology.

I have successfully added new search functions by adding field names to OPERATOR_FIELD_OVERRIDE and by adding logic to do_search_function(). I can imagine easily adding a hook to the former but not the latter. This fits with my conception of extensions: in PRACA we use them when we need to add custom logic for an instance-specific field, not to apply to whole categories of fields.

Seeing all these sweeping changes to search functions, I think that it would be nice to change their interface: to return SQL instead of setting $$term, and in general to set attributes of $self directly, instead of going through pointers in %func_args. But that should probably be a separate bug.

Comment 6

[Max Kanat-Alexander]

Thank you!! :-)

Yeah, I agree that the whole method of operation here should be changed. We'll do it in a separate bug. At first, I'm going to have things just update $func_args instead of $$some_var.

Comment 7

[Max Kanat-Alexander]

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/trunk/
modified Bugzilla/Search.pm
Committed revision 7209.

Comment 8

[Max Kanat-Alexander]

Added to the release notes in bug 604256.