PATCH (and RFC) for IQueryPreprocessor extension point

[chris.nelson.1022@…]

I'm working in 0.11.6 and I don't expect this to be adopted there but I'd like to get feedback on structure, naming, etc. so I can make this patch more acceptable before porting to an active version.

In my PM work, I find it desirable to query tickets based in special ways like asking for all the tickets required by a ticket (and their predecessors, etc.) or all the descendants of a ticket. My plugin supports this with goal=id|id|... and root=id|id|... but every place I want this support, I have to modify plugin source to use my preprocessor or query wrapper. For example, I did this locally for Estimation Tools so I could do a workload chart of all the work in a ticket's predecessors.

By adding an extension point to the core query system, all ticket queries can benefit from the PM logic. For example, you could do [[TicketQuery(goal=self)]] in a ticket to list all the ticket's predecessors.

This patch defines and uses IQueryPreprocessor to accomplish this extensible query system. Then I implement this interface in my PM plugin:

class PMQueryHelper(Component):
    implements(IQueryPreprocessor)

    def __init__(self):
        self.pm = TracPM(self.env)


    # IQueryPreprocessor methods

    # Return the list of constraints handled by process_constraints()
    def custom_constraints(self):
        return ['root', 'goal', 'scheduled']

    # Turn custom constraints (e.g., root) into standard contraints (e.g., id)
    #
    # @param req the Trac web request object, may be None
    # @param constraints hash indexed by contraint name (e.g., 'root',
    #   'id').  Each entry is a list of values.
    #
    # @return updated constraints.
    def process_constraints(self, req, constraints):
        # constraint values are lists, TracPM.preQuery() expects
        # pipe-delimited strings.
        options = {}
        for constraint in self.custom_constraints():
            if constraint in constraints:
                options[constraint] = "|".join(constraints[constraint])
                del constraints[constraint]

        ids = list(self.pm.preQuery(options, req))

        if len(ids):
            if 'id' not in constraints:
                constraints['id'] = []
            for tid in ids:
                if tid not in constraints['id']:
                    constraints['id'].append(tid)
        
        return constraints

and the above TicketQuery() works.

[Chris.Nelson@…]

Patch to add IQueryPreprocessor extension point to 0.11.6

[Christian Boos]

I suppose one would have to be careful when translating custom constraints to core constraints, as the "id" one probably doesn't scale that well (btw, we should at least deal with duplicates numbers there if we don't do it already, maybe even convert to ranges when possible).

Other than that, I couldn't think of obvious problems with this approach, so maybe we should just give it a try in 1.1.1dev see how it fares ;-) PatchWelcome on trunk.

Also, maybe you should use an OrderedExtensionOption, so that the admin is in control if one IQueryPreprocessor implementation depends on another…

[Chris.Nelson@…]

Replying to cboos:

I suppose one would have to be careful when translating custom constraints to core constraints, as the "id" one probably doesn't scale that well (btw, we should at least deal with duplicates numbers there if we don't do it already, maybe even convert to ranges when possible).

Note that my example does:

if len(ids):
    if 'id' not in constraints:
        constraints['id'] = []
    for tid in ids:
        if tid not in constraints['id']:
            constraints['id'].append(tid)

so it doesn't add duplicates.

Other than that, I couldn't think of obvious problems with this approach, so maybe we should just give it a try in 1.1.1dev see how it fares ;-) PatchWelcome on trunk.

Now I have to get an environment where I can work on the trunk.

Also, maybe you should use an OrderedExtensionOption, so that the admin is in control if one IQueryPreprocessor implementation depends on another…

That make sense. I haven't tried one of those but I'll look into it.

[Chris.Nelson@…]

Patch to add IQueryPreprocessor interface to Trac 1.1.1dev

[Chris.Nelson@…]

Patch to add IQueryPreprocessor example to Trac 1.1.1dev

[Chris.Nelson@…]

The patches I just attached work on 1.1.1dev when last I fetched it (git-svn isn't working for me today, I can't refresh my local repo).

With these patches applied (and the sample built and installed), you can do [[TicketQuery(foo=1)]] and get the same results as [[TicketQuery(id=1)]]. (There is more explanation in the example.)

This is an ordered extension option controlled by preprocessors in the[query] section of trac.ini. However, even without that configuration, this works. I don't quite know why.

[Chris.Nelson@…]

I noticed a strange change in the self.constraints field of the query() class. In 0.11.6, it was a hash of constraints like {'id': [1, 2, 3], 'status': ["closed"]} In 1.1.1, it is a one-item list containing that hash (that is, self.env.log.debug('constraints:%s' % self.constraints) writes [{'id': [1, 2, 3], 'status': ["closed"]}] to the log. I don't understand that. The example accounts for this strange data by doing:

constraints = constraints[0]
# ... process the hash ...
return [ constraints ]

but that's awkward.

[Christian Boos]

Replying to Chris.Nelson@…:

… In 0.11.6, it was a hash of constraints like {'id': [1, 2, 3], 'status': ["closed"]} In 1.1.1, it is a one-item list containing that hash

Isn't that the support for or, which incidentally was just discussed minutes ago? See #2647.

[Chris.Nelson@…]

Replying to cboos:

Replying to Chris.Nelson@…:

… In 0.11.6, it was a hash of constraints like {'id': [1, 2, 3], 'status': ["closed"]} In 1.1.1, it is a one-item list containing that hash

Isn't that the support for or, which incidentally was just discussed minutes ago? See #2647.

Not sure where it was discussed but I can buy that each entry in the list is a different query and that the results are or-ed together.

That means my sample needs to iterate over the elements rather than processing just the first. That's not hard. The core code should stand, however.

[Chris.Nelson@…]

Reworked this a lot. Added a bunch of tracing to make sure all my code was executing and doing what I intended. Turns out it hadn't been. The previous patch seemed to work by an accident of my limited test data in a newly-installed 1.0 system. What I'm about to upload is better.

If I leave off the include_missing=False for loading the extension points, Query finds the sample and executes it and I get the expected results. If I include that, I cannot figure out what to put in trac.ini to configure the list of query preprocessors. Specifically,

[query]
preprocessors = SampleQueryPreprocessor

does not work. I cannot figure out the magic incantation and cannot find an example of a working ordered extension point option.

[Chris.Nelson@…]

Patch to add IQueryPreprocessor interface to Trac 1.1.1dev

[Chris.Nelson@…]

Patch to add IQueryPreprocessor example to Trac 1.1.1dev

[Chris.Nelson@…]

Replying to Chris.Nelson@…:

…Specifically,

[query]
preprocessors = SampleQueryPreprocessor

does not work. I cannot figure out the magic incantation and cannot find an example of a working ordered extension point option.

The names in the option configuration are class names. Since I have:

class SampleQueryHelper(Component):
    implements(IQueryPreprocessor)

the answer is:

[query]
preprocessors = SampleQueryHelper

Voilà!

[ejucovy]

This looks like it would be very useful. Do you run a separate database query to translate your custom constraints into ids? If that's possible then this interface could also be implemented by plugins like ​th:wiki:TracHoursPlugin or ​th:wiki:VotePlugin to provide search options for tickets with e.g. at least N votes or hours worked.

It would be nice if there was also a way for these components to supply additional options to the custom query building interface. If I'm understanding correctly, this patch doesn't add that feature; I also don't see any existing hook in Trac for this short of injecting it into the genshi stream.

In the long run it might also be useful for a plugin to be able to inject SQL directly into the constructed query clauses, rather than having to translate its constraints into known fields. That looks like it would be very complicated though.

But, I wonder if it might make sense for this interface to be called something like IQueryParticipant, and eventually grow those additional features without stretching the meaning of its name.

[Chris.Nelson@…]

Replying to ejucovy:

This looks like it would be very useful. Do you run a separate database query to translate your custom constraints into ids? If that's possible then this interface could also be implemented by plugins like ​th:wiki:TracHoursPlugin or ​th:wiki:VotePlugin to provide search options for tickets with e.g. at least N votes or hours worked.

Yes. My actual use of this in TracPM queries tables to produce the new IDs.

It would be nice if there was also a way for these components to supply additional options to the custom query building interface. If I'm understanding correctly, this patch doesn't add that feature; I also don't see any existing hook in Trac for this short of injecting it into the genshi stream.

I'm working on a refinement that allows the plugin to provide enough information so that the custom query form can show something like Parent or whatever. I got buried in a project I get paid for before I could finish but hope to get back to it in a week or so.

In the long run it might also be useful for a plugin to be able to inject SQL directly into the constructed query clauses, rather than having to translate its constraints into known fields. That looks like it would be very complicated though.

But, I wonder if it might make sense for this interface to be called something like IQueryParticipant, and eventually grow those additional features without stretching the meaning of its name.

I'm open to naming suggestions. That seems like a fine one.

[anonymous]

Would this allow writing a plugin that supports milestone status queries? (E.g. query only tickets in:

• completed milestones.
• open milestones.
• open milestones that have a due date set.
• missed milestones.
• …)

That would be very useful to me.

[Chris.Nelson@…]

Replying to anonymous:

Would this allow writing a plugin that supports milestone status queries? (E.g. query only tickets in:

• completed milestones.
• open milestones.
• open milestones that have a due date set.
• missed milestones.
• …)

That would be very useful to me.

Yes, I imagine it would. You'd make up your own keywords (e.g., milestonestatus=closed) and return something like milestone=first closed|second closed|....

[Chris.Nelson@…]

I have a more complete example which starts to support augmenting the query form. I get the plugin's fields in the and and or drop downs and they add items in the Filters frame but when I click Update, the plugin fields have no effect and they disappear from the Filters section.

I've added code to process_request in trac/ticket/query.py but even at the beginning of that function, my plugin fields are already missing. I'm guessing there's magic in the query template or query.js but I can't find it. What gets executed when I click Update?

[Chris.Nelson@…]

Patch to add IQueryParticipant interface definition and use (applies to r11528)

[Chris.Nelson@…]

Patch to add IQueryParticipant sample (applies to r11528 after interface.patch)

[Chris.Nelson@…]

Patch to add debugging, looking for missing custom query constraints

[Chris.Nelson@…]

trac.log from attempt to use custom constraints in query form

[Chris.Nelson@…]

After applying interface.patch and sample.patch to r11528, I can do:

[[TicketQuery(foo-id=1,id=2)]]

and see tickets 1 and 2 in the output.

After applying custom.patch, I can go to /query and my custom constraints show up. I can pick one and it gets added to the filter list. When I enter a value and click "Update", the args listed in process_request() lists the custom constraint (see attached trac.log) but it does not affect the query output and when /query refreshes, the filter item for the custom constraint is gone.

[Chris.Nelson@…]

Working interface and sample plugin; can be applied to 1.1.1dev at r11528

[Chris.Nelson@…]

Replying to ejucovy:

… It would be nice if there was also a way for these components to supply additional options to the custom query building interface. If I'm understanding correctly, this patch doesn't add that feature; I also don't see any existing hook in Trac for this short of injecting it into the genshi stream.

This is done now.

In the long run it might also be useful for a plugin to be able to inject SQL directly into the constructed query clauses, rather than having to translate its constraints into known fields. That looks like it would be very complicated though.

But, I wonder if it might make sense for this interface to be called something like IQueryParticipant, and eventually grow those additional features without stretching the meaning of its name.

I liked that name and ended up using it.

[Chris.Nelson@…]

Replying to anonymous:

Would this allow writing a plugin that supports milestone status queries? (E.g. query only tickets in:

• completed milestones.
• open milestones.
• open milestones that have a due date set.
• missed milestones.
• …)

That would be very useful to me.

This should be fairly to implement by adapting the sample in the patch. You'd have the custom constraints as something like:

{
'ms-status': {
    'col': 'milestone',
    'prop': {
        'type': 'select',
        'label': "Milestone status',
        'name': 'ms-status',
        'type': 'select',
        'options': ['completed', 'open', 'due', 'late']
    }
}

and then process_constraints() would have suitable SQL to set or update milestone.

[Chris.Nelson@…]

Has anyone CC'd on this ticket tried the latest patch? Has anyone responsible for core considered merging it into the next dev release?

[Christian Boos]

Not yet tried. But last time I looked, I was thinking that implementing comment:19 as an example would be more useful than the current examples (with foo as synonym for id). Also, sample plugins should be self-contained 1 file plugins.

[ethan.jucovy@…]

sample IQueryParticipant plugin to search tickets by milestone status (completed/overdue/etc)

[ethan.jucovy@…]

I tried this patch out on a trac site by writing a sample "milestone status" plugin as described in comment:19, in attachment:milestone_query.py​ It's working very nicely for me so far. (This sample plugin could end up being inefficient if you have a very large number of milestones — it just selects all milestones from the database once per query clause and then iterates over them multiple times per query clause. That could certainly be improved, but I was testing on a trac site with only ~20 milestones so I didn't really need to worry about efficiency.)

[Christian Boos]

Replying to ethan.jucovy@…:

I tried this patch out on a trac site by writing a sample "milestone status" plugin as described in comment:19, in attachment:milestone_query.py​ It's working very nicely for me so far.

Good to hear!

(This sample plugin could end up being inefficient if you have a very large number of milestones — it just selects all milestones from the database once per query clause and then iterates over them multiple times per query clause. That could certainly be improved, but I was testing on a trac site with only ~20 milestones so I didn't really need to worry about efficiency.)

Nevertheless you may be able to improve upon that quite easily by using the milestone cache introduced in 1.0.1 (see #10879).

[Chris.Nelson@…]

Replying to cboos:

Not yet tried. But last time I looked, I was thinking that implementing comment:19 as an example would be more useful than the current examples (with foo as synonym for id).

I see your point but the existing sample illustrates more features.

Also, sample plugins should be self-contained 1 file plugins.

It is *implemented* in one file, it just has suitable infrastructure to make it easy to use. Is only one file a strict requirement when it is isolated into its own subdirectory?

[Peter Suter]

In the long run it might also be useful for a plugin to be able to inject SQL directly into the constructed query clauses, rather than having to translate its constraints into known fields.

This also sounds preferable to me. What are the upsides of the "translation" approach?

I also think the useful MilestoneQueryParticipant sample plugin in a single file would be a better fit. Explain more features etc. in TracDev/PluginDevelopment/ExtensionPoints/trac.ticket.api.IQueryParticipant instead.

[Chris.Nelson@…]

Replying to psuter:

In the long run it might also be useful for a plugin to be able to inject SQL directly into the constructed query clauses, rather than having to translate its constraints into known fields.

This also sounds preferable to me. What are the upsides of the "translation" approach?

It's already coded. ;-) I wouldn’t know how to inject SQL.

I also think the useful MilestoneQueryParticipant sample plugin in a single file would be a better fit. Explain more features etc. in TracDev/PluginDevelopment/ExtensionPoints/trac.ticket.api.IQueryParticipant instead.

Sure, I can update the documents if this gets merged.

(If I may editorialize for a moment, I find the single-file "sample" plugins frustrating; Granted that these may be intended to illustrate how to implement various APIs but there needs to be at least one that shows how to build a non-trivial plugin that takes multiple files. I always fail and iterate to get a new plugin working. A multi-file "Hello, world" example would be a godsend so you had less functionality to cut out to start building your own. And the various things that need to match for the plugin to work need to be distinct (thePluginsModuleName and thePluginsNameInWebAdmin, etc., not ThePlugin and thePlugin and theplugin).)

[Ryan J Ollos]

Retargetting tickets to narrow focus for milestone:1.2. Please move the ticket back to milestone:next-dev-1.1.x if you intend to resolve it by milestone:1.2.

[Ryan J Ollos]

This ticket was discussed in ​trac-dev:09KHkId-deE/ibH2mJCAdD0J for inclusion in milestone:1.2. I'll try to revisit and review before then.

[Chris.Nelson@…]

What can I do to help move this ticket forward?

[Ryan J Ollos]

I'm not sure I'll have time to work this ticket prior to milestone:1.2, but rebasing the patch on the latest trunk would be a good start. We'll also need unit tests for the patch.

[Peter Suter]

#10556 seems to be another vote for the MilestoneQueryParticipant. #10286 might be another possible use case.

[Peter Suter]

#2855 might be one more usecase.