Edgewall Software
Modify

Opened 7 years ago

Last modified 15 months ago

#10983 new enhancement

PATCH (and RFC) for IQueryPreprocessor extension point

Reported by: chris.nelson.1022@… Owned by:
Priority: normal Milestone: next-major-releases
Component: ticket system Version: 0.11.6
Severity: normal Keywords: patch query extension
Cc: leho@…, ethan.jucovy@…, Steffen Hoffmann, Ryan J Ollos Branch:
Release Notes:
API Changes:

Description

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.

Attachments (11)

IQueryPreprocessor.diff (2.9 KB ) - added by Chris.Nelson@… 7 years ago.
Patch to add IQueryPreprocessor extension point to 0.11.6
IQueryPreprocessor-interface.patch (3.0 KB ) - added by Chris.Nelson@… 7 years ago.
Patch to add IQueryPreprocessor interface to Trac 1.1.1dev
IQueryPreprocessor-example.patch (2.9 KB ) - added by Chris.Nelson@… 7 years ago.
Patch to add IQueryPreprocessor example to Trac 1.1.1dev
IQueryPreprocessor-interface.2.patch (3.0 KB ) - added by Chris.Nelson@… 7 years ago.
Patch to add IQueryPreprocessor interface to Trac 1.1.1dev
IQueryPreprocessor-example.2.patch (2.9 KB ) - added by Chris.Nelson@… 7 years ago.
Patch to add IQueryPreprocessor example to Trac 1.1.1dev
interface.patch (4.2 KB ) - added by Chris.Nelson@… 6 years ago.
Patch to add IQueryParticipant interface definition and use (applies to r11528)
sample.patch (3.3 KB ) - added by Chris.Nelson@… 6 years ago.
Patch to add IQueryParticipant sample (applies to r11528 after interface.patch)
custom.patch (6.3 KB ) - added by Chris.Nelson@… 6 years ago.
Patch to add debugging, looking for missing custom query constraints
trac.log (21.7 KB ) - added by Chris.Nelson@… 6 years ago.
trac.log from attempt to use custom constraints in query form
working.patch (11.0 KB ) - added by Chris.Nelson@… 6 years ago.
Working interface and sample plugin; can be applied to 1.1.1dev at r11528
milestone_query.py (2.5 KB ) - added by ethan.jucovy@… 6 years ago.
sample IQueryParticipant plugin to search tickets by milestone status (completed/overdue/etc)

Download all attachments as: .zip

Change History (46)

by Chris.Nelson@…, 7 years ago

Attachment: IQueryPreprocessor.diff added

Patch to add IQueryPreprocessor extension point to 0.11.6

comment:1 by lkraav <leho@…>, 7 years ago

Cc: leho@… added

comment:2 by Ethan Jucovy <ethan.jucovy@…>, 7 years ago

Cc: ethan.jucovy@… added

comment:3 by Christian Boos, 7 years ago

Milestone: undecidednext-dev-1.1.x

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…

in reply to:  3 comment:4 by Chris.Nelson@…, 7 years ago

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.

by Chris.Nelson@…, 7 years ago

Patch to add IQueryPreprocessor interface to Trac 1.1.1dev

by Chris.Nelson@…, 7 years ago

Patch to add IQueryPreprocessor example to Trac 1.1.1dev

comment:5 by Chris.Nelson@…, 7 years ago

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.

comment:6 by Chris.Nelson@…, 7 years ago

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.

in reply to:  6 ; comment:7 by Christian Boos, 7 years ago

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.

in reply to:  7 comment:8 by Chris.Nelson@…, 7 years ago

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.

comment:9 by Chris.Nelson@…, 7 years ago

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.

by Chris.Nelson@…, 7 years ago

Patch to add IQueryPreprocessor interface to Trac 1.1.1dev

by Chris.Nelson@…, 7 years ago

Patch to add IQueryPreprocessor example to Trac 1.1.1dev

in reply to:  9 comment:10 by Chris.Nelson@…, 7 years ago

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à!

comment:11 by ejucovy, 7 years ago

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.

in reply to:  11 comment:12 by Chris.Nelson@…, 7 years ago

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.

comment:13 by anonymous, 6 years ago

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.

in reply to:  13 comment:14 by Chris.Nelson@…, 6 years ago

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|....

comment:15 by Chris.Nelson@…, 6 years ago

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?

comment:16 by Steffen Hoffmann, 6 years ago

Cc: Steffen Hoffmann added

by Chris.Nelson@…, 6 years ago

Attachment: interface.patch added

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

by Chris.Nelson@…, 6 years ago

Attachment: sample.patch added

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

by Chris.Nelson@…, 6 years ago

Attachment: custom.patch added

Patch to add debugging, looking for missing custom query constraints

by Chris.Nelson@…, 6 years ago

Attachment: trac.log added

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

comment:17 by Chris.Nelson@…, 6 years ago

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.

by Chris.Nelson@…, 6 years ago

Attachment: working.patch added

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

in reply to:  11 comment:18 by Chris.Nelson@…, 6 years ago

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.

in reply to:  13 comment:19 by Chris.Nelson@…, 6 years ago

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.

comment:20 by Chris.Nelson@…, 6 years ago

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

comment:21 by Christian Boos, 6 years ago

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.

by ethan.jucovy@…, 6 years ago

Attachment: milestone_query.py added

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

comment:22 by ethan.jucovy@…, 6 years ago

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.)

in reply to:  22 comment:23 by Christian Boos, 6 years ago

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).

in reply to:  21 comment:24 by Chris.Nelson@…, 6 years ago

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?

comment:25 by Ryan J Ollos, 5 years ago

Cc: Ryan J Ollos added

in reply to:  11 ; comment:26 by Peter Suter, 5 years ago

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.

in reply to:  26 comment:27 by Chris.Nelson@…, 5 years ago

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).)

comment:28 by Ryan J Ollos, 5 years ago

Milestone: next-dev-1.1.xnext-major-releases

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.

comment:29 by Ryan J Ollos, 4 years ago

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.

comment:30 by Chris.Nelson@…, 4 years ago

What can I do to help move this ticket forward?

comment:31 by Ryan J Ollos, 4 years ago

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.

comment:32 by Peter Suter, 4 years ago

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

comment:33 by Ryan J Ollos, 4 years ago

Reporter: changed from Chris.Nelson@… to chris.nelson.1022@…

comment:34 by figaro, 4 years ago

Keywords: patch added

comment:35 by Peter Suter, 15 months ago

#2855 might be one more usecase.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as new The ticket will remain with no owner.
The ticket will be disowned. Next status will be 'new'.
as The resolution will be set. Next status will be 'closed'.
The owner will be changed from (none) to anonymous. Next status will be 'assigned'.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.