819420 - new_from_list should use the object cache

Status: NEW

(Bugzilla :: Bugzilla-General, enhancement)

Description

[:glob ✱]

currently object created with new_from_list() don't make use of the object cache.  it should :)

a complication to doing this is match() (which new_from_list calls) creates objects by blessing the hashref returned by dbi, bypassing the normal bugzilla::object constructors.

some refactoring may be required to ensure that objects which perform extra steps in their constructor still work.

Comment 1

[David Lawrence [:dkl]]

(In reply to Byron Jones ‹:glob› from comment #0)
> currently object created with new_from_list() don't make use of the object
> cache.  it should :)
> 
> a complication to doing this is match() (which new_from_list calls) creates
> objects by blessing the hashref returned by dbi, bypassing the normal
> bugzilla::object constructors.
> 
> some refactoring may be required to ensure that objects which perform extra
> steps in their constructor still work.

It isn't trivial to be sure. My original idea was

sub new_from_list {
    my $invocant = shift;
    my $class = ref($invocant) || $invocant;
    my ($id_list) = @_;
    my $id_field = $class->ID_FIELD;

    my @detainted_ids;
    foreach my $id (@$id_list) {
        detaint_natural($id) ||
            ThrowCodeError('param_must_be_numeric',
                          {function => $class . '::new_from_list'});
        # Too large integers make PostgreSQL crash.
        next if $id > MAX_INT_32;
        push(@detainted_ids, $id);
    }


    # We don't do $invocant->match because some classes have
    # their own implementation of match which is not compatible
    # with this one. However, match() still needs to have the right $invocant
    # in order to do $class->DB_TABLE and so on.
    return match($invocant, { $id_field => \@detainted_ids });
}

Comment 2

[David Lawrence [:dkl]]

(In reply to David Lawrence [:dkl] from comment #1)
> (In reply to Byron Jones ‹:glob› from comment #0)
> > currently object created with new_from_list() don't make use of the object
> > cache.  it should :)
> > 
> > a complication to doing this is match() (which new_from_list calls) creates
> > objects by blessing the hashref returned by dbi, bypassing the normal
> > bugzilla::object constructors.
> > 
> > some refactoring may be required to ensure that objects which perform extra
> > steps in their constructor still work.
> 
> It isn't trivial to be sure. My original idea was
> 
> sub new_from_list {
>     my $invocant = shift;
>     my $class = ref($invocant) || $invocant;
>     my ($id_list) = @_;
>     my $id_field = $class->ID_FIELD;
> 
>     my @detainted_ids;
>     foreach my $id (@$id_list) {
>         detaint_natural($id) ||
>             ThrowCodeError('param_must_be_numeric',
>                           {function => $class . '::new_from_list'});
>         # Too large integers make PostgreSQL crash.
>         next if $id > MAX_INT_32;
>         push(@detainted_ids, $id);
>     }
> 
> 
>     # We don't do $invocant->match because some classes have
>     # their own implementation of match which is not compatible
>     # with this one. However, match() still needs to have the right $invocant
>     # in order to do $class->DB_TABLE and so on.
>     return match($invocant, { $id_field => \@detainted_ids });
> }

Damn, hit enter too quickly. Let me revise...

sub new_from_list {
    my $invocant = shift;
    my $class = ref($invocant) || $invocant;
    my ($id_list) = @_;
    my $id_field = $class->ID_FIELD;

    my @detainted_ids;
    foreach my $id (@$id_list) {
        detaint_natural($id) ||
            ThrowCodeError('param_must_be_numeric',
                          {function => $class . '::new_from_list'});
        # Too large integers make PostgreSQL crash.
        next if $id > MAX_INT_32;
        push(@detainted_ids, $id);
    }

    my @filtered_ids;
    my @results;
    foreach my $id (@detainted_ids) {
        my $cache_key = $class->cache_key($id);
        if ($my $object = class->_cache_get($cache_id)) {
      
        }
    }
    # We don't do $invocant->match because some classes have
    # their own implementation of match which is not compatible
    # with this one. However, match() still needs to have the right $invocant
    # in order to do $class->DB_TABLE and so on.
    return match($invocant, { $id_field => \@detainted_ids });
}

Comment 3

[David Lawrence [:dkl]]

(In reply to David Lawrence [:dkl] from comment #2)
> (In reply to David Lawrence [:dkl] from comment #1)
> > (In reply to Byron Jones ‹:glob› from comment #0)
> > > currently object created with new_from_list() don't make use of the object
> > > cache.  it should :)
> > > 
> > > a complication to doing this is match() (which new_from_list calls) creates
> > > objects by blessing the hashref returned by dbi, bypassing the normal
> > > bugzilla::object constructors.
> > > 
> > > some refactoring may be required to ensure that objects which perform extra
> > > steps in their constructor still work.
> > 
> > It isn't trivial to be sure. My original idea was
> > 
> > sub new_from_list {
> >     my $invocant = shift;
> >     my $class = ref($invocant) || $invocant;
> >     my ($id_list) = @_;
> >     my $id_field = $class->ID_FIELD;
> > 
> >     my @detainted_ids;
> >     foreach my $id (@$id_list) {
> >         detaint_natural($id) ||
> >             ThrowCodeError('param_must_be_numeric',
> >                           {function => $class . '::new_from_list'});
> >         # Too large integers make PostgreSQL crash.
> >         next if $id > MAX_INT_32;
> >         push(@detainted_ids, $id);
> >     }
> > 
> > 
> >     # We don't do $invocant->match because some classes have
> >     # their own implementation of match which is not compatible
> >     # with this one. However, match() still needs to have the right $invocant
> >     # in order to do $class->DB_TABLE and so on.
> >     return match($invocant, { $id_field => \@detainted_ids });
> > }
> 
> Damn, hit enter too quickly. Let me revise...
> 
> sub new_from_list {
>     my $invocant = shift;
>     my $class = ref($invocant) || $invocant;
>     my ($id_list) = @_;
>     my $id_field = $class->ID_FIELD;
> 
>     my @detainted_ids;
>     foreach my $id (@$id_list) {
>         detaint_natural($id) ||
>             ThrowCodeError('param_must_be_numeric',
>                           {function => $class . '::new_from_list'});
>         # Too large integers make PostgreSQL crash.
>         next if $id > MAX_INT_32;
>         push(@detainted_ids, $id);
>     }
> 
>     my @filtered_ids;
>     my @results;
>     foreach my $id (@detainted_ids) {
>         my $cache_key = $class->cache_key($id);
>         if ($my $object = class->_cache_get($cache_id)) {
>       
>         }
>     }
>     # We don't do $invocant->match because some classes have
>     # their own implementation of match which is not compatible
>     # with this one. However, match() still needs to have the right $invocant
>     # in order to do $class->DB_TABLE and so on.
>     return match($invocant, { $id_field => \@detainted_ids });
> }

Damn, hit enter prematurely again :( I will come back to this in a bit when I have had some more caffeine.

Comment 4

[:glob ✱]

> Damn, hit enter prematurely again :( I will come back to this in a bit when
> I have had some more caffeine.

looks like you really want to take this bug.. reassigning :)

you should also populate the cache with the results from new_from_list, taking heed of objects which perform additional work in their constructors after calling SUPER::new (such as Bugzilla::Search::Saved).

Comment 5

[David Lawrence [:dkl]]

Attachment: Patch to make new_from_list use/set cached objects (v1)

Comment 6

[:glob ✱]

Comment on attachment 689937 [details] [diff] [review]
Patch to make new_from_list use/set cached objects (v1)

i'm still not comfortable with caching objects which aren't fully initialised.  skipping the Bugzilla::Search::Saved object is a kludge and likely to cause problems in future if someone enables caching on it (or if it's enabled by default).  objects created via new_from_list() should be the same as objects created by new().

you could update the bugzilla::object constructor to optionally accept a hashref which it would then use instead of hitting the database, or split the object initialisation code into a new public bugzilla::object method, and have subclasses overwrite this method.

given the scope of this, i'd be happy for this work to be split into two bugs if you think that's appropriate -- this bug can deal with new_from_list just reusing any objects already cached but not populating the cache with objects it has created from the db, and the refactoring and cache population happening in a different bug.