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
-                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
+            return
+
+        entry = self.log_entry or LogEntry()
+        entry.data = value
+        entry.save()
+        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@redhat.com
https://www.redhat.com/mailman/listinfo/patchew-devel

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@redhat.com
> https://www.redhat.com/mailman/listinfo/patchew-devel

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

_______________________________________________
Patchew-devel mailing list
Patchew-devel@redhat.com
https://www.redhat.com/mailman/listinfo/patchew-devel

On 28/11/18 16:40, Caio Carrara wrote:
>> @@ -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?

Same answer as below...

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

... The idea is that "log" behaves as a field.  The caller is supposed
to do save() on the Result, we should not do it on their behalf.

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

That cannot happen; self.log_entry will be None if and only if I created
a new LogEntry().

Would you prefer if I had

    entry = self.log_entry
    if entry is None:
         entry = self.log_entry = LogEntry()
    entry.data = value
    entry.save()

?

Paolo

_______________________________________________
Patchew-devel mailing list
Patchew-devel@redhat.com
https://www.redhat.com/mailman/listinfo/patchew-devel