Modify

Opened 13 years ago

Closed 13 years ago

Last modified 13 years ago

#8643 closed defect (fixed)

Cannot submit ticket in Mylyn

Reported by: wrobel@… Owned by: Mark Ryan
Priority: normal Component: ChildTicketsPlugin
Severity: blocker Keywords:
Cc: Trac Release: 0.12

Description

When submitting a new ticket from Mylyn for Eclipse I got the following error: "'Environment' object has no attribute 'childtickets' while executing ticket.update()"

This happens even when the ticket has an empty "Parent ID" field. I am not sure whether it is a Mylyn or ChildTickets issue. Perhaps the latter since the problem does not happen with other Trac plugins.

Attachments (0)

Change History (16)

comment:1 Changed 13 years ago by osimons

You better paste the full traceback for the error. I'd like to see the stack involved when this error occurs. Mylyn uses XmlRpcPlugin and not the regular web interface, so the incoming request will follow another path internally than what you get when you perform same action via web.

I maintain the RPC interface, but don't know anything about ChildTicketsPlugin.

comment:2 Changed 13 years ago by Mark Ryan

Status: newassigned

Hi guys

I've not used Mylyn nor the the XmlRpcPlugin but if the latter does not use the regular web interface then the ChildTicketsPlugin will fail to initialise the 'childtickets/parenttickets' dictionaries used subsequently throughout the plugin. The plugin implements the 'IRequestFilter' and uses the 'pre_process_request()' as a filter to check it's dealing with a ticket type request ('ticket' or 'newticket') via the 'req.path_info' attribute.

@osimons : does the XmlRpcPlugin call the 'IRequestFilter' methods at all?

If it does, then a fix would be to change the first few lines of the file 'childtickets.py' from:

    # IRequestFilter methods
    def pre_process_request(self, req, handler):

        # Get ticket relationships before processing anything.
        if req.path_info[0:8] == '/ticket/' or req.path_info[0:10] == '/newticket':
            cursor = self.env.get_db_cnx().cursor()
            cursor.execute("SELECT ticket,value FROM ticket_custom WHERE name='parent'")

            # Create two dicts for later use:
            self.env.childtickets = {} # { parent -> children } - 1:n
            self.env.parenttickets = {} # { child -> parent }   - 1:1

            for child,parent in cursor.fetchall():
                if parent and re.match('#\d+',parent):
                    parent = int(parent.lstrip('#'))
                    self.env.childtickets.setdefault(parent,[]).append(child)
                    self.env.parenttickets[child] = parent
        return handler

To:

    # IRequestFilter methods
    def pre_process_request(self, req, handler):

        # Get ticket relationships before processing anything.
        cursor = self.env.get_db_cnx().cursor()
        cursor.execute("SELECT ticket,value FROM ticket_custom WHERE name='parent'")

        # Create two dicts for later use:
        self.env.childtickets = {} # { parent -> children } - 1:n
        self.env.parenttickets = {} # { child -> parent }   - 1:1

        for child,parent in cursor.fetchall():
            if parent and re.match('#\d+',parent):
                parent = int(parent.lstrip('#'))
                self.env.childtickets.setdefault(parent,[]).append(child)
                self.env.parenttickets[child] = parent
    return handler

This has the BIG disadvantage that it will incur the overhead of the db call every time a request is made to the server (ie. also for wiki pages, roadmap pages, etc...) but it will be a start to see if this is where the problem lies.

If the XmlRpcPlugin does NOT call the 'IRequestFilter' methods then the 'childtickets/parenttickets' values will have to be defined somewhere else...... I put this call here in the plugin as I had the idea that this information would be used later on by other macros and plugins (and would therefore be extended for wiki and roadmap pages, etc....)

Please have a go with the above changes and let me know how you get on.

Regards Mark (aka. Walnut!)

comment:3 Changed 13 years ago by osimons

Yes, the incoming request is handled by Trac and regular request dispatching. You may still restrict of course, but you would be better to then check for handler. To avoid trying to import RPC that may not exist, perhaps checking by name is sufficient:

if handler and handler.__class__.__name__ in ['TicketModule', `RPCWeb`]:
    # should be the components of interest
    # ... as before

Anyway, depending on pre_process_request() to initialize the structures seems like a bad idea to me - not to mention the fact that it seems like those structures are re-created on every request? Is that because information may have been updated since last request?

Here is how I would have done it:

  1. Define an __init__(self) method on your plugin / component that loads the initial structures when your plugin is loaded.
  2. Have your component subscribe to ITicketChangeListener and recreate the cache whenever new tickets are added, updated or deleted.

That way the cache should always be up-to-date on new requests, and it does not matter at all how those requests are made. Even via RPC I take care of calling change-listeners, so hooking onto that event mechanism is how you should do it.

comment:4 in reply to:  3 Changed 13 years ago by Mark Ryan

Replying to osimons:

Here is how I would have done it:

  1. Define an __init__(self) method on your plugin / component that loads the initial structures when your plugin is loaded.
  2. Have your component subscribe to ITicketChangeListener and recreate the cache whenever new tickets are added, updated or deleted.

....done the old way as that's how I first wrote it back for trac 0.10 and had no reason to change it since. (ie. Lazy!)

I like your suggestion above and will take a look at changing the approach (especially if it makes it more compatible with other plugins). Thanks for the feedback - I'll get onto looking at the changes asap.

Thx Mark

comment:5 Changed 13 years ago by osimons

Right, my alternative would also require db caching seeing multiple processes would not be able to detect changes that happen in individual processes.

comment:6 Changed 13 years ago by Mark Ryan

(In [10069]) see #8643:

  • Release 2.2.0 (trac 0.12)
  • Changed the way the child/parent ticket information is cached in the plugin.
  • Now has a 'proper' constructor and implements iTicketChangeListener to update when tickets are changed.
  • Should now provide better support for non html access (eg. XmlRpcPlugin)

comment:7 Changed 13 years ago by Mark Ryan

Hi guys - could you see if this now works in Mylyn and give feedback (I'll merge into '0.11' branch if it looks OK.)

@osimons - Thx for the idea to use a constructor and cache the results - hopefully will provide a better integration for other tools, etc... Thankyou!

Regards Mark

comment:8 Changed 13 years ago by osimons

Caching works if only one process runs Trac, but won't work if multiple processes run at web server. As mentioned you need to use db to communicate need for updaring across processes - ie. if one process gets the change and updates via listener, the next request hitting some other process will not have the same map seeing it never got notified.

You need either a marker in db, or reading the most recent change timestamp and comparing if refresh is neeed, or for a brute-force method you can call self.config.touch() which makes the env reload itself in all processes - including a new plugin that loads a fresh cache. First though I'd look into the 0.12 cachemanager as it is made for just this very purpose of caching data across requests and processes. Deposit the data with the cache manager instead, and using the listener to invalidate the cache?

Sorry, hope I'm not complicating matters trying to be helpful :-)

comment:9 Changed 13 years ago by Mark Ryan

(In [10077])

See #8643:

  • Reverted changes made in [10069] as it looks more complicated than I thought.
  • Thanks to 'osimons' for the feedback and pointer in the right direction.
  • Will have a re-think and try and get a better fix in place asap....

comment:10 Changed 13 years ago by Mark Ryan

(In [10110]) Release 2.4.0-BETA

See #8643

Hi guys

I've created this beta release with some changes that 'should' ensure the childtickets data is now correctly cached and synced using the trac 0.12 'cached' decorator. I don't have much of a test env here at home to see if this 'really' works but as far as I can tell it's regenerating the cache when required (ticket add, mod, delete) and the decorator should ensure it is being done so across all threads.

I'd appreciate it if you could try it out and see if it also works with the Mylyn plugins.

Many Thx Mark.

comment:11 Changed 13 years ago by Mark Ryan

Did anyone manage to see if this version works with Mylyn plugin?

comment:12 Changed 13 years ago by wrobel@…

Hi, I have installed it and up to now no errors :)

comment:13 Changed 13 years ago by wrobel@…

It would be nice if you also managed to integrate with Mylyn subtasks, but perhaps it is also an XML_RPC issue?

comment:14 in reply to:  13 Changed 13 years ago by anonymous

Replying to wrobel@wsb-nlu.edu.pl:

It would be nice if you also managed to integrate with Mylyn subtasks, but perhaps it is also an XML_RPC issue?

Already been requested in #8603. I don't know the Mylyn plugin for Eclipse myself but will try and get around to seeing how it works sometime soon!

comment:15 Changed 13 years ago by anonymous

Resolution: fixed
Status: assignedclosed

Closing ticket after hearing no negative feedback to recent changes which should resolve this issue (at least for 0.12).

Please re-open if there are further issues.

comment:16 in reply to:  15 Changed 13 years ago by Mark Ryan

Replying to anonymous:

Closing ticket after hearing no negative feedback to recent changes which should resolve this issue (at least for 0.12).

Please re-open if there are further issues.

Sorry - forgot to log in first. Ticket was closed by 'walnut'.....

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Mark Ryan.
The resolution will be deleted. Next status will be 'reopened'.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.