Look and fix some DB model issues

[lw]

I think some DB models (i.e. those classes in cms.db) have some strange fields definitions. For example the User.ip field is nullable but we usually use the '0.0.0.0' string to mean "no IP address assigned". Likewise User.email is almost never used, thus left blank, but it's not nullable. I suspect other such issues are around, even of other kinds. We should take a look at all models and fix the ones we find.

[giomasce]

I'm of course in favor, in general, to such changes and agree that the two cases you indicate are to be fixed.

[bblackham]

Can I suggest for User.ip, that instead of hard-coding '0.0.0.0', we actually just use NULL or a specific IP(v4 or v6) address? (i.e. make NULL the new '0.0.0.0', as '0.0.0.0' is only ever treated as a magic string).

[bblackham]

My proposed fix for the User.ip issue is here: bblackham/cms@a102720

[lw]

Seems good. I suggest to merge it after the release of v1.0, which should happen very soon.

Just a quick note: there's no UpdateDB script... not sure if we want one ;-)

[bblackham]

Yep, post definitely 1.0.

UpdateDB script would be trivial, but probably completely useless. (New contests wouldn't run with old IP locks). I don't think we should worry :)

[lw]

I'm putting this here, even if it's not strictly related: the DB allows Contest.start and .stop to be nullable, and assign to these values a meaning of -infinity and +infinity (respectively). I don't think that all of the code is aware of these cases...

[lw]

The current problems have been fixed in https://github.com/cms-dev/cms/commit/483ee85965f527cbc459ebe7e010c6854661b6eb, https://github.com/cms-dev/cms/commit/e051ee4d2667ba381a24c7b1764a6d9c3d792b45 and https://github.com/cms-dev/cms/commit/d66951d3149a954fb0b81b6015e8e0b060020152. I keep this issue open to remember to look if I can find more...

[lw]

TODO: rename the table name of Testcases from "task_testcases" to "testcases".

[lw]

TODO: check that nobody is using Task.scorer and remove it.

[lw]

TODO: try to use a SmartMappedCollection for Contest.users and Contest.tasks (indexed by username and task name respectively).

[lw]

TODO: move Submission.LANGUAGES and Submission.LANGUAGES_MAP out of the DB classes since it has nothing to do with the DB... for example in cms/__init__.py.

[lw]

TODO: now that UpdateDB has been removed I think we don't need to give names to UniqueConstraints (and other constraints) anymore; we can remove them.

[lw]

TODO: use a proper many-to-many relationship for Task. and User.primary_statements. With the new ContestExporter/Importer it should be a piece of cake.

[lw]

@bblackham I was reading this discussion again and I'm not getting anymore the reason we don't need an UpdateDB script for the User.ip change (which, at this point, would be an UpdateDump script). Wouldn't it be useful to convert the '0.0.0.0' values into null? If we don't do that then reimported dumps won't work, right?

[bblackham]

@lerks Updating them wouldn't hurt, but I can't imagine a scenario where it would be useful. I figured re-imported dumps wouldn't run twice with the same users (nor with the same IP locks).

[lw]

Yes, we usually think of dumps as a way to "archive" contests after they've been run, or to re-run them again. But suppose one uses CMS to manage a long-running online repository of tasks: one could export just to update CMS and then import again. Sure, in both cases this particular change doesn't seem necessary (users will be changed on re-runs, and no IP locks will be in place for online contests) but I'd prefer to do it anyway.

[bblackham]

@lerks Cool, no objections from me.

[lw]

TODO: consider making (Submission|UserTest).get_result(_or_create)? use the active dataset if no one is given as argument.

[lw]

• [ ] Use @property (or other tools, i.e. TypeDecorator) to provide direct access to TaskTypes and ScoreTypes (both class and instance) on Dataset objects.
• [ ] Put an UniqueConstraint on Contest.name.
• [ ] Use usernames and task names as fields for PrimaryKeys.
• [ ] Split Relationships with backref into two attributes (one for each direction), and use back_populates instead.
• [ ] Using collection_class=ordering_list makes order_by useless.
• [ ] Use SQLAlchemy's version_id_col to avoid concurrent edits (maybe coupled with server_default for automatic column population). By that I mean: we may try to set a column last_modification, with type TIMESTAMP, automatically filled by PostgreSQL (with a trigger?) on each edit (even those made from outside SQLAlchemy). Then, an update will succeed only if the value on that column hasn't changed since the row was fetched.
• [ ] See if introducing @parent and @child decorators helps in making relationship discovery automatic (e.g. useful to have a generic reimporter).
• [ ] Same as above with @file on digest columns to auto-discover files.
• [ ] Introducing database indexes on submissions and/or submission results may speed up queries.