[Patchew-devel] [PATCH] models: avoid IntegrityError when clearing log property

[Caio Carrara]

Hello, Paolo.

Just some questions.

On Wed, Nov 28, 2018 at 03:44:59PM +0100, Paolo Bonzini wrote:
> Result's log property provides some magic to hide the existence of
> the LogEntry table (which in turn is only there to optimize access
> to results, since the logs are rarely needed).
> 
> However, deleting the entry directly in the setter does not work,
> because at this point the entry is still referenced in the Result
> table.  Postgres complains about a foreign key constraint violation
> when the setter calls entry.delete().
> 
> To fix this, which shows up as an error in the git-reset action,
> detect at save time whether the log_entry became None, and if so
> delete it _after_ saving the model.  While I am touching the code,
> fix the override of save() to correctly pass the arguments to the
> superclass.
> ---
>  api/models.py | 31 ++++++++++++++++++-------------
>  1 file changed, 18 insertions(+), 13 deletions(-)
> 
> diff --git a/api/models.py b/api/models.py
> index e24098b..8ec89b6 100644
> --- a/api/models.py
> +++ b/api/models.py
> @@ -74,11 +74,19 @@ class Result(models.Model):
>      def is_running(self):
>          return self.status == self.RUNNING
>  
> -    def save(self):
> +    def save(self, *args, **kwargs):
>          self.last_update = datetime.datetime.utcnow()
>          old_result = Result.objects.filter(pk=self.pk).first()
>          old_status = old_result.status if old_result else None
> -        super(Result, self).save()
> +        old_entry = old_result.log_entry if old_result else None
> +        super(Result, self).save(*args, **kwargs)
> +
> +        if self.log_entry is None and old_entry is not None:
> +            # Quick way to check if the field was actually saved to the database
> +            new_result = Result.objects.filter(pk=self.pk).first()
> +            if new_result.log_entry is None:
> +                old_entry.delete()
> +
>          emit_event("ResultUpdate", obj=self.obj,
>                     old_status=old_status, result=self)
>  
> @@ -109,18 +117,15 @@ class Result(models.Model):
>  
>      @log.setter
>      def log(self, value):
> -        entry = self.log_entry
>          if value is None:
> -            if entry is not None:
> -                self.log_entry = None

What if you just call self.save() here before calling entry.delete(),
whouldn't work?

> -                entry.delete()
> -        else:
> -            if entry is None:
> -                entry = LogEntry()
> -            entry.data = value
> -            entry.save()
> -            if self.log_entry is None:
> -                self.log_entry = entry
> +            self.log_entry = None

Shouldn't you have to save the model before returning here?

> +            return
> +
> +        entry = self.log_entry or LogEntry()
> +        entry.data = value
> +        entry.save()

The way the check is done here you can create a new log entry and do not
associate it with the Result. Right? Because you're checking the current
self.log_entry after creating the new one.

> +        if self.log_entry is None:
> +            self.log_entry = entry
>  
>      def get_log_url(self, request=None):
>          if not self.is_completed() or self.renderer is None:
> -- 
> 2.19.1
> 
> _______________________________________________
> Patchew-devel mailing list
> Patchew-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/patchew-devel

-- 
Caio Carrara
Software Engineer, Virt Team - Red Hat
ccarrara at redhat.com