From 118fcb9fed7a6d0320d387edab8262a0a862ed04 Mon Sep 17 00:00:00 2001 From: Aurélien Bompard Date: Dec 18 2017 13:58:21 +0000 Subject: Redesign the hub config DB schema Fixes #487 --- diff --git a/docs/api/auth.rst b/docs/api/auth.rst index c5e8382..2d1b25b 100644 --- a/docs/api/auth.rst +++ b/docs/api/auth.rst @@ -12,7 +12,7 @@ authorization system is in the :py:mod:`hubs.authz` module. Authorization ------------- -A hub's visibility is controlled by the ``HubConfig.visibility`` parameter. It +A hub's visibility is controlled by the ``hub.config["visibility"]`` parameter. It can have 3 values: - ``public``: the hub is visible to everyone diff --git a/hubs/feed.py b/hubs/feed.py index 4c92454..939f503 100644 --- a/hubs/feed.py +++ b/hubs/feed.py @@ -10,8 +10,8 @@ import flask import pymongo from fedmsg.encoding import loads, dumps -import hubs.app from hubs.models import Hub, User, Association +from hubs.utils import get_fedmsg_config log = logging.getLogger(__name__) @@ -142,7 +142,7 @@ class Feed(object): "You must subclass Feed and set self.msgtype.") self.owner = owner self.db = None - fedmsg_config = hubs.app.fedmsg_config + fedmsg_config = get_fedmsg_config() self.db_config = { "url": fedmsg_config.get('hubs.mongodb.url'), "db": fedmsg_config.get('hubs.mongodb.database', "hubs"), diff --git a/hubs/migrations/versions/20b23e867aeb_refactored_hubs_config_table.py b/hubs/migrations/versions/20b23e867aeb_refactored_hubs_config_table.py new file mode 100644 index 0000000..c3fc606 --- /dev/null +++ b/hubs/migrations/versions/20b23e867aeb_refactored_hubs_config_table.py @@ -0,0 +1,83 @@ +# This Alembic database migration is part of the Fedora Hubs project. +# Copyright (C) 2017 The Fedora Project +# +# This program is free software: you can redistribute it and/or modify +# it under the terms of the GNU Affero General Public License as published by +# the Free Software Foundation, either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU Affero General Public License for more details. +# +# You should have received a copy of the GNU Affero General Public License +# along with this program. If not, see . +""" +Refactored hubs config table + +Revision ID: 20b23e867aeb +Revises: bed8bbc0f78e +Create Date: 2017-12-08 11:58:15.289144 +""" + +from __future__ import absolute_import, unicode_literals + +from alembic import op +import sqlalchemy as sa + + +# revision identifiers, used by Alembic. +revision = '20b23e867aeb' +down_revision = 'bed8bbc0f78e' +branch_labels = None +depends_on = None + + +def upgrade(): + # Don't attempt to migrate the data. + op.drop_table('hubs_config') + op.create_table( + 'hubs_config', + sa.Column('id', sa.Integer(), nullable=False), + sa.Column('hub_id', sa.String(length=50), nullable=False), + sa.Column('key', sa.String(length=256), nullable=False), + sa.Column('value', sa.Text(), nullable=False), + sa.ForeignKeyConstraint(['hub_id'], ['hubs.name'], ), + sa.PrimaryKeyConstraint('id') + ) + op.create_index( + op.f('ix_hubs_config_hub_id'), 'hubs_config', ['hub_id'], unique=False) + op.create_index( + op.f('ix_hubs_config_key'), 'hubs_config', ['key'], unique=False) + op.create_index( + op.f('ix_hubs_config_value'), 'hubs_config', ['value'], unique=False) + # http://alembic.zzzcomputing.com/en/latest/batch.html + with op.batch_alter_table("hubs") as batch_op: + batch_op.drop_column('archived') + + +def downgrade(): + op.add_column('hubs', sa.Column('archived', sa.BOOLEAN(), nullable=True)) + op.drop_index(op.f('ix_hubs_config_value'), table_name='hubs_config') + op.drop_index(op.f('ix_hubs_config_key'), table_name='hubs_config') + op.drop_index(op.f('ix_hubs_config_hub_id'), table_name='hubs_config') + op.drop_table('hubs_config') + op.create_table( + 'hubs_config', + sa.Column('id', sa.INTEGER(), nullable=False), + sa.Column('hub_id', sa.VARCHAR(length=50), nullable=False), + sa.Column('summary', sa.VARCHAR(length=128), nullable=True), + sa.Column('left_width', sa.INTEGER(), nullable=False), + sa.Column('avatar', sa.VARCHAR(length=256), nullable=True), + sa.Column('header_img', sa.VARCHAR(length=256), nullable=True), + sa.Column('chat_channel', sa.VARCHAR(length=256), nullable=True), + sa.Column('chat_domain', sa.VARCHAR(length=256), nullable=True), + sa.Column('auth_group', sa.VARCHAR(length=256), nullable=True), + sa.Column('visibility', sa.VARCHAR(length=7), nullable=False), + sa.CheckConstraint( + "visibility IN ('public', 'preview', 'private')", + name='hub_visibility'), + sa.ForeignKeyConstraint(['hub_id'], ['hubs.name'], ), + sa.PrimaryKeyConstraint('id') + ) diff --git a/hubs/models.py b/hubs/models.py index 2b30812..6c44dc1 100644 --- a/hubs/models.py +++ b/hubs/models.py @@ -26,12 +26,12 @@ import datetime import json import logging import operator -import os -import random from collections import defaultdict +from collections.abc import MutableMapping import bleach import flask +import six import sqlalchemy as sa from sqlalchemy.orm import relation from sqlalchemy.orm import backref @@ -47,14 +47,8 @@ from hubs.signals import hub_created, user_created log = logging.getLogger(__name__) -def randomheader(): - location = '/static/img/headers/' - header_dir = os.path.dirname(__file__) + location - choice = random.choice(os.listdir(header_dir)) - return location + choice - - -ROLES = ['subscriber', 'member', 'owner', 'stargazer'] +ROLES = ('subscriber', 'member', 'owner', 'stargazer') +VISIBILITIES = ("public", "preview", "private") class Association(BASE): @@ -89,17 +83,26 @@ class Hub(ObjectAuthzMixin, BASE): created_on = sa.Column(sa.DateTime, default=datetime.datetime.utcnow) widgets = relation('Widget', cascade='all,delete', backref='hub', order_by="Widget.index") - config = relation('HubConfig', uselist=False, cascade='all,delete', - backref='hub') - archived = sa.Column(sa.Boolean, default=False) user_hub = sa.Column(sa.Boolean, default=False) # Timestamps about various kinds of "freshness" last_refreshed = sa.Column(sa.DateTime, default=datetime.datetime.utcnow) last_edited = sa.Column(sa.DateTime, default=datetime.datetime.utcnow) + config_values = relation( + 'HubConfig', backref="hub", cascade='all,delete-orphan') # fas_group = sa.Column(sa.String(32), nullable=False) @property + def config(self): + return HubConfigProxy(self) + + @config.setter + def config(self, config): + proxy = HubConfigProxy(self) + proxy.clear() + proxy.update(config) + + @property def days_idle(self): return (datetime.datetime.utcnow() - self.last_refreshed).days @@ -189,9 +192,8 @@ class Hub(ObjectAuthzMixin, BASE): session = Session() hub = cls(name=username, user_hub=True) session.add(hub) - hub_config = HubConfig( - hub=hub, summary=fullname, avatar=username2avatar(username)) - session.add(hub_config) + hub.config["summary"] = fullname + hub.config["avatar"] = username2avatar(username) session.flush() hub_created.send(hub) return hub @@ -201,10 +203,10 @@ class Hub(ObjectAuthzMixin, BASE): session = Session() hub = cls(name=name, user_hub=False) session.add(hub) - # TODO -- do something else, smarter for group avatars - hub_config = HubConfig( - hub=hub, summary=summary, avatar=username2avatar(name)) - session.add(hub_config) + hub.config["summary"] = summary + if extra.get("irc_channel") and extra.get("irc_network"): + hub.config["chat_domain"] = extra["irc_network"] + hub.config["chat_channel"] = extra["irc_channel"] session.flush() hub_created.send(hub, **extra) return hub @@ -222,7 +224,7 @@ class Hub(ObjectAuthzMixin, BASE): if not widget_instance.enabled: continue widget = widget_instance.module - new_config = self.config.__json__() + new_config = self.config.to_dict() will_reload = False cached_functions = widget.get_cached_functions() for fn_name, fn_class in cached_functions.items(): @@ -256,7 +258,7 @@ class Hub(ObjectAuthzMixin, BASE): return self.name # When the CAIAPI is in place we will be able to use the auth_group # setting: - # group = self.config.auth_group + # group = self.config["auth_group"] # if group is None: # group = self.name # return group @@ -276,14 +278,14 @@ class Hub(ObjectAuthzMixin, BASE): def _get_auth_permission_name(self, action): if action == "view": - action = "{}.view".format(self.config.visibility) + action = "{}.view".format(self.config["visibility"]) return "hub.{}".format(action) def get_props(self): """Get the hub properties for the Javascript UI""" result = { "name": self.name, - "config": self.config.__json__(), + "config": self.config.to_dict(), "users": {role: [] for role in ROLES}, "mtime": self.last_refreshed, "user_hub": self.user_hub, @@ -307,7 +309,7 @@ class Hub(ObjectAuthzMixin, BASE): return { 'name': self.name, 'archived': self.archived, - 'config': self.config.__json__(), + 'config': self.config.to_dict(), 'widgets': [widget.idx for widget in self.widgets], @@ -317,45 +319,162 @@ class Hub(ObjectAuthzMixin, BASE): } -class HubConfig(BASE): +class Converter(object): - __tablename__ = 'hubs_config' + def __init__(self, func=None): + if func is None: + self.func = lambda v: v + else: + self.func = func - VISIBILITY = ["public", "preview", "private"] + def from_db(self, value): + return self.func(value) - id = sa.Column(sa.Integer, primary_key=True) - hub_id = sa.Column(sa.String(50), sa.ForeignKey('hubs.name'), - nullable=False) - summary = sa.Column(sa.String(128)) - left_width = sa.Column(sa.Integer, nullable=False, default=8) - # A URL to the "avatar" for this hub. - avatar = sa.Column(sa.String(256), default="") - header_img = sa.Column(sa.String(256), default=randomheader) - chat_channel = sa.Column(sa.String(256), nullable=True) - chat_domain = sa.Column(sa.String(256), nullable=True) - auth_group = sa.Column(sa.String(256), nullable=True) - visibility = sa.Column( - sa.Enum(*VISIBILITY, name="hub_visibility"), - default="public", nullable=False) + def to_db(self, value): + return six.text_type(value) - def __json__(self): - return { - 'summary': self.summary, - 'left_width': self.left_width, - 'right_width': self.right_width, - 'avatar': self.avatar, - 'chat_channel': self.chat_channel, - 'chat_domain': self.chat_domain, - 'visibility': self.visibility, - } - @property - def right_width(self): - return 12 - self.left_width +class BooleanConverter(Converter): + + def __init__(self): + self.func = bool - @right_width.setter - def right_width(self, value): - self.left_width = 12 - value + def to_db(self, value): + if value: + return "True" + else: + return "" + + +class EnumConverter(Converter): + + def __init__(self, allowed_values): + self.func = lambda v: v + self.allowed_values = allowed_values + + def to_db(self, value): + if value not in self.allowed_values: + raise ValueError("{} is not in {}".format( + value, repr(self.allowed_values))) + return super(EnumConverter, self).to_db(value) + + +class HubConfigProxy(MutableMapping): + + KEYS = ( + "archived", "summary", "left_width", "avatar", "visibility", + "chat_domain", "chat_channel", + ) + CONVERTERS = { + "archived": BooleanConverter(), + "left_width": Converter(int), + "visibility": EnumConverter(VISIBILITIES), + } + # Default is None if not specified here: + DEFAULTS = { + "archived": False, + "summary": "", + "visibility": "public", + "left_width": 8, + "avatar": "", + } + LISTS = [] + + def __init__(self, hub): + self.hub = hub + self.db = object_session(hub) + + def __getitem__(self, key): + if key not in self.KEYS: + raise KeyError + converter = self.CONVERTERS.get(key, Converter()) + query = self.db.query(HubConfig.value).filter_by(hub=self.hub, key=key) + if key in self.LISTS: + return [ + converter.from_db(r[0]) + for r in query.order_by(HubConfig.value) + ] + else: + try: + return converter.from_db(query.one()[0]) + except sa.orm.exc.NoResultFound: + return self.DEFAULTS.get(key) + # Raise an exception on MultipleResultsFound, this should not + # happen if the key is not in LISTS. + + def __setitem__(self, key, value): + converter = self.CONVERTERS.get(key, Converter()) + if key in self.LISTS: + self.db.query(HubConfig).filter_by(hub=self.hub, key=key).delete() + for item in value: + self.db.add(HubConfig( + hub=self.hub, key=key, value=converter.to_db(item))) + else: + value = converter.to_db(value) + try: + config = self.db.query(HubConfig).filter_by( + hub=self.hub, key=key).one() + except sa.orm.exc.NoResultFound: + config = self.db.add(HubConfig( + hub=self.hub, key=key, value=value)) + else: + config.value = value + self.db.flush() + + def __delitem__(self, key): + self.db.query(HubConfig).filter_by(hub=self.hub, key=key).delete() + + def __iter__(self): + return self.KEYS.__iter__() + + def __len__(self): + return len(self.KEYS) + + def to_dict(self): + result = {} + for conf in self.db.query(HubConfig).filter_by(hub=self.hub): + if conf.key in self.LISTS: + if conf.key not in result: + result[conf.key] = [] + result[conf.key].append(conf.value) + else: + result[conf.key] = conf.value + # Add defaults + for key in self.KEYS: + if key not in result: + if key in self.LISTS: + result[key] = [] + else: + result[key] = self.DEFAULTS.get(key) + return result + + # Methods below are not necessary but are optimizations + + def items(self): + # Avoid making multiple DB queries. + return self.to_dict().items() + + def values(self): + # Avoid making multiple DB queries. + return self.to_dict().values() + + def clear(self): + self.db.query(HubConfig).filter_by(hub=self.hub).delete() + + def __contains__(self, key): + # Avoid calling __getitem__ + return key in self.KEYS + + +class HubConfig(BASE): + + __tablename__ = 'hubs_config' + + id = sa.Column(sa.Integer, primary_key=True) + hub_id = sa.Column( + sa.String(50), sa.ForeignKey('hubs.name'), index=True, nullable=False) + key = sa.Column(sa.String(256), index=True, nullable=False) + value = sa.Column(sa.Text, index=True, nullable=False) class SpecificDefaultDict(defaultdict): @@ -454,7 +573,7 @@ class Widget(ObjectAuthzMixin, BASE): def _get_auth_permission_name(self, action): if action != "view": return self.hub._get_auth_permission_name(action) - hub_visibility = self.hub.config.visibility + hub_visibility = self.hub.config["visibility"] if hub_visibility != "preview": return "hub.{}.view".format(hub_visibility) return "widget.{}.view".format(self.visibility) diff --git a/hubs/static/client/app/components/CobWeb/index.js b/hubs/static/client/app/components/CobWeb/index.js index 582b494..4333f20 100644 --- a/hubs/static/client/app/components/CobWeb/index.js +++ b/hubs/static/client/app/components/CobWeb/index.js @@ -11,7 +11,7 @@ export default class CobWeb extends React.Component { const old_limit = Date.now() - (1000 * 86400 * 31); // 31 days let icon = null, msg = null; - if (this.props.hub.archived) { + if (this.props.hub.config.archived) { icon = ArchivedIcon; msg = "This hub has been archived and locked."; } else if (this.props.hub.mtime < old_limit) { diff --git a/hubs/static/client/app/components/HubHeader.js b/hubs/static/client/app/components/HubHeader.js index fe1e2ee..e4a6fd7 100644 --- a/hubs/static/client/app/components/HubHeader.js +++ b/hubs/static/client/app/components/HubHeader.js @@ -13,6 +13,7 @@ import "./HubHeader.css"; class HubHeader extends React.Component { render() { + const right_width = 12 - (this.props.hub.config.left_width || 8); return (
{ this.props.isLoading && @@ -45,7 +46,7 @@ class HubHeader extends React.Component {
-
+
{ this.props.hub.perms.config &&
diff --git a/hubs/static/client/app/components/WidgetsArea.js b/hubs/static/client/app/components/WidgetsArea.js index 5dcbc18..ec712b7 100644 --- a/hubs/static/client/app/components/WidgetsArea.js +++ b/hubs/static/client/app/components/WidgetsArea.js @@ -21,6 +21,7 @@ class WidgetsArea extends React.PureComponent { if (!this.props.hub.name) { return null; } + const right_width = 12 - (this.props.hub.config.left_width || 8); return (
{ this.props.widgets.isLoading && @@ -48,7 +49,7 @@ class WidgetsArea extends React.PureComponent { />
-
+
' % (hub.config.avatar) + if hub.config["avatar"] != "": + return '' % (hub.config["avatar"]) return ("
" "%s
") % (hubname2monogramcolour(hub.name), hubname2monogramcolour(hub.name), diff --git a/hubs/utils/views.py b/hubs/utils/views.py index ea6038e..2e305e7 100644 --- a/hubs/utils/views.py +++ b/hubs/utils/views.py @@ -10,8 +10,7 @@ import logging import flask from six.moves.urllib import parse as urlparse -from sqlalchemy import or_ -from sqlalchemy.orm import joinedload +from sqlalchemy import or_, and_ from sqlalchemy.orm.exc import NoResultFound from hubs.models import Hub, HubConfig, Widget @@ -20,11 +19,9 @@ from hubs.models import Hub, HubConfig, Widget log = logging.getLogger(__name__) -def get_hub(name, load_config=False): +def get_hub(name): """ Utility shorthand to get a hub and 404 if not found. """ query = Hub.query.filter(Hub.name == name) - if load_config: - query = query.options(joinedload(Hub.config)) try: return query.one() except NoResultFound: @@ -32,9 +29,15 @@ def get_hub(name, load_config=False): def query_hubs(querystring): - query = Hub.query.join(HubConfig) - query = query.filter(or_(HubConfig.summary.ilike('%%%s%%' % querystring), - Hub.name.ilike('%%%s%%' % querystring))) + query = Hub.query.join(HubConfig).filter( + or_( + Hub.name.ilike('%{}%'.format(querystring)), + and_( + HubConfig.key == "summary", + HubConfig.value.ilike('%{}%'.format(querystring)), + ) + ) + ) return query.all() @@ -272,7 +275,7 @@ def require_hub_access(action, url_param="name", json=False): @functools.wraps(function) def wrapper(*args, **kwargs): hub_name = kwargs[url_param] - hub = get_hub(hub_name, load_config=True) + hub = get_hub(hub_name) check_hub_access(hub, action, json) return function(*args, **kwargs) return wrapper diff --git a/hubs/views/api/hub_config.py b/hubs/views/api/hub_config.py index 4da5d95..08444c8 100644 --- a/hubs/views/api/hub_config.py +++ b/hubs/views/api/hub_config.py @@ -25,7 +25,7 @@ def api_hub_config(name): hub = get_hub(name) if flask.request.method == 'PUT': check_hub_access(hub, "config", json=True) - old_config = hub.config.__json__() + old_config = hub.config.to_dict() request_data = flask.request.get_json() if request_data is None: return flask.jsonify({ @@ -66,8 +66,8 @@ def hub_config_put_config(hub, config): return value validator = RequestValidator(dict( - # Only allow the parameters listed in __json__(). - (key, None) for key in hub.config.__json__().keys() + # Only allow the parameters listed in the hub config. + (key, None) for key in hub.config.keys() )) validator.converters["chat_domain"] = _validate_chat_domain try: @@ -79,7 +79,7 @@ def hub_config_put_config(hub, config): # Now set the configuration values. for key, value in values.items(): - setattr(hub.config, key, value) + hub.config[key] = value def hub_config_put_users(hub, user_roles): diff --git a/hubs/views/hub.py b/hubs/views/hub.py index cc4025a..b683c70 100644 --- a/hubs/views/hub.py +++ b/hubs/views/hub.py @@ -13,10 +13,10 @@ from hubs.utils.views import ( @require_hub_access("view") @login_required def hub(name): - hub = get_hub(name, load_config=True) + hub = get_hub(name) global_config = { "chat_networks": app.config["CHAT_NETWORKS"], - "hub_visibility": hubs.models.HubConfig.VISIBILITY, + "hub_visibility": hubs.models.VISIBILITIES, "roles": ["owner", "member"], } urls = { diff --git a/hubs/widgets/halp/utils.py b/hubs/widgets/halp/utils.py index 23ffec8..47c514d 100644 --- a/hubs/widgets/halp/utils.py +++ b/hubs/widgets/halp/utils.py @@ -17,9 +17,10 @@ def find_hubs_for_msg(msg): return [ h[0] for h in Hub.query.join(HubConfig).filter( - HubConfig.chat_channel == msg["channel"] + HubConfig.key == "chat_channel", + HubConfig.value == msg["channel"] ).values(Hub.name) - ] + ] def listofhubs_validator(value): diff --git a/hubs/widgets/rules/__init__.py b/hubs/widgets/rules/__init__.py index 12e830e..e69ce0f 100644 --- a/hubs/widgets/rules/__init__.py +++ b/hubs/widgets/rules/__init__.py @@ -65,9 +65,10 @@ class BaseView(RootWidgetView): 'https://lists.fedoraproject.org/archives/list/{}@' 'lists.fedoraproject.org/').format(instance.hub.name) irc_channel = irc_network = None - if instance.hub.config.chat_channel: - irc_channel = instance.hub.config.chat_channel - irc_network = instance.hub.config.chat_domain + hub_config = instance.hub.config + if hub_config["chat_channel"]: + irc_channel = hub_config["chat_channel"] + irc_network = hub_config["chat_domain"] return dict( oldest_owners=oldest_owners, owners=owners, diff --git a/populate.py b/populate.py index 88c4337..d48c4e3 100755 --- a/populate.py +++ b/populate.py @@ -39,10 +39,13 @@ for username in users: db.commit() # ############# Internationalizationteam -hub = hubs.models.Hub(name='i18n', archived=True) +hub = hubs.models.Hub(name='i18n') db.add(hub) -db.add(hubs.models.HubConfig( - hub=hub, summary='The Internationalization Team', avatar=placekitten)) +hub.config.update(dict( + summary='The Internationalization Team', + avatar=placekitten, + archived=True, +)) widget = hubs.models.Widget( plugin='rules', index=1, _config=json.dumps({ @@ -94,8 +97,7 @@ db.commit() # ############# CommOps hub = hubs.models.Hub(name='commops') db.add(hub) -db.add(hubs.models.HubConfig( - hub=hub, summary='The Fedora Community Operations Team')) +hub.config["summary"] = 'The Fedora Community Operations Team' widget = hubs.models.Widget( plugin='rules', index=1, _config=json.dumps({ @@ -144,8 +146,7 @@ db.commit() # ############# Marketing team hub = hubs.models.Hub(name='marketing') db.add(hub) -db.add(hubs.models.HubConfig( - hub=hub, summary='The Fedora Marketing Team')) +hub.config["summary"] = 'The Fedora Marketing Team' widget = hubs.models.Widget( plugin='rules', index=1, _config=json.dumps({ @@ -201,8 +202,7 @@ db.commit() # ############# Design team hub = hubs.models.Hub(name='designteam') db.add(hub) -db.add(hubs.models.HubConfig( - hub=hub, summary='The Fedora Design Team')) +hub.config["summary"] = 'The Fedora Design Team' widget = hubs.models.Widget( plugin='rules', index=1, _config=json.dumps({ @@ -255,8 +255,7 @@ db.commit() # ############# Infra team hub = hubs.models.Hub(name='infrastructure') db.add(hub) -db.add(hubs.models.HubConfig( - hub=hub, summary='The Fedora Infra Team')) +hub.config["summary"] = 'The Fedora Infra Team' widget = hubs.models.Widget( plugin='rules', index=1, _config=json.dumps({