Skip to contentSkip to conversation

Various fixes

PATCH New Paul Eggleton 1 participant 7 messages Jan 9, 2018

lore
Share & tools
lore
#0 lore raw

cover Various fixes

Fix a bunch of issues I found recently working on the layer index. I plan on merging these almost immediately as we need several of these fixes in the live instance on openembedded.org.

The following changes since commit fa0b01082fb0596959841e95af8c516ae3f267a7:

admin: fix dependency display and search (2017-12-21 16:14:17 +1300)

are available in the git repository at:

git://git.yoctoproject.org/layerindex-web paule/fixes
http://git.yoctoproject.org/cgit.cgi//log/?h=paule/fixes

Paul Eggleton (6):
update_layer: fix handling of database errors Explicitly handle too-long field values
Fix error on publish if no non-root superuser/staff accounts exist
update: don't stop on unsatisfied layer dependencies
views: fix "layer:" keyword on recipe search page
views: fix classic recipe search redirecting to recipe page with
single result

 layerindex/models.py       | 21 +++++++++++++++++++++
 layerindex/update.py       | 20 ++++++++++----------
 layerindex/update_layer.py |  4 ++++
 layerindex/views.py        | 11 ++++++++---
 4 files changed, 43 insertions(+), 13 deletions(-)
Signature · 1 line
2.9.5
#1 lore raw

PATCH 1/6 update_layer: fix handling of database errors

If a database error occurs when we save a recipe (e.g. because a database-level constraint is voilated) it will mess up the transaction. Unfortunately that means we need to break out of updating the entire layer rather than catching the error, because if we do catch it we just get errors on every update after the initial error; failing early and giving up on the transaction is a little better in terms of not filling up the update logs with further useless errors.

---
 layerindex/update_layer.py | 4 ++++
 1 file changed, 4 insertions(+)
layerindex/update_layer.py +4 -0
index 307c43a..42e4da9 100644
@@ -56,6 +56,8 @@ def split_recipe_fn(path):
     return (pn, pv)
 
 def update_recipe_file(tinfoil, data, path, recipe, layerdir_start, repodir):
+    from django.db import DatabaseError
+
     fn = str(os.path.join(path, recipe.filename))
     from layerindex.models import PackageConfig, StaticBuildDep, DynamicBuildDep
     try:
@@ -156,6 +158,8 @@ def update_recipe_file(tinfoil, data, path, recipe, layerdir_start, repodir):
 
     except KeyboardInterrupt:
         raise
+    except DatabaseError:
+        raise
     except BaseException as e:
         if not recipe.pn:
             recipe.pn = recipe.filename[:-3].split('_')[0]
Signature · 1 line
2.9.5
#2 lore raw

PATCH 2/6 Explicitly handle too-long field values

In reply to Paul Eggleton

If you use a traditional database engine (such as MySQL/MariaDB) then maximum character field lengths are enforced, however depending on the configuration this may result in an exception rather than a warning and truncation and will also break the per-layer transaction. To avoid that ugliness, add a signal handler to do it internally, which as a bonus lets us know if field lenghts are too short for data when using database engines that don't enforce lengths (e.g. SQLite).

---
 layerindex/models.py | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)
layerindex/models.py +21 -0
index 6fdef04..648ef1f 100644
@@ -9,10 +9,31 @@ from datetime import datetime
 from django.contrib.auth.models import User
 from django.core.urlresolvers import reverse
 from django.core.validators import URLValidator
+from django.db.models.signals import pre_save
+from django.dispatch import receiver
 import os.path
 import re
 import posixpath
 
+from . import utils
+
+
+logger = utils.logger_create('LayerIndexModels')
+
+
+@receiver(pre_save)
+def truncate_charfield_values(sender, instance, *args, **kwargs):
+    # Instead of leaving this up to the database, check and handle it
+    # ourselves to avoid nasty exceptions; as a bonus we won't miss when
+    # the max length is too short with databases that don't enforce
+    # the limits (e.g. sqlite)
+    for field in instance._meta.get_fields():
+        if isinstance(field, models.CharField):
+            value = getattr(instance, field.name)
+            if value and len(value) > field.max_length:
+                logger.warning('%s.%s: length %s exceeds maximum (%s), truncating' % (instance.__class__.__name__, field.name, len(value), field.max_length))
+                setattr(instance, field.name, value[:field.max_length])
+
 
 class PythonEnvironment(models.Model):
     name = models.CharField(max_length=50)
Signature · 1 line
2.9.5
#3 lore raw

PATCH 3/6 Fix error on publish if no non-root superuser/staff accounts exist

In reply to Paul Eggleton

We need to set a default for this variable or you get an UnboundLocalError if no non-root superuser/staff accounts are set up in the database.

---
 layerindex/views.py | 1 +
 1 file changed, 1 insertion(+)
layerindex/views.py +1 -0
index ccb85b8..5b111a3 100644
@@ -270,6 +270,7 @@ def publish(request, name):
     bodytext = get_template('layerindex/publishemail.txt')
     maintainer_names = [m.name for m in maintainers]
     # find appropriate help contact
+    help_contact = None
     for user in User.objects.all():
         if user.username != 'root' and (user.is_staff or user.is_superuser) and user.is_active:
             help_contact = user
Signature · 1 line
2.9.5
#4 lore raw

PATCH 4/6 update: don't stop on unsatisfied layer dependencies

In reply to Paul Eggleton

Unsatisfied layer dependencies shouldn't error out of the script - broken metadata isn't supposed to terminate the index update process.

---
 layerindex/update.py | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)
layerindex/update.py +10 -10
index d1e67a0..dc20b30 100755
@@ -299,7 +299,7 @@ def main():
                 # If layer_A depends(or recommends) on layer_B, add layer_B before layer_A
                 deps_dict_all = {}
                 layerquery_sorted = []
-                collections_done = set()
+                collections = set()
                 branchobj = utils.get_branch(branch)
                 for layer in layerquery_all:
                     # Get all collections from database, but we can't trust the
@@ -309,7 +309,7 @@ def main():
                         continue
                     layerbranch = layer.get_layerbranch(branch)
                     if layerbranch:
-                        collections_done.add((layerbranch.collection, layerbranch.version))
+                        collections.add((layerbranch.collection, layerbranch.version))
 
                 for layer in layerquery:
                     cmd = prepare_update_layer_command(options, branchobj, layer, initial=True)
@@ -323,11 +323,12 @@ def main():
                     deps = re.search("^LAYERDEPENDS = \"(.*)\"", output, re.M).group(1) or ''
                     recs = re.search("^LAYERRECOMMENDS = \"(.*)\"", output, re.M).group(1) or ''
 
+                    collections.add((col, ver))
+
                     deps_dict = utils.explode_dep_versions2(bitbakepath, deps + ' ' + recs)
                     if len(deps_dict) == 0:
                         # No depends, add it firstly
                         layerquery_sorted.append(layer)
-                        collections_done.add((col, ver))
                         continue
                     deps_dict_all[layer] = {'requires': deps_dict, 'collection': col, 'version': ver}
 
@@ -342,24 +343,23 @@ def main():
                                 req_ver = req_ver_list[0]
                             else:
                                 req_ver = None
-                            if utils.is_deps_satisfied(req_col, req_ver, collections_done):
+                            if utils.is_deps_satisfied(req_col, req_ver, collections):
                                 del(value['requires'][req_col])
                         if not value['requires']:
-                            # All the depends are in collections_done:
+                            # All the depends are in collections:
                             del(deps_dict_all[layer])
                             layerquery_sorted.append(layer)
-                            collections_done.add((value['collection'], value['version']))
 
                     if not len(deps_dict_all):
                         break
 
-                    # Something is wrong if nothing changed after a run
+                    # If nothing changed after a run then some dependencies couldn't be resolved
                     if operator.eq(deps_dict_all_copy, deps_dict_all):
-                        logger.error("Cannot find required collections on branch %s:" % branch)
+                        logger.warning("Cannot find required collections on branch %s:" % branch)
                         for layer, value in deps_dict_all.items():
                             logger.error('%s: %s' % (layer.name, value['requires']))
-                        logger.error("Known collections: %s" % collections_done)
-                        sys.exit(1)
+                        logger.warning("Known collections on branch %s: %s" % (branch, collections))
+                        break
 
                 for layer in layerquery_sorted:
                     layerupdate = LayerUpdate()
Signature · 1 line
2.9.5
#5 lore raw

PATCH 5/6 views: fix "layer:" keyword on recipe search page

In reply to Paul Eggleton

We were using the layerbranch id to search for the specified layer, which is most likely to return either no results or results for the wrong layer. We can also avoid specifying the id field at all here as the filter() function can handle real objects.

---
 layerindex/views.py | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
layerindex/views.py +3 -3
index 5b111a3..84de4e0 100644
@@ -449,11 +449,11 @@ class RecipeSearchView(ListView):
                     messages.add_message(self.request, messages.ERROR, 'The \
 layer name is expected to follow the \"layer:\" prefix without any spaces.')
                 else:
-                    query_layer = LayerBranch.objects.filter(
-                        layer__name=query_layername)
+                    query_layer = LayerItem.objects.filter(
+                        name=query_layername)
                     if query_layer:
                         init_qs = init_qs.filter(
-                            layerbranch__layer__id=query_layer[0].id)
+                            layerbranch__layer=query_layer[0])
                     else:
                         messages.add_message(self.request, messages.ERROR,
                                             'No layer \"%s\" was found.'
Signature · 1 line
2.9.5
#6 lore raw

PATCH 6/6 views: fix classic recipe search redirecting to recipe page with single result

In reply to Paul Eggleton

If your classic recipe search returned a single result, then since commit c8c25fb641c500354cf36c3c59abb6f9fe96d223 you got a standard recipe page instead of the proper page with fields you can edit. To fix it, just drop the redirection functionality from the classic recipe search, since we don't really want it here.

---
 layerindex/views.py | 4 ++++
 1 file changed, 4 insertions(+)
layerindex/views.py +4 -0
index 84de4e0..4f6c2c9 100644
@@ -865,6 +865,10 @@ class RecipeDetailView(DetailView):
 
 
 class ClassicRecipeSearchView(RecipeSearchView):
+    def render_to_response(self, context, **kwargs):
+        # Bypass the redirect-to-single-instance behaviour of RecipeSearchView
+        return super(ListView, self).render_to_response(context, **kwargs)
+
     def get_queryset(self):
         self.kwargs['branch'] = 'oe-classic'
         query_string = self.request.GET.get('q', '')
Signature · 1 line
2.9.5