#2 Module table
Merged 8 years ago by clime. Opened 8 years ago by frostyx.
https://github.com/FrostyX/copr.git module-table  into  master

[frontend] ensure that module.id will be known
Jakub Kadlčík • 8 years ago  
[frontend] permissions should be also checked here
Jakub Kadlčík • 8 years ago  
[frontend] specify how to create module table, not generate it from module
Jakub Kadlčík • 8 years ago  
[frontend] remove owner records from module table
Jakub Kadlčík • 8 years ago  
[frontend] group support for modules
Jakub Kadlčík • 8 years ago  
[frontend] validate unique user/nvr in forms
Jakub Kadlčík • 8 years ago  
[frontend] add method for querying module by user/nvr
Jakub Kadlčík • 8 years ago  
[frontend] refactor and document the add_module_table migration script
Jakub Kadlčík • 8 years ago  
[frontend] use module table
Jakub Kadlčík • 8 years ago  
[frontend] module release is of the type int now
Jakub Kadlčík • 8 years ago  
[frontend] create proper module table
Jakub Kadlčík • 8 years ago  
frontend/coprs_frontend/alembic/versions/3fdedd58ac73_add_module_table.py
file added
+94
@@ -0,0 +1,94 @@

+ """add module table

+ 

+ Revision ID: 3fdedd58ac73

+ Revises: 414a86b37a0f

+ Create Date: 2016-10-26 22:01:09.361070

+ 

+ """

+ 

+ # revision identifiers, used by Alembic.

+ revision = '3fdedd58ac73'

+ down_revision = '414a86b37a0f'

+ 

+ from alembic import op

+ import sqlalchemy as sa

+ 

+ from coprs.models import Module, Action, Copr, User, Group

+ from sqlalchemy.orm import sessionmaker

+ 

+ import json

+ import base64

+ import modulemd

+ from coprs.logic.coprs_logic import CoprsLogic

+ from coprs.logic.actions_logic import ActionsLogic

+ from coprs.helpers import ActionTypeEnum

+ 

+ 

+ def upgrade():

+     bind = op.get_bind()

+     Session = sessionmaker()

+     session = Session(bind=bind)

+ 

+     op.create_table(

+         "module",

+         sa.Column("id", sa.Integer, primary_key=True),

+         sa.Column("name", sa.String(100), nullable=False),

+         sa.Column("version", sa.String(100), nullable=False),

+         sa.Column("release", sa.Integer, nullable=False),

+         sa.Column("summary", sa.String(100), nullable=False),

+         sa.Column("description", sa.Text),

+         sa.Column("created_on", sa.Integer, nullable=True),

+         sa.Column("yaml_b64", sa.Text),

+         sa.Column("copr_id", sa.Integer, sa.ForeignKey("copr.id")),

+     )

+     op.create_unique_constraint("pk_module", "module",

+                                 ["name", "version", "release", "copr_id"])

+     session.commit()

+ 

+     # Now, let's seed the table with existing modules which are violently stored in the `action` table

+     added_modules = set()

+     for action in ActionsLogic.get_many(ActionTypeEnum("build_module")).order_by(Action.id.desc()):

+         data = json.loads(action.data)

+         copr = get_copr(session, data["ownername"], data["projectname"])

+         yaml = base64.b64decode(data["modulemd_b64"])

+         mmd = modulemd.ModuleMetadata()

+         mmd.loads(yaml)

+ 

+         module_kwargs = {

+             "name": mmd.name,

+             "version": mmd.version,

+             "release": mmd.release,

+             "summary": mmd.summary,

+             "description": mmd.description,

+             "yaml_b64": data["modulemd_b64"],

+             "created_on": action.created_on,

+             "copr_id": copr.id,

+         }

+ 

+         # There is no constraint for currently existing modules, but in new table, there

+         # must be unique (copr, nvr). Therefore in the case of duplicit modules,

+         # we will add only the newest one

+         if full_module_name(copr, mmd) in added_modules:

+             print("Skipping {}; Already exists".format(full_module_name(copr, mmd)))

+             continue

+ 

+         session.add(Module(**module_kwargs))

+         added_modules.add(full_module_name(copr, mmd))

+ 

+ 

+ def downgrade():

+     ### commands auto generated by Alembic - please adjust! ###

+     op.drop_table('module')

+     ### end Alembic commands ###

+ 

+ 

+ def full_module_name(copr, mmd):

+     return "{}/{}-{}-{}".format(copr.full_name, mmd.name, mmd.version, mmd.release)

+ 

+ 

+ def get_copr(session, ownername, projectname):

+     if ownername[0] == "@":

+         coprs = CoprsLogic.filter_by_group_name(session.query(Copr), ownername[1:])

+     else:

+         coprs = CoprsLogic.filter_by_user_name(session.query(Copr), ownername)

+     return CoprsLogic.filter_by_name(coprs, projectname).first()

frontend/coprs_frontend/coprs/forms.py
file modified
+14 -1
@@ -8,12 +8,14 @@

  from flask_wtf.file import FileAllowed, FileRequired, FileField

  

  from flask.ext import wtf

+ from jinja2 import Markup

  

  from coprs import constants

  from coprs import helpers

  from coprs import models

  from coprs.logic.coprs_logic import CoprsLogic

  from coprs.logic.users_logic import UsersLogic

+ from coprs.logic.modules_logic import ModulesLogic

  from coprs.models import Package

  from exceptions import UnknownSourceTypeException

  
@@ -733,16 +735,27 @@

  class CreateModuleForm(wtf.Form):

      name = wtforms.StringField("Name")

      version = wtforms.StringField("Version")

-     release = wtforms.StringField("Release")

+     release = wtforms.IntegerField("Release")

      filter = wtforms.FieldList(wtforms.StringField("Package Filter"))

      api = wtforms.FieldList(wtforms.StringField("Module API"))

      profile_names = wtforms.FieldList(wtforms.StringField("Install Profiles"), min_entries=2)

      profile_pkgs = wtforms.FieldList(wtforms.FieldList(wtforms.StringField("Install Profiles")), min_entries=2)

  

+     def __init__(self, copr=None, *args, **kwargs):

+         self.copr = copr

+         super(CreateModuleForm, self).__init__(*args, **kwargs)

+ 

      def validate(self):

          if not wtf.Form.validate(self):

              return False

  

+         # User/nvr should be unique
clime commented 8 years ago

Obsoleted comment?

+         module = ModulesLogic.get_by_nvr(self.copr, self.name.data, self.version.data, self.release.data).first()

+         if module:

+             self.errors["nvr"] = [Markup("Module <a href='{}'>{}</a> already exists".format(

+                 helpers.copr_url("coprs_ns.copr_module", module.copr, id=module.id), module.full_name))]

+             return False

+ 

          # Profile names should be unique

          names = filter(None, self.profile_names.data)

          if len(set(names)) < len(names):

frontend/coprs_frontend/coprs/logic/actions_logic.py
file modified
+3 -6
@@ -237,7 +237,7 @@

          db.session.add(action)

  

      @classmethod

-     def send_build_module(cls, user, copr, modulemd):

+     def send_build_module(cls, user, copr, module):

          """

          :type copr: models.Copr

          :type modulemd: str content of module yaml file
@@ -246,17 +246,14 @@

          if not user.can_build_in(copr):

              raise exceptions.InsufficientRightsException("You don't have permissions to build in this copr.")

  

-         modulemd_b64 = base64.b64encode(modulemd)

          data = {

-             "ownername": copr.owner_name,

-             "projectname": copr.name,

              "chroots": [c.name for c in copr.active_chroots],

-             "modulemd_b64": modulemd_b64,

          }

  

          action = models.Action(

              action_type=helpers.ActionTypeEnum("build_module"),

-             object_type="copr",

+             object_type="module",

+             object_id=module.id,

              old_value="",

              new_value="",

              data=json.dumps(data),

frontend/coprs_frontend/coprs/logic/modules_logic.py
file modified
+36 -13
@@ -1,28 +1,51 @@

+ import time

+ import base64

+ import modulemd

+ from sqlalchemy import and_

  from coprs import models

- from coprs import helpers

- 

- 

- ACTION_TYPE = helpers.ActionTypeEnum("build_module")

+ from coprs import db

+ from coprs import exceptions

  

  

  class ModulesLogic(object):

-     """

-     @FIXME ?

-     Module builds are currently stored as Actions, so we are querying subset of Action objects

-     """

- 

      @classmethod

      def get(cls, module_id):

          """

          Return single module identified by `module_id`

          """

-         return models.Module.query.filter(models.Action.id == module_id)

+         return models.Module.query.filter(models.Module.id == module_id)

+ 

+     @classmethod

+     def get_by_nvr(cls, copr, name, version, release):

+         return models.Module.query.filter(

+             and_(models.Module.name == name,

+                  models.Module.version == version,

+                  models.Module.release == release,

+                  models.Module.copr_id == copr.id))

  

      @classmethod

      def get_multiple(cls):

-         return models.Module.query.filter(models.Module.action_type == ACTION_TYPE).order_by(models.Module.id.desc())

+         return models.Module.query.order_by(models.Module.id.desc())

  

      @classmethod

      def get_multiple_by_copr(cls, copr):

-         return filter(lambda m: m.ownername == copr.owner_name and m.projectname == copr.name,

-                       cls.get_multiple())

+         return cls.get_multiple().filter(models.Module.copr_id == copr.id)

+ 

+     @classmethod

+     def from_modulemd(cls, yaml):

+         mmd = modulemd.ModuleMetadata()

+         mmd.loads(yaml)

+         return models.Module(name=mmd.name, version=mmd.version, release=mmd.release, summary=mmd.summary,

+                              description=mmd.description, yaml_b64=base64.b64encode(yaml))

+ 

+     @classmethod

+     def add(cls, user, copr, module):

+         if not user.can_build_in(copr):

+             raise exceptions.InsufficientRightsException("You don't have permissions to build in this copr.")

+ 

+         module.copr_id = copr.id

+         module.copr = copr

+         module.created_on = time.time()

+ 

+         db.session.add(module)

+         return module

frontend/coprs_frontend/coprs/models.py
file modified
+53 -22
@@ -7,7 +7,6 @@

  import base64

  import modulemd

  

- from sqlalchemy import orm

  from sqlalchemy.ext.associationproxy import association_proxy

  from libravatar import libravatar_url

  import zlib
@@ -1043,6 +1042,19 @@

          return "Action {0} on {1}, old value: {2}, new value: {3}.".format(

              self.action_type, self.object_type, self.old_value, self.new_value)

  

+     def to_dict(self, **kwargs):

+         d = super(Action, self).to_dict()

+         if d.get("object_type") == "module":

+             module = Module.query.filter(Module.id == d["object_id"]).first()

+             data = json.loads(d["data"])

+             data.update({

+                 "projectname": module.copr.name,

+                 "ownername": module.copr.owner_name,

+                 "modulemd_b64": module.yaml_b64,

+             })

+             d["data"] = json.dumps(data)

+         return d

+ 

  

  class Krb5Login(db.Model, helpers.Serializer):

      """
@@ -1095,26 +1107,45 @@

          return "{} (fas: {})".format(self.name, self.fas_name)

  

  

- class Module(Action):

-     """

-     Wrapper class for representing modules

-     Module builds are currently stored as Actions, so we are querying properties from action data

-     """

-     @orm.reconstructor

-     def init_on_load(self):

-         data = json.loads(self.data)

-         yaml = base64.b64decode(json.loads(self.data)["modulemd_b64"])

+ class Module(db.Model, helpers.Serializer):

+     id = db.Column(db.Integer, primary_key=True)

+     name = db.Column(db.String(100), nullable=False)

+     version = db.Column(db.String(100), nullable=False)

+     release = db.Column(db.Integer, nullable=False)

+     summary = db.Column(db.String(100), nullable=False)

+     description = db.Column(db.Text)

+     created_on = db.Column(db.Integer, nullable=True)

+ 

+     # When someone submits YAML (not generate one on the copr modules page), we might want to use that exact file.

+     # Yaml produced by deconstructing into pieces and constructed back can look differently,

+     # which is not desirable (Imo)

+     #

+     # Also if there are fields which are not covered by this model, we will be able to add them in the future

+     # and fill them with data from this blob

+     yaml_b64 = db.Column(db.Text)

+ 

+     # relations

+     copr_id = db.Column(db.Integer, db.ForeignKey("copr.id"))

+     copr = db.relationship("Copr", backref=db.backref("modules"))

  

+     @property

+     def yaml(self):

+         return base64.b64decode(self.yaml_b64)

+ 

+     @property

+     def modulemd(self):

          mmd = modulemd.ModuleMetadata()

-         mmd.loads(yaml)

- 

-         attrs = ["name", "version", "release", "summary"]

-         for attr in attrs:

-             setattr(self, attr, getattr(mmd, attr))

- 

-         self.ownername = data["ownername"]

-         self.projectname = data["projectname"]

-         self.modulemd = mmd

-         self.yaml = mmd.dumps()

-         self.full_name = "-".join([self.name, self.version, self.release])

-         self.status = self.result

+         mmd.loads(self.yaml)

+         return mmd

+ 

+     @property

+     def nvr(self):

+         return "-".join([self.name, self.version, str(self.release)])

+ 

+     @property

+     def full_name(self):

+         return "{}/{}".format(self.copr.owner_name, self.nvr)

+ 

+     @property

+     def action(self):

+         return Action.query.filter(Action.object_type == "module").filter(Action.object_id == self.id).first()

frontend/coprs_frontend/coprs/templates/_helpers.html
file modified
+5 -1
@@ -133,7 +133,11 @@

  {% endmacro %}

  

  {% macro module_state(module) %}

-   {{ build_state_text(module.status | module_state_from_num) }}

+   {% if module.action %}

+     {{ build_state_text(module.action.result | module_state_from_num) }}

+   {% else %}

+     -

+   {% endif %}

  {% endmacro %}

  

  {% macro chroot_to_os_logo(chroot) %}

frontend/coprs_frontend/coprs/templates/coprs/create_module.html
file modified
+1
@@ -45,6 +45,7 @@

                  <div class="form-group col-sm-4">

                    <label for="moduleName">Name:</label>

                    <input type="text" class="form-control" id="moduleName" placeholder="{{ copr.name }}" disabled>

+                   <input type="hidden" value="{{ copr.name }}" name="name">

                  </div>

                  <div class="form-group col-sm-4">

                    <label for="moduleVersion">Version:</label>

frontend/coprs_frontend/coprs/templates/coprs/detail/module.html
file modified
+1 -1
@@ -18,7 +18,7 @@

  

  {% block detail_body %}

  

- {% if copr.owner_name != module.ownername or copr.name != module.projectname %}

+ {% if copr.id != module.copr.id %}

    <h2 class="build-detail"> Module {{ module.id }} doesn't belong to this project. </h2>

    <p> It belongs to {{ module.ownername }}/{{ module.projectname }} </p>

  {% else %}

frontend/coprs_frontend/coprs/templates/coprs/detail/modules.html
file modified
+1 -1
@@ -34,7 +34,7 @@

      </p>

      {% if g.user and g.user.can_build_in(copr) %}

      <div class="blank-slate-pf-main-action">

-         <a class="btn btn-primary btn-lg" href="{{ copr_url('coprs_ns.copr_add_build', copr) }}"> Create a New Module </a>

+         <a class="btn btn-primary btn-lg" href="{{ copr_url('coprs_ns.copr_create_module', copr) }}"> Create a New Module </a>

      </div>

      {% endif %}

  </div>

frontend/coprs_frontend/coprs/views/api_ns/api_general.py
file modified
+16 -7
@@ -4,6 +4,7 @@

  import json

  import os

  import flask

+ import sqlalchemy

  

  from werkzeug import secure_filename

  
@@ -18,6 +19,7 @@

  from coprs.logic.complex_logic import ComplexLogic

  from coprs.logic.users_logic import UsersLogic

  from coprs.logic.packages_logic import PackagesLogic

+ from coprs.logic.modules_logic import ModulesLogic

  

  from coprs.views.misc import login_required, api_login_required

  
@@ -856,11 +858,18 @@

          raise LegacyApiError(form.errors)

  

      modulemd = form.modulemd.data.read()

-     ActionsLogic.send_build_module(flask.g.user, copr, modulemd)

-     db.session.commit()

+     module = ModulesLogic.from_modulemd(modulemd)

+     try:

+         module = ModulesLogic.add(flask.g.user, copr, module)

+         db.session.flush()

+         ActionsLogic.send_build_module(flask.g.user, copr, module)

+         db.session.commit()

  

-     return flask.jsonify({

-         "output": "ok",

-         "message": "Module build was submitted",

-         "modulemd": modulemd,

-     })

+         return flask.jsonify({

+             "output": "ok",

+             "message": "Module build was submitted",

+             "modulemd": modulemd,

+         })

+ 

+     except sqlalchemy.exc.IntegrityError:

+         raise LegacyApiError({"nvr": ["Module {} already exists".format(module.nvr)]})

frontend/coprs_frontend/coprs/views/coprs_ns/coprs_general.py
file modified
+7 -5
@@ -895,8 +895,8 @@

  

  

  def render_copr_modules(copr):

-     query = ModulesLogic.get_multiple_by_copr(copr=copr)

-     return flask.render_template("coprs/detail/modules.html", copr=copr, modules=query)

+     modules = ModulesLogic.get_multiple_by_copr(copr=copr).all()

+     return flask.render_template("coprs/detail/modules.html", copr=copr, modules=modules)

  

  

  @coprs_ns.route("/<username>/<coprname>/create_module/")
@@ -922,7 +922,7 @@

  @login_required

  @req_with_copr

  def copr_create_module_post(copr):

-     form = forms.CreateModuleForm(csrf_enabled=False)

+     form = forms.CreateModuleForm(copr=copr, csrf_enabled=False)

      args = [copr, form]

      if "add_profile" in flask.request.values:

          return add_profile(*args)
@@ -966,7 +966,9 @@

          for package in packages:

              mmd.profiles[name].add_rpm(package)

  

-     actions_logic.ActionsLogic.send_build_module(flask.g.user, copr, mmd.dumps())

+     module = ModulesLogic.add(flask.g.user, copr, ModulesLogic.from_modulemd(mmd.dumps()))

+     db.session.flush()

+     actions_logic.ActionsLogic.send_build_module(flask.g.user, copr, module)

      db.session.commit()

      flask.flash("Modulemd yaml file successfully generated and submitted to be build", "success")

      return flask.redirect(url_for_copr_details(copr))
@@ -992,5 +994,5 @@

      response = flask.make_response(module.yaml)

      response.mimetype = "text/plain"

      response.headers["Content-Disposition"] = \

-         "filename={}.yaml".format("-".join([str(module.id), module.name, module.version, module.release]))

+         "filename={}.yaml".format("-".join([str(module.id), module.name, module.version, str(module.release)]))

      return response

no initial comment

Copr moved from GitHub to Pagure during this pull request. Please see the previous conversation here https://github.com/fedora-copr/copr/pull/26

My main objection is that you don't need those two unique constraints on (module, user) and (module, group) and you don't need to keep the owner records in the table. It is good enough to have just copr_id in the table with a single unique constraint (module, copr). Module owner is the same as the project owner.

I also thought about it this way, but I am not sure if is it really good enough. See, following scenario:

user1/copr1      --->    module1
user1/copr2      --->    module1

In case of primary key on (nvr, copr_id) this was be possible to store, but it should not be. Because AFAIK module in Copr should be identified by username/modulenvr or @groupname/modulenvr so not in the distant future I might want to enable module on my system by user1/module1 which would end in conflict.

If course we can check it on application level instead of database and not allow to add such conflicting modules, but personally I think that having user_id and group_id in module table is better approach. If you can see some reason why it would be better the other way, I can change it of course.

My main objection is that you don't need those two unique constraints on (module, user) and (module, group) and you don't need to keep the owner records in the table. It is good enough to have just copr_id in the table with a single unique constraint (module, copr). Module owner is the same as the project owner.

I also thought about it this way, but I am not sure if is it really good enough. See, following scenario:
user1/copr1 ---> module1
user1/copr2 ---> module1

In case of primary key on (nvr, copr_id) this was be possible to store, but it should not be. Because AFAIK module in Copr should be identified by username/modulenvr or @groupname/modulenvr so not in the distant future I might want to enable module on my system by user1/module1 which would end in conflict.
If course we can check it on application level instead of database and not allow to add such conflicting modules, but personally I think that having user_id and group_id in module table is better approach. If you can see some reason why it would be better the other way, I can change it of course.

What do you mean by "conflict". Client will simply say there are two matching modules, one being owner/projectx/module1, the second being owner/projecty/module2 and let the user pick.

By the way, nowadays, if you try to install a package present in multiple (enabled) repos with the same name-version-release, dnf itself picks one of them and installs it.

4 new commits added

  • [frontend] ensure that module.id will be known
  • [frontend] permissions should be also checked here
  • [frontend] specify how to create module table, not generate it from module
  • [frontend] remove owner records from module table
8 years ago

Ok, my mistake, I reconsidered it again and you seem to be right. I lived with premise in my head that module will be installed to a system with something like

dnf module enable <user>/<modulenvr>

and therefore there need to be unique user/modulenvr across all user's projects. Which is not true, actually. @asamalik corrected me that it will be done as following

dnf copr enable <user>/<project>   # Same command for enabling repos that as we already use
dnf module enable <modulenvr>

Which is basically the same concept as enabling regular repos and installing packages from them. I don't know why I missed that, sorry.

I've removed user and group records from the module table and did some small enhancements. If it looks good to you, please merge :-)

Pull-Request has been merged by clime

8 years ago