Don't take a database object, grab its ID, then use the ID to load the same database object once again. This also applies to creating AuthUser objects, which will happily take a User object instead of a user_id.
cleanup: don't check if currently logged-in user exists
Don't check if the currently logged-in User exists. Every request runs in a database transaction; if the object existed at the start, it still exists (unless we add a way for a user to delete themselves).
Rename the 'user_id' field to 'author_id' and replace other references to the comment 'user' throughout the model. The database column name 'user_id' remain unchanged for now; a later Alembic script can fix the name of these and other columns to match their Python name.
db: PullRequest/Repository/RepoGroup/UserGroup: change 'user' to 'owner'
Rename the 'user' and 'user_id' fields on the four classes to something more informative. The database column names remain unchanged for now; a later Alembic script can fix the name of these and other columns to match their Python name.
This might break rcextensions, though, and external scripts that use the HTML form interface.
Don't re-add objects to the SQLAlchemy Session just because they were modified. Session.add is only for freshly constructed objects that SQLAlchemy doesn't know about yet.
The rules are quite simple:
When creating a database object by calling the constructor directly, it must explicitly be added to the session.
When creating an object using a factory function (like "create_repo"), the returned object has already (by convention) been added to the session, and should not be added again.
When getting an object from the session (via Session.query or any of the utility functions that look up objects in the database), it's already added, and should not be added again. SQLAlchemy notices attribute modifications automatically for all objects it knows about.
db: do case-insensitive explicit sorting of RepoGroup names
This does not change the implicit sorting enabled via __mapper_args__, which is a bad idea anyway (and now deprecated), and will have to be dealt with in a later changeset.
The use of __mapper_args__ implicitly adds sorting to every query of RepoGroup objects throughout the code (including implicit queries via relationships). For the relationships, __mapper_args can be replaced with "order_by" on each individual relationship, and it's reasonably straight-forward to identify every RepoGroup query throughout the code, and add explicit sorting. But we don't really need that sorting most of the time, so a better way forward may be to identify all the places that actually needs the sorting, make it explicit there, and then kill the __mapper_args__. (Anyway, future work.)
db: always do case-insensitive sorting of repository names
We retain the implicit order_by on the follows_repository relationship. This is probably a bad idea, since it causes sorting even when it's not needed; but for now, at least, we consistently do case-insensitive sort.
This makes database query code more explicit and increases readability.
E.g. the function name get_pullrequest_cnt_for_user was bad, because the concept of "pullrequest for user" is incredibly vague, and could refer to any kind of association between PRs and users. (Quiz time! Does it mean that the user is the PR owner, that the user is reviewing, or that the user has commented on the PR and thus is receiving notifications?)
A descriptive name could be "get_open_pull_request_count_for_reviewer", because the function is indeed only concerned with reviewers and only with open pull requests. But at this point, we might as well say PullRequest.query(reviewer_id=user, include_closed=False).count() which is only slightly longer, and doesn't require us to write dozens of little wrapper functions (including, any moment now, a separate function for listing the PRs instead of counting them).
Note that we're not actually going down an abstraction level by doing this. We're still operating on the concepts of "pull request", "open" and "reviewer", and are not leaking database implementation details.
The query() shortcuts are designed so they default to not altering the query. Any processing requires explicit opt-in by the caller.
BaseRepoController already provides the repository object in c.db_repo (may be None) and BaseController already provides the repository name in c.repo_name (as given in the URL).
(Arguably, that's a bad design, and we should revisit that decision in the future. For now, the code just performs slightly better.)
This method saves basically no typing, compared to "query().all()". Additionally, "all()" returns a list, forcing all records to be loaded into a memory at the same time, but some callers just need to iterate over the objects one at a time, in which case "query()" alone is more efficient. In one case, the caller can even use "count()" and avoid loading any objects from the database at all.
Turbogears2 migration: replace pylons.url by kallithea.config.routing.url
In preparation for the migration to Turbogears2, introduce a kallithea.config.routing.url to replace pylons.url. The implementation is basically the same: wrap around routes.url().
This change involves: - a number of import statement changes - fixing some tests in test_libs.py; to avoid duplication, the different implementations of fake_url were grouped in one place.
This change was first proposed by Alessandro Molina in his initial port. Following changes were made afterwards: - move UrlGenerator from kallithea.lib.utils to kallithea.config.routing - add documentation to UrlGenerator - kallithea/lib/auth.py used url_for instead of url, for no apparent reason so this was changed. - fix libs tests - rebase onto Pylons-based Kallithea first
tests: fix assertion rewriting in some tests with pytest-3.0.0+
Since pytest 3.0.0, asserts present in modules that are not directly seen by pytest as 'test modules', are no longer rewritten to have improved reporting, unless they are explicitly marked as up-for-rewriting. Rationale from pytest upstream:
However since we do not want to test different bytecode then you will run in production this hook only re-writes test modules themselves as well as any modules which are part of plugins. Any other imported module will not be re-written and normal assertion behaviour will happen.
This is e.g. the case for asserts in files like kallithea/tests/api/api_base.py and kallithea/tests/models/common.py.
This commit registers all modules below kallithea.tests for assert rewriting, but note that asserts in kallithea/tests/__init__.py itself are not rewritten as kallithea.tests is already imported when the register statement is encountered. Moving the register statement to kallithea/__init__.py would fix that, but even then asserts in kallithea/tests/__init__.py seem not to be rewritten (let alone the issue that we do not want a pytest dependency enforced in kallithea/__init__.py which is also used in production). Moving the code from kallithea/tests/__init__.py to a proper module, as suggested by Mads Kiilerich, solves that problem.
tests: add as little code as possible in __init__.py
kallithea/tests/__init__.py contained quite a lot of code, including the test base class TestController. This in itself may be considered bad practice.
Specifically, this poses a problem when using pytest 3.0+, in which asserts in some files are not automatically rewritten to give improved assert output. That problem can be fixed by explicitly registering such files for assertion rewriting, but that register call should be executed _before_ said files are imported. I.e. if the register call is in kallithea/tests/__init__.py, assert calls in __init__.py itself can not be rewritten.
Since the TestController base class does effectively contain asserts, and we do not want to execute the register call from somewhere outside the kallithea/tests directory, we need to move the TestController class to another file (kallithea/tests/base.py) so we can have a register call in __init__.py before loading base.py.
While not strictly necessary to fix the mentioned pytest problem, we take the opportunity to fully clean __init__.py and move everything to the new kallithea/tests/base.py. While doing so, unnecessary imports are removed, and imports are ordered alphabetically. Explicit imports of symbols from modules that were already imported as a whole, are removed in favor of fully qualifying the references (e.g. tempfile._RandomNameSequence).
Turbogears2 migration: remove some references to Pylons in comments
In order to minimize the diff of the actual Turbogears2 migration, this commit already removes certain unnecessary references to Pylons from the Kallithea source base. Places where the reference to Pylons is important are still kept for now, as well as references in kallithea/config where many changes are made for Turbogears2 anyway.
Turbogears2 migration: use sqlalchemy.url iso sqlalchemy.db1.url
In Turbogears2, much of the application initialization is handled by the framework, whereas in Pylons the application was responsible for it. Initializing SQLAlchemy is one such part of initialization which is handled by Turbogears2.
Turbogears2 expects the configuration file to refer to the database using 'sqlalchemy.url' rather than the current 'sqlalchemy.db1.url'. While the exact name is not really important, not following this approach means we'll need to override the sqlalchemy initialization method.
Therefore, as a preparation to the Turbogears2 migration, already change the database reference string under Pylons. When upgrading to a version of Kallithea containing this commit, the .ini file will manually need to be adapted to remove the .db1 strings.
lib: simplify version; drop get_current_revision - it was run every time on import time - we don't want that
This functionality was already not in use with the version numbers we use. I also doubt it is possible to have version numbers where it worked - it seems like there might have been some regression that nobody noticed. Dropping it will thus not really lose any actual functionality.
If we want to include the revision number in the version (for browser cache invalidation) then we should do it in a different way. Perhaps writing it to some file from setup.py .
utils: when building a Mercurial ui object with configuration, don't use 'None' for NULL values
If Ui had an entry with ui_section='extensions', ui_key='largefiles', ui_value=NULL it would be passed to Mercurial as if the .ini file had [extensions] largefiles = None and it would fail to load the largefiles extension because it couldn't find './None/'.
Note: get_current_revision might currently mask this problem. It will not get the Ui from the database and will thus read a normal .ini file from the system and (if configured) read the largefiles extension from the default location. That will make the largefiles extension available for later largefiles imports even if they specify the bogus path.
As f8a714c2c5a1 noted in a FIXME: ui_value should perhaps not be nullable.
For now, just handle NULL in extension configuration.
helpers: introduce ascii preserving visual markup of *bold*
No matter what kind of text (code and markup) is parsed, it will at most have slightly odd styling. Nothing will be lost and it can be copy-pasted correctly.
Other kinds of markup (like _underline_ and /italic/) has also been considered ... but they will too often interfere with valid code and command snippets.
We inline it so we eventually can match all patterns in the same regexp and thus avoid problems with parsing of formatted html. Inlining it will also make repo_name and other parameters easily available.
helpers: in urlify_text, use <br> for newlines in pre-formatted text so it can be cut'n'pasted correctly
Before, when copying the preformatted nice looking ASCII-artsy text in a PR or changeset description from a browser to a text editor, it would be pasted as text without newline. Simply put, copy/paste ignores that it is inside a <pre> / white-space:pre-wrap section.
Instead, translate newlines to <br> which always translates to a newline when pasted.
celery: use Celery 3 config settings instead of deprecated
As warned by: The 'CELERYD_LOG_LEVEL' setting is scheduled for deprecation in version 2.4 and removal in version v4.0. Use the --loglevel argument instead remove celeryd.log.file and celeryd.log.level from the ini file. Instead, use: paster celeryd my.ini --loglevel=DEBUG --logfile=my.log or, in the future: gearbox celeryd -c my.ini -- --loglevel=DEBUG --logfile=my.log
As warned by: The 'BROKER_VHOST' setting is scheduled for deprecation in version 2.5 and removal in version v4.0. Use the BROKER_URL setting instead The 'BROKER_HOST' setting is scheduled for deprecation in version 2.5 and removal in version v4.0. Use the BROKER_URL setting instead The 'BROKER_USER' setting is scheduled for deprecation in version 2.5 and removal in version v4.0. Use the BROKER_URL setting instead The 'BROKER_PASSWORD' setting is scheduled for deprecation in version 2.5 and removal in version v4.0. Use the BROKER_URL setting instead The 'BROKER_PORT' setting is scheduled for deprecation in version 2.5 and removal in version v4.0. Use the BROKER_URL setting instead change the .ini template to use: broker.url = amqp://rabbitmq:qewqew@localhost:5672/rabbitmqhost
As warned by: Starting from version 3.2 Celery will refuse to accept pickle by default.
The pickle serializer is a security concern as it may give attackers the ability to execute any command. It's important to secure your broker from unauthorized access when using pickle, so we think that enabling pickle should require a deliberate action and not be the default choice.
If you depend on pickle then you should set a setting to disable this warning and to be sure that everything will continue working when you upgrade to Celery 3.2::
Celery 3 is more than 2 and is the future and should be more stable than Celery 2.
There might be problems like PRECONDITION_FAILED - cannot redeclare exchange 'celeryresults' in vhost 'kallithea' with different type, durable, internal or autodelete value when celery is upgraded and it tries to upgrade existing tasks.
One way to avoid that problem: - make sure celery has run to completion and there are no important pending tasks, - delete the old vhost, - upgrade celery, - create new vhost - set permissions for vhost - profit
Most paster commands inserted the root of the kallithea app in sys.path ... but only after loading all the Kallithea modules. That seems a bit pointless.
pullrequests: when creating, don't convert selected revision to tag name
It kind of makes sense that if a branch head or bookmarked revision is chosen, then pretend the symbolic name was chosen. The name can move and will give the PR a future. That is currently done both when populating the select boxes and when actually creating the new PR. There, a selected revision is also "upgraded" to its branch.
Tags are however mostly static. Fair enough if a user wants to create a PR from a tag, but if a revision is selected, it shouldn't be side tracked to the tag name.
With some other dev-requirements having no version specification, there is no real reason to keep an upper bound to pytest-sugar either. The lower bound is still relevant as that fixed a bug.
tests: bump pytest version requirement to allow 3.x
pytest is an active project, since our previous requirement setting there have been many releases, with new features and bug fixes.
One of the improvements in pytest-3.0 is that 'yield_fixture' (a fixture that can embed cleanup code after a yield) is no longer different than 'fixture', i.e. a standard fixture supports the yield cleanup mechanism. We will switch to use that when we make 3.0 mandatory.
yield-based tests (tests that are actually generating tests) are a nose feature which were also supported in pytest. However, since pytest-3.0 this type of tests is deprecated, they will be removed in pytest-4.0.
The only yield tests present in the kallithea test suite are simply yielding a method that does a single assert. The benefit of having each of these asserts in a separate test is not very high.
Replace the yield by a direct call to the previously-yielded method. This means the total test count reduces but the actual amount of test code is the same.
test.ini: align some logging-related settings with development.ini
When manually using test.ini (that is, not via the test suite) the coloring of log output like development.ini provides can be very useful. Note that this color output is not taken along by pytest-capturelog, regardless of this patch.
Some extra-verbose logging in test.ini is on the other hand not needed and could be enabled when needed by a specific developer.
Note: the 'level=DEBUG' setting for handler_console_sql is not taken along to test.ini: it causes duplicate sqlalchemy debug logs, one through handler_console_sql and another through another path.
If future experience points out that some of these changes are actually worse than they were, adjustments can still be made.
tests: admin_users: make sure repo group is deleted
test_delete_repo_group_err creates then deletes a repository group. However, if the delete fails the repository group remains. This later causes problems in the model tests.
Introduce a pytest yield fixture to handle the creation _and_ deletion of the repository group (suggested by Søren Løvborg). The creation of the user needs to happen _before_ that of the user group, and we cannot share data between two pytest fixtures, so the user is created in the fixture as well.
tests: admin_users: make sure all custom IP permissions are cleared
test_delete_ip changes IP permissions and at the end tries to clean up by deleting it again. When the delete fails, there is still a restricted IP permission configuration, causing other tests to fail. Use the recently added pytest fixture auto_clear_ip_permissions to fix this. The fixture is extended to not only clear IP permissions for the default user, but also for the 'regular' test user.
Similar cleanup code in test_add_ip is deleted because it serves the same purpose, but would fail to execute if something went wrong earlier in the test method.
This commit is very similar to an earlier commit that covers similar add/delete IP functionality for the default user, in test_permissions.py.
tests: admin_permissions: split test_delete_ips from test_add_ips
While it is not necessary to be pedantic and split each assert in a separate test, it makes sense to create separate tests for separate logical actions. This makes it easy to understand from a test summary what works and what doesn't.
Add and delete are deemed two such separate logical actions. The original test_add_ips test did both add and delete, but was not even named to cover this.
Note that the 'add' in the delete test is not the same as the 'add' in the add test, i.e. for the delete test direct method calls are made instead of passing through self.app.get/post (which is a higher layer of abstraction).
Background of this commit: during the Turbogears2 port, delete actions were not yet functional, and 'test_add_ips' failed as a result even though 'add' was perfectly fine.
tests: admin_permissions: make sure all custom IP permissions are cleared
test_add_ips changes IP permissions and at the end tries to clean up by deleting it again. When the delete fails, there is still a restricted IP permission configuration, causing other tests to fail.
Attempt a direct delete to the database without relying on the application post/get interface.
tests: remove sleep hack to expire sql_cache_short
A number of tests sleep for 1.5 or 2 seconds to let the beaker cache 'sql_cache_short' expire. This cache is for example used for IP permissions; tests changing such permissions need to make sure they take effect before proceeding, especially when exiting from the test.
A much faster method is to effectively invalidating the caches. Because it is difficult and fragile to only invalidate the relevant cache -- difficult to know exactly which cache needs to be invalidated, fragile because the string indicating the cache line is not very nice and might change in the future (in the case of IP permissions for the default user, the cache is referred to with something like "get_user_ips_default_Mapper|UserIpMap|users". This string changes when the permissions are for a different user.
Clearing all caches shouldn't be a problem in a test context.