Code

Opened 22 months ago

Closed 20 months ago

Last modified 20 months ago

#18989 closed Cleanup/optimization (fixed)

Supspicious code in CursorWrapper.

Reported by: zimnyx Owned by: nobody
Component: Database layer (models, ORM) Version: 1.4
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by claudep)

Shouldn't it be __getattribute__() ?
If we want set_dirty() to be called on every access to 'db' and 'cursor' attributes, definitely yes. Otherwise "if attr in self.dict: ..." should be removed.

# Django 1.4
class CursorWrapper(object):
    def __init__(self, cursor, db): 
        self.cursor = cursor
        self.db = db

    def set_dirty(self):
        if self.db.is_managed():        
            self.db.set_dirty()

    def __getattr__(self, attr):    
        self.set_dirty()
        if attr in self.__dict__:       
            return self.__dict__[attr]      
        else:
            return getattr(self.cursor, attr)

Attachments (1)

18989-1.diff (639 bytes) - added by claudep 21 months ago.
Optimize CursorWrapper getattr

Download all attachments as: .zip

Change History (6)

comment:2 Changed 21 months ago by claudep

  • Description modified (diff)
  • Type changed from Uncategorized to Cleanup/optimization

comment:3 Changed 21 months ago by claudep

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

Current coverage report for this file seems to give you credit: http://ci.djangoproject.com/job/Django%20Coverage/HTML_Coverage_Report/_var_lib_jenkins_jobs_Django%20Coverage_workspace_django_db_backends_util.html

I'd rather suppress the if attr in self.__dict__ part, as we mainly want to catch the calls to execute/executemany on this class' instances.

Changed 21 months ago by claudep

Optimize CursorWrapper getattr

comment:4 Changed 21 months ago by claudep

  • Has patch set

comment:5 Changed 20 months ago by Claude Paroz <claude@…>

  • Resolution set to fixed
  • Status changed from new to closed

In 8c69278764ae31b0cd495d21b84444bf2471c103:

Fixed #18989 -- Removed unused condition in CursorWrapper

Thanks zimnyx for the report.

comment:6 Changed 20 months ago by Claude Paroz <claude@…>

In a023952e10092775215aeb2277d2fe3af5794e3b:

[1.5.x] Fixed #18989 -- Removed unused condition in CursorWrapper

Thanks zimnyx for the report.
Backport of 8c6927876 from master.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


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

 
Note: See TracTickets for help on using tickets.