From 9e24eb73b9a45349ca9fd0a9c818fc1e6430cc93 Mon Sep 17 00:00:00 2001
From: Tom Gottfried <Tom Gottfried tom@intevation.de>
Date: Mon, 21 Aug 2017 19:23:26 +0200
Subject: [PATCH 1/6] Query.scalar() is equivalent to try one() and return None
 on exception.

---
 ringo/lib/extension.py | 23 ++++++++++-------------
 ringo/lib/security.py  |  5 +----
 2 files changed, 11 insertions(+), 17 deletions(-)

diff --git a/ringo/lib/extension.py b/ringo/lib/extension.py
index fa799181..221f8090 100644
--- a/ringo/lib/extension.py
+++ b/ringo/lib/extension.py
@@ -106,19 +106,16 @@ def _add_modul(config, actions, dbsession):
 
 
 def _load_modul_by_name(session, modulname):
-    try:
-        # FIXME:
-        # Compatibilty mode. Older versions of Ringo added a 's' to the
-        # extensions modul name as Ringo usally uses the plural form.
-        # Newer versions use the configured extension name. So there
-        # might be a mixture of old and new modul names in the database.
-        # This code will handle this. (ti) <2016-01-04 13:50>
-        return session.query(ModulItem)\
-               .filter(sa.or_(ModulItem.name == modulname,
-                              ModulItem.name == modulname + 's'))\
-                .one()
-    except sa.orm.exc.NoResultFound:
-        return None
+    # FIXME:
+    # Compatibilty mode. Older versions of Ringo added a 's' to the
+    # extensions modul name as Ringo usally uses the plural form.
+    # Newer versions use the configured extension name. So there
+    # might be a mixture of old and new modul names in the database.
+    # This code will handle this. (ti) <2016-01-04 13:50>
+    return session.query(ModulItem)\
+                  .filter(sa.or_(ModulItem.name == modulname,
+                                 ModulItem.name == modulname + 's'))\
+                  .scalar()
 
 
 def register_modul(config, modul_config, actions=None):
diff --git a/ringo/lib/security.py b/ringo/lib/security.py
index df8f3724..40fa34aa 100644
--- a/ringo/lib/security.py
+++ b/ringo/lib/security.py
@@ -128,10 +128,7 @@ def load_user(login):
     :returns: User or None
 
     """
-    try:
-        return DBSession.query(User).filter_by(login=login).one()
-    except NoResultFound:
-        return None
+    return DBSession.query(User).filter_by(login=login).scalar()
 
 
 def csrf_token_validation(event):

From e379021dd51bc67d4bb6338c5088663176ea4dc2 Mon Sep 17 00:00:00 2001
From: Tom Gottfried <Tom Gottfried tom@intevation.de>
Date: Wed, 23 Aug 2017 17:26:58 +0200
Subject: [PATCH 2/6] Replace filtering by ID and using Query.one() by
 Query.get().

get() takes already loaded objects from the sessions identity map,
whereas one() always triggers a database query. Thus, using get()
potentially improves performance. Consequently, changed BaseFactory.load()
to always return None instead of raising NoResultFound.
---
 ringo/lib/helpers/misc.py |  2 +-
 ringo/lib/imexport.py     |  2 --
 ringo/model/base.py       | 22 +++++++++++++---------
 ringo/views/users.py      |  3 +--
 4 files changed, 15 insertions(+), 14 deletions(-)

diff --git a/ringo/lib/helpers/misc.py b/ringo/lib/helpers/misc.py
index 9f0d3388..9febf796 100644
--- a/ringo/lib/helpers/misc.py
+++ b/ringo/lib/helpers/misc.py
@@ -225,7 +225,7 @@ def import_model(clazzpath):
     orig_clazz = dynamic_import(clazzpath)
     # Load entry from the database for the given modul
     mid = orig_clazz._modul_id
-    modul = DBSession.query(ModulItem).filter_by(id=mid).one()
+    modul = DBSession.query(ModulItem).get(mid)
     if modul.clazzpath == clazzpath:
         return orig_clazz
     else:
diff --git a/ringo/lib/imexport.py b/ringo/lib/imexport.py
index 3080c54b..9ddbe65b 100644
--- a/ringo/lib/imexport.py
+++ b/ringo/lib/imexport.py
@@ -454,8 +454,6 @@ def perform(self, data, user=None, translate=lambda x: x, load_key="uuid"):
                 else:
                     id = values.get(load_key)
                 try:
-                    # uuid might be empty for new items, which will raise an
-                    # error on loading.
                     item = factory.load(id, field=load_key)
                     item.set_values(values, use_strict=self._use_strict)
                     operation = _("UPDATE")
diff --git a/ringo/model/base.py b/ringo/model/base.py
index 6476a033..c8c8d029 100644
--- a/ringo/model/base.py
+++ b/ringo/model/base.py
@@ -17,6 +17,7 @@
 import Levenshtein
 from sqlalchemy import Column, CHAR
 from sqlalchemy.orm import joinedload, Session
+from sqlalchemy.orm.exc import NoResultFound
 from ringo.lib.helpers import (
     serialize, get_item_modul,
     get_raw_value, set_raw_value,
@@ -115,7 +116,7 @@ def load_modul(item):
     # Loading the modul is expensive! So try to cache it.
     if not CACHE_MODULES.get(mid):
         if session:
-            modul = session.query(ModulItem).filter_by(id=mid).one()
+            modul = session.query(ModulItem).get(mid)
         else:
             modul = get_item_modul(None, item)
         CACHE_MODULES.set(modul.id, modul)
@@ -895,21 +896,20 @@ def create(self, user, values):
                 item.set_values(values)
         return item
 
-    def load(self, id, db=None, cache="", uuid=False, field=None):
+    def load(self, id, db=DBSession, cache="", uuid=False, field=None):
         """Loads the item with id from the database and returns it.
 
-        :id: ID of the item to be loaded
+        :id: Primary key or field value (if field is given) of the item to
+             be loaded
         :db: DB session to load the item
         :cache: Name of the cache region. If empty then no caching is
                 done.
-        :uuid: If true the given id is a uuid. Default to false
-        :field: If given the item can be loaded by an alternative field.
-                Default to None
+        :uuid: (deprecated) If True the given id is a uuid. Defaults to False
+        :field: If given, id is expected to be a unique value of the given
+                field. Defaults to None
         :returns: Instance of clazz
 
         """
-        if db is None:
-            db = DBSession
         q = db.query(self._clazz)
         if cache in regions.keys():
             q = set_relation_caching(q, self._clazz, cache)
@@ -924,4 +924,8 @@ def load(self, id, db=None, cache="", uuid=False, field=None):
                           "field='uuid' instead", DeprecationWarning)
             return q.filter(self._clazz.uuid == id).one()
         else:
-            return q.filter(self._clazz.id == id).one()
+            item = q.get(id)
+            if item:
+                return item
+            else:
+                raise NoResultFound
diff --git a/ringo/views/users.py b/ringo/views/users.py
index d82c0b64..5d7cac6d 100644
--- a/ringo/views/users.py
+++ b/ringo/views/users.py
@@ -219,7 +219,7 @@ def setstandin(request, allowed_users=None):
         if request.GET.get('backurl'):
             user_id = urlparse.urlparse(
                 request.GET.get('backurl')).path.split('/')[-1]
-            user = request.db.query(User).filter(User.id == user_id).one()
+            user = request.db.query(User).get(user_id)
         # Otherwise the user sets the standin of his own group. In this
         # case the user is already in the request.
         else:
@@ -330,7 +330,6 @@ def removeaccount(request):
     _ = request.translate
     # Load the item return 400 if the item can not be found.
     factory = clazz.get_item_factory()
-
     try:
         item = factory.load(id, request.db)
         # Check authorisation

From 3652271d27d0a525b747b8a4bd046f4e68e952c0 Mon Sep 17 00:00:00 2001
From: Tom Gottfried <Tom Gottfried tom@intevation.de>
Date: Wed, 23 Aug 2017 17:38:35 +0200
Subject: [PATCH 3/6] With load_modul(), the modul can be fetched from sessions
 identity map.

At least when importing a list of n objects of the same type, this
reduces the queries for modul loading from n to 1.
---
 ringo/model/base.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ringo/model/base.py b/ringo/model/base.py
index c8c8d029..921dbac4 100644
--- a/ringo/model/base.py
+++ b/ringo/model/base.py
@@ -866,7 +866,7 @@ def __init__(self, clazz, request=None, use_strict=False):
         self._use_strict = use_strict
 
     def create(self, user, values):
-        """Will create a new instance of clazz. The instance is it is
+        """Will create a new instance of clazz. The instance is
         not saved persistent at this moment. The method will also take
         care of setting the correct ownership.
 
@@ -884,7 +884,7 @@ def create(self, user, values):
            user is not None):
             item.uid = user.id
         if (hasattr(item, 'gid')):
-            modul = get_item_modul(None, item)
+            modul = load_modul(item)
             if modul.default_gid:
                 item.gid = modul.default_gid
             elif (user is not None and user.default_gid):

From 509cb4bc3a1d6205a5082c88ec0de5ae9cb3406f Mon Sep 17 00:00:00 2001
From: Tom Gottfried <Tom Gottfried tom@intevation.de>
Date: Wed, 23 Aug 2017 17:42:32 +0200
Subject: [PATCH 4/6] Importer.perform already adds new objects to session.

Adding them again is harmless but superfluous.
---
 ringo/scripts/db.py | 1 -
 1 file changed, 1 deletion(-)

diff --git a/ringo/scripts/db.py b/ringo/scripts/db.py
index 6e5d9df6..5b959377 100644
--- a/ringo/scripts/db.py
+++ b/ringo/scripts/db.py
@@ -279,7 +279,6 @@ def do_import(session, importer, data, load_key):
     for item, action in items:
         # Add all new items to the session
         if action.find("CREATE") > -1:
-            session.add(item)
             created += 1
         else:
             updated += 1

From 542295f6976ee0708ff976e201b47935df0e0abc Mon Sep 17 00:00:00 2001
From: Tom Gottfried <Tom Gottfried tom@intevation.de>
Date: Thu, 24 Aug 2017 12:30:19 +0200
Subject: [PATCH 5/6] Unify loading of modul by name.

---
 ringo/lib/extension.py        | 17 ++--------------
 ringo/lib/helpers/__init__.py |  1 +
 ringo/lib/helpers/misc.py     | 14 +++++++++++++
 ringo/scripts/application.py  |  8 ++++----
 ringo/scripts/db.py           | 38 +++++++++++++++++------------------
 5 files changed, 40 insertions(+), 38 deletions(-)

diff --git a/ringo/lib/extension.py b/ringo/lib/extension.py
index 221f8090..fe556de9 100644
--- a/ringo/lib/extension.py
+++ b/ringo/lib/extension.py
@@ -29,10 +29,10 @@
 
 """
 import logging
-import sqlalchemy as sa
 import transaction
 from ringo.lib.sql.db import DBSession
 from ringo.model.modul import ModulItem, ACTIONS
+from ringo.lib.helpers import get_modul_by_name
 
 log = logging.getLogger(name=__name__)
 
@@ -105,19 +105,6 @@ def _add_modul(config, actions, dbsession):
     return modul
 
 
-def _load_modul_by_name(session, modulname):
-    # FIXME:
-    # Compatibilty mode. Older versions of Ringo added a 's' to the
-    # extensions modul name as Ringo usally uses the plural form.
-    # Newer versions use the configured extension name. So there
-    # might be a mixture of old and new modul names in the database.
-    # This code will handle this. (ti) <2016-01-04 13:50>
-    return session.query(ModulItem)\
-                  .filter(sa.or_(ModulItem.name == modulname,
-                                 ModulItem.name == modulname + 's'))\
-                  .scalar()
-
-
 def register_modul(config, modul_config, actions=None):
     return check_register_modul(config, modul_config, actions)
 
@@ -139,7 +126,7 @@ def check_register_modul(config, modul_config, actions=None):
     """
 
     modulname = modul_config.get('name')
-    modul = _load_modul_by_name(DBSession, modulname)
+    modul = get_modul_by_name(modulname)
     if modul:
         log.info("Extension '%s' OK" % modulname)
     else:
diff --git a/ringo/lib/helpers/__init__.py b/ringo/lib/helpers/__init__.py
index 762d9702..6cbba3d5 100644
--- a/ringo/lib/helpers/__init__.py
+++ b/ringo/lib/helpers/__init__.py
@@ -31,6 +31,7 @@
     get_raw_value,
     set_raw_value,
     dynamic_import,
+    get_modul_by_name,
     import_model,
     get_item_modul,
     get_modules,
diff --git a/ringo/lib/helpers/misc.py b/ringo/lib/helpers/misc.py
index 9febf796..01fe2a72 100644
--- a/ringo/lib/helpers/misc.py
+++ b/ringo/lib/helpers/misc.py
@@ -2,6 +2,7 @@
 import re
 import string
 import base64
+import sqlalchemy as sa
 from datetime import datetime
 from pyramid.threadlocal import get_current_request, get_current_registry
 import formbar.converters as converters
@@ -211,6 +212,19 @@ def dynamic_import(cl):
     return getattr(m, classname)
 
 
+def get_modul_by_name(modulname, session=DBSession):
+    # FIXME:
+    # Compatibilty mode. Older versions of Ringo added a 's' to the
+    # extensions modul name as Ringo usally uses the plural form.
+    # Newer versions use the configured extension name. So there
+    # might be a mixture of old and new modul names in the database.
+    # This code will handle this. (ti) <2016-01-04 13:50>
+    from ringo.model.modul import ModulItem
+    return session.query(ModulItem).filter(
+        sa.or_(ModulItem.name == modulname,
+               ModulItem.name == modulname + 's')).scalar()
+
+
 def import_model(clazzpath):
     """Will return the clazz defined by modul entry in the database of
     the given model. The clazzpath defines the base clazz which which
diff --git a/ringo/scripts/application.py b/ringo/scripts/application.py
index 0032e6a1..46641ae6 100644
--- a/ringo/scripts/application.py
+++ b/ringo/scripts/application.py
@@ -1,8 +1,8 @@
 import os
 import transaction
 from ringo.scripts.db import get_session
-from ringo.lib.helpers import get_app_location, dynamic_import
-from ringo.lib.extension import _add_modul, _load_modul_by_name
+from ringo.lib.helpers import get_app_location, dynamic_import, get_modul_by_name
+from ringo.lib.extension import _add_modul
 from ringo.model import extensions
 
 
@@ -49,7 +49,7 @@ def handle_ext_init_command(args):
     session = get_session(os.path.join(*path))
     for ext in extensions:
         if ext == args.name:
-            if _load_modul_by_name(session, ext.replace("ringo_", "")):
+            if get_modul_by_name(ext.replace("ringo_", ""), session):
                 print "Extension %s already added!" % args.name
             else:
                 extension = dynamic_import("%s." % args.name)
@@ -65,7 +65,7 @@ def handle_ext_delete_command(args):
     path.append(get_app_location(args.app))
     path.append(args.config)
     session = get_session(os.path.join(*path))
-    modul = _load_modul_by_name(session, args.name.replace("ringo_", ""))
+    modul = get_modul_by_name(args.name.replace("ringo_", ""), session)
     if modul:
         if args.name in extensions:
             session.delete(modul)
diff --git a/ringo/scripts/db.py b/ringo/scripts/db.py
index 5b959377..d9e5ea5f 100644
--- a/ringo/scripts/db.py
+++ b/ringo/scripts/db.py
@@ -5,7 +5,6 @@
 import time
 import json
 from sqlalchemy import engine_from_config
-from sqlalchemy.orm.exc import NoResultFound
 import transaction
 
 from invoke import run
@@ -17,7 +16,7 @@
     setup_logging,
 )
 from ringo.lib.sql import DBSession, NTDBSession, setup_db_session
-from ringo.lib.helpers import get_app_location, dynamic_import
+from ringo.lib.helpers import get_app_location, dynamic_import, get_modul_by_name
 from ringo.lib.imexport import (
     JSONExporter, JSONImporter,
     CSVExporter, CSVImporter,
@@ -98,6 +97,17 @@ def get_session(config_file, transactional=True):
         return NTDBSession
 
 
+def get_modul(modulname, session):
+    modul_item = get_modul_by_name(modulname, session)
+    if not modul_item:
+        modules = [m.name for m in session.query(ModulItem).all()]
+        msg = "Can not load modul '{}'. Please choose one from [{}].".format(
+            modulname, ", ".join(modules))
+        print >> sys.stderr, msg
+        sys.exit(1)
+    return dynamic_import(modul_item.clazzpath)
+
+
 def copy_initial_migration_scripts(args):
     """Will copy the initial db migration scripts into the alembic
     versions folder of the application.
@@ -189,8 +199,7 @@ def handle_db_savedata_command(args):
     path = []
     path.append(args.config)
     session = get_session(os.path.join(*path))
-    modul_clazzpath = session.query(ModulItem).filter(ModulItem.name == args.modul).all()[0].clazzpath
-    modul = dynamic_import(modul_clazzpath)
+    modul = get_modul(args.modul, session)
     data = session.query(modul).order_by(modul.id).all()
 
     if args.filter:
@@ -256,19 +265,11 @@ def handle_db_loaddata_command(args):
 
 
 def get_importer(session, modulname, fmt):
-    try:
-        modul_clazzpath = session.query(ModulItem).filter(
-            ModulItem.name == modulname).one().clazzpath
-        modul = dynamic_import(modul_clazzpath)
-        if fmt == "json":
-            return JSONImporter(modul, session)
-        else:
-            return CSVImporter(modul, session)
-    except NoResultFound:
-        modules = [m.name for m in session.query(ModulItem).all()]
-        print "Can not load modul '{}'. Please choose one from [{}].".format(
-            modulname, ", ".join(modules))
-        sys.exit(1)
+    modul = get_modul(modulname, session)
+    if fmt == "json":
+        return JSONImporter(modul, session)
+    else:
+        return CSVImporter(modul, session)
 
 
 def do_import(session, importer, data, load_key):
@@ -289,8 +290,7 @@ def handle_db_uuid_command(args):
     path = []
     path.append(args.config)
     session = get_session(os.path.join(*path))
-    modul_clazzpath = session.query(ModulItem).filter(ModulItem.name == args.modul).all()[0].clazzpath
-    modul = dynamic_import(modul_clazzpath)
+    modul = get_modul(args.modul, session)
     updated = 0
     for item in session.query(modul).all():
         item.reset_uuid()

From 1bea2e7da4704ca552dfc8e077f6359b47901600 Mon Sep 17 00:00:00 2001
From: Tom Gottfried <Tom Gottfried tom@intevation.de>
Date: Thu, 7 Sep 2017 11:05:47 +0200
Subject: [PATCH 6/6] Just a little bit of error handling.

---
 ringo/views/users.py | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/ringo/views/users.py b/ringo/views/users.py
index 5d7cac6d..40a1b59b 100644
--- a/ringo/views/users.py
+++ b/ringo/views/users.py
@@ -220,6 +220,8 @@ def setstandin(request, allowed_users=None):
             user_id = urlparse.urlparse(
                 request.GET.get('backurl')).path.split('/')[-1]
             user = request.db.query(User).get(user_id)
+            if not user:
+                raise HTTPBadRequest()
         # Otherwise the user sets the standin of his own group. In this
         # case the user is already in the request.
         else: