hasgeek / coaster Goto Github PK

As the title states, I think this module should provide Python 3 compatibility. :)

The markdown filter currently does not touch quotes. It should take an optional flag to automatically converts straight quotes into smart quotes, but only outside pre/code blocks.

Every once in a while, a user manages to double click a submit button, causing their browser to send two identical requests, causing our web server to fire up two parallel workers to handle the request (workers A & B), which then perform this dance:

• A checks for an existing record, finds none
• B checks for an existing record, finds none
• A makes a new record and attempts to commit
• B makes a new record and attempts to commit
• A's commit succeeds
• B's commit fails with an IntegrityError (some unique constraint was violated)
• A returns status to browser, but browser has since cancelled the request in favour of B
• B returns failure, resulting in user seeing a failure message when the operation actually succeeded

While we have changed our UI to disable submit buttons after the first submit (in hasgeek/baseframe@b03b666), the correct solution is to use a SAVEPOINT and roll-back just that failure. This has been documented in hasgeek/lastuser#130, but since it requires 7 lines of code, it's best abstracted into a base class. Here is the code outline from Mike Bayer:

session.begin_nested() 
try: 
     session.add(thing_that_may_exist_already) 
     session.commit()  # flushes, and commits only the "savepoint" 
except exc.IntegrityError: 
    session.rollback()     
    thing_that_may_exist_already = session.query(Thing).filter_by(<criteiron>).one() 

In our case, we could have:

class AddAndCommitMixin(object):  # mixin class added to BaseMixin
    @classmethod
    def add_and_commit(cls, ob, loader, *args, **kwargs):
        cls.query.session.begin_nested()
        try:
            cls.query.session.add(ob)
            cls.query.session.commit()
            return ob
        except exc.IntegrityError:
            cls.query.session.rollback()
            return loader(*args, **kwargs)

Typical use would be like this (assuming Lastuser's UserEmail confirmation, where the confirmation happens in a GET request via their email client, and so has no protection from disabling submit buttons):

useremail = UserEmail(user=g.user, email=email)
useremail = UserEmail.add_and_commit(useremail, UserEmail.get, user=g.user, email=email)

Assuming the same logic that applies to UUID1 generating linear and therefore index-friendly UUIDs also applies to Base64 representations of these UUIDs, the buid function should switch to these using uuid1mc. There's no obvious downside to this switch, but a potential upside.

Coaster's current configuration mechanism for Flask apps is clumsy. It'll be far cleaner if coaster.app provides a Flask subclass that includes an init_for(env) method that handles its own configuration.

http://travis-ci.org/#!/deepakjois/coaster/jobs/1781934

I added some basic Travis CI integration, and I see the failures above. I am guessing, based on build output that it is a dependency related issue. Travis installs the package using pip install . --use-mirrors.

Rich text fields often require three columns:

class MyModel(Base):
    description_text = Column(UnicodeText)  # Plain text version
    description_html = Column(UnicodeText)  # HTML version
    description_format = Column(Unicode(20))  # Plain text format

There is no convenient way to declare three columns at once in SQLAlchemy, or to link them to each other so that setting one will autogenerate the other version using the specified format.

However, since these columns are almost always used together, they could be stored as one column with a JSON data structure (using Coaster's existing JsonDict column).

This new column type will automatically generate one format from the other and will supply a Markup() string for the HTML version, reducing the need for templates to be aware of content type.

Along with email and SMS, Coaster should be able to send error reports to Slack (with throttling like in SMS) so that a tech team can read and respond right away.

Since error reports are rather long, we need to ensure the text can be collapsed or is sent as an attachment rather than in the main body.

The db object in baseframe is mislocated because some projects may not use baseframe. It should be in coaster instead, with guarantees of being a singleton within the same process.

Should add user column and provide stubs for user merging.

Please add licensing info, e.g. a LICENSE.txt file at the root, to allow reuse by third parties.

Also, it would be nice to do it for all your projects.

Thanks.

The docs for SQLAlchemy patterns provided by Coaster in coaster.sqlalchemy is sort of inconsistent what's available in the code.

The docs page doesn't list Query class docs and some classes are missing sample use cases.
For the latter, we could with do with some sample use cases, just a matter of writing them.

However the former is tricky.

Context: we use readthedocs to generate this documentation. So running git submodule update --init --recursive brings in the theme dir required for you to compile docs locally with cd docs && make html. The compiled HTML files are ../docs/build/html

At the moment, I am unable to view documentation added to any new class in the ../coaster/sqlalchemy.py file. Even copy pasting an existing class and renaming it is not picked up by readthedocs despite a fresh make. Only changes to existing classes (whose docs are being picked up by readthedocs) are picked up by readthedocs.

Returning an empty bundle makes it hard to debug typos.

• class coaster.utils.NameTitle(name, title)
• coaster.utils.deobfuscate_email(text)
• coaster.utils.domain_namespace_match(domain, namespace)
• coaster.utils.namespace_from_url(url)

don't have an examples associated with them.

load_models raises 404 when a query fails. It should be possible to provide a custom 404 handler that offers suggestions given that load_models knows exactly which model in the chain failed and therefore where to look for suggestions.

Since manage.py is so consistent across our apps, we should move the script to coaster. Apps will just need the following now:

from hgapp import app, db
from coaster import manage

manage.init_app(app, db)

if __name__=='__main__':
    manage.main()

Coaster's GFM implementation does not match the spec correctly. It should be replaced with the canonical JS version, which can be called with PyExecJS (which is already a dependency in coaster.assets).

This replacement will mean losing Pygments support, but this is okay because JS-based alternatives such as PrismJS exist.

Only caveat is that the JS code will need to be updated regularly and Coaster itself does not have an asset management mechanism -- it only provides the framework used by Baseframe (which requires manual updates to assets).

Consider this use case from Funnel:

@app.route('/<space>/users/new', defaults={'group': None}, endpoint='usergroup_new', methods=['GET', 'POST'])
@app.route('/<space>/users/<group>/edit', methods=['GET', 'POST'])
@lastuser.requires_login
@load_model(ProposalSpace, {'name': 'space'}, 'space', kwargs=True,
    permission=('new-usergroup', 'siteadmin'), addlperms=lastuser.permissions)
def usergroup_edit(space, kwargs):
    group = kwargs.get('group')
    if group is not None:
        usergroup = UserGroup.query.filter_by(name=group, proposal_space=space).first_or_404()

usergroup is being queried manually to support the /new endpoint. The @load_models decorator needs a way to be told that querying UserGroup is optional based on the value of the group parameter. At the moment the None value for group will be passed on to the query.

One way to handle this is for coaster.views to define a singleton (perhaps named OPTIONALQ) that can be passed as the default value in the route. If this singleton is received by load_models, it can skip the query. The singleton can also evaluate to boolean false to support typical use cases.

This is a minor cleanup task.

The logic in configure method is a bit awkward : https://github.com/hasgeek/coaster/blob/master/coaster/app.py#L8

• There is no need to call app.config.from_pyfile multiple times
• We could check for the the existence of settings.py, failing which we can check for the files specified in the additional dict, and call app.config.from_pyfile with the appropriate argument, and raise an error if no files are found.

What happens if a url_id is assigned and then deleted? Does it get re-used? Tests are required to watch this behaviour.

set_alembic_revision() is not successfully creating the alembic_version table in some cases.

This issue was noted in lastuser. On subsequently attempting to run it for hacknight in the same environment, it created the table.

Flask's url_for is view-centric. Given a view name and parameters, it generates a URL. We often have to work the reverse: given a model instance, create a URL to its view. All BaseMixin models should have a url_for(view="view") method that implementations can override to return a URL.

To be decided: If a view (default value view, implying the primary public view) is not known by the model, should the method:

1. Return None, implying no known view? or
2. Raise an exception, making it easier to catch gaps in implementation?

The url_id field is currently set based on an SQL query. This is not concurrency-safe. It should be set to an SQL expression where the actual value is determined on commit.

We have to check for instances where url_id is used immediately, before committing.

In BaseScopedNameMixin, use short_title for make_name instead of title. This will generate cleaner URLs.

Flask-Alembic is unmaintained and doesn't work with Flask-Script > 0.5.3, so we've been off the mainstream for a while. Flask-Migrate is newer and better maintained, so we should switch.

1. We have a custom fork of Flask-Alembic that inserts an init_for call as required for our apps. Flask-Migrate provides a MigrateCommand for Flask-Script which doesn't know about init_for, so we'll to have to override that with our version.
2. Versions have to move from alembic/versions to migrations in all apps. This unfortunately means it is a breaking change and we have to sync updates with all apps, or write another wrapper that looks in both paths for migrations.
3. The real motivation for doing this is that we want the ability to split migrations between blueprints and keep multiple alembic_version numbers in the database. While this is supported by Alembic, it's not exposed by the wrappers. Reasoning: this will allow splitting up large apps into multiple, independently maintained blueprints (in separate git repositories), all working off the same database but with an independent set of models (except shared models like User from a shared blueprint like lastuser_core) and independent versioning for their models.

coaster.views.requestargs is insecure because a wrapped function cannot be called by another function without risk of data contamination from the ongoing request.

Flask's own app.rule does not wrap the decorated function. It merely registers it in the app's URL map. This ensures that direct function calls are disconnected from the request context.

requestargs needs to be similarly replaced with requestargs_rule with this usage:

@requestargs_rule(app, '/foo/bar', ...,
    args=('arg1', ...))
def function(arg1, ...):
    pass

This decorator passes a requestargs-wrapped function to app.rule, but does not replace the original function.

It is not clear if coaster.sanitize_html handles javascript protocol targets in a href tags. This needs testing.

Example URL for project: http://hacknight.in/fifthelephant/bangalore2012/projects/2-dise

When a user tries to visit URL like http://hacknight.in/fifthelephant/bangalore2012/projects/new ends up with Internal Server Error,

Here is a portion of stack trace

'SELECT project.event_id AS project_event_id, project.participant_id AS project_participant_id, project.blurb AS project_blurb, project.description AS project_description, project.maximum_size AS project_maximum_size, project.participating AS project_participating, project.votes_id AS project_votes_id, project.comments_id AS project_comments_id, project.name AS project_name, project.title AS project_title, project.url_id AS project_url_id, project.id AS project_id, project.created_at AS project_created_at, project.updated_at AS project_updated_at \nFROM project \nWHERE project.url_id = %(url_id_1)s AND %(param_1)s = project.event_id' {'url_id_1': u'new', 'param_1': 1}

{'url_id_1': u'new', 'param_1': 1} url_id is supposed to be int.

so it doesn't have to be typed every time.

Coaster's BaseNameMixin and BaseScopedName mixins disallow null values for the name via an SQL NOT NULL construct. However, they do not block blank values (empty strings) because neither SQL nor SQLAlchemy provide a handy method to do that.

Non-blank constraints can be applied at the application level with a field validator, or the SQL level with a CHECK constraint, and doing so is straightforward, but we have two problems:

1. BaseNameMixin and BaseScopedName have been in Coaster for years and while adding the constraints now will not break any application, a migration will be necessary to ensure that existing installations have the exact same database schema as new installations. This is a bother for apps that aren't actively maintained.
2. Eventframe exploited the lack of a non-blank constraint in its architecture and requires index nodes to be named with an empty string. Adding a constraint to the mixins now will break new installations of Eventframe unless Eventframe's Node class is amended to redefine the name column.

When faced with a similar situation in Flask-SQLAlchemy, we came up with the imaginatively named UserBase2 class and started updating apps to use it. That migration is as yet incomplete. Having a BaseNameMixin2 class is going to hit harder because it's used in many more models, and Alembic's autogenerator does not discover the CHECK constraint, so migrations have to be written by hand.

Our options:

1. Make a new class suffixed 2 or whatever and mark the older class as deprecated, and start migrating apps. Let deprecated cruft collect in Coaster and make a schedule for removal.
2. Since the change is non-breaking, recommended and won't have autogenerated migrations, just add the constraint, write the workaround for Eventframe, and manually write migrations for apps that won't error out if the target database table already has a CHECK constraint on it. This also avoids cruft collecting in Coaster.

In coaster.sqlalchemy.JsonDict line 595, we use simplejson with use_decimal=True to parse non-integer numbers as Decimal instead of float. While this is a good idea because of the general unreliability of floats, it is inconsistent: this line is not reached when using PostgreSQL >= 9.2 with Psycopg2 >= 2.5. Psycopg2 does its own JSON decoding then and does not use Decimal by default.

Psycopg2 allows customisation of the way it handles JSON. This customisation can be applied at the connection or cursor level, but not per column, so the customisation code does not belong in JsonDict. Instead, it should be an interface Coaster provides per-app. JsonDict can then use this interface to parse JSON in a manner identical to what Psycopg2 would have done.

coaster.sqlalchemy.UrlForMixin has made view construction much simpler by allowing for usage in the form of {{ ob.url_for() }} instead of the longer {{ url_for('ob_view', name=ob.name) }} syntax.

However, the models now require a url_for method which is aware of each view that applies to the model. Since models are lower level than views in our hierarchy (models < forms < workflows < views) and lower levels are by intent not supposed to be aware of higher levels, this has been a bad compromise.

UrlForMixin should provide a is_url_for classmethod that can be used to decorate views, to indicate that this view applies to it. Sample use:

from coaster.views import load_model
from .models import Post  # Instance identified by Post.name

@app.route('/view/<post>')
@Post.is_url_for('view', post='name')
@load_model(Post, {'name': 'post'}, 'post')
def post_view(post):  # This expects to receive the Post object
    pass

Like load_model, is_url_for will need assistance knowing what parts of the post are required to construct the URL, but in reverse, including sub-attributes like self.parent.name. To support this, it accepts keyword arguments after the "action" name, where the value of each argument is a string naming the attribute to extract from self, or a tuple containing a recursive set of attributes (like ('parent', 'name')). As a shortcut, a dotted string may be provided, which is broken down into a tuple at decorator-load time.

load_models currently treats all parameters as an AND query when loading a model. It needs the ability to perform an OR query. Consider this:

@app.route('/<channel>/<playlist>')
@load_models(
    (Channel, {'name': 'channel'}, 'channel'),
    (Playlist, {'name': 'playlist', 'channel': 'channel'}, 'playlist')
)
def playlist_view(channel, playlist):
    pass

The dictionary syntax treats all dictionary keys as part of an AND query. It should be possible to perform an OR query, checking for a match in one or more attributes. I propose this syntax:

@app.route('/<channel>/<playlist>')
@load_models(
    (Channel, {'name': 'channel'}, 'channel'),
    (Playlist, ({'name': 'playlist', 'channel': 'channel'}, {'uuid': 'playlist', 'channel': 'channel'}), 'playlist')
)
def playlist_view(channel, playlist):
    pass

Supplying a list/tuple of dictionaries should signal an OR query.

coaster.manage.shell is currently hardcoded for production environments. This is incorrect. It should use the user-supplied environment.

Namespaces are reversed domains, turning www.website.tld into tld.website.www. A new function domain_namespace_match is required that turns the namespace into a domain and compares it with the given domain using base_domain_matches.

It would be handy to have Flask-debugtoolbar available across all our Flask apps. Would be very handy to be able to see SQL queries being executed with a request, especially in Hasjob.

In line https://github.com/hasgeek/coaster/blob/develop/coaster/views.py#L472, update fails since werkzeug.datastructures.Headers is dict-like not real dict, doesn't have update method. http://werkzeug.pocoo.org/docs/datastructures/#werkzeug.datastructures.Headers

extend method may be right choice, which can accept dict or iterable which yields key, value.

How to reproduce

def viewcallable(data):
     return Response(repr(data), mimetype='text/plain')

@app.route('/renderedview4')
@render_with({
      'text/plain': viewcallable})
def view_for_text():
      return {'data': 'value'}, 201,  {'Referer', 'http://example.com/'}

app.testing = True
client = app.test_client()
client.get('/renderedview4', headers=[('Accept', 'text/plain')])

A typical load_models decoration looks like this:

@load_models(
    (Profile, {'name': 'profile'}, 'g.profile'),
    (ProposalSpace, {'name': 'space', 'profile': 'profile'}, 'space'),
    (TicketType, {'name': 'name', 'proposal_space': 'space'}, 'ticket_type'),
    permission='event-edit')

It asks for instances of Profile, ProposalSpace and TicketType to be loaded in sequence, raising a 404 if one is not found. This requires three database roundtrips, which is inefficient. This can be done in one query if we work up from the bottom:

TicketType.query.join(ProposalSpace).join(Profile).filter(TicketType.name == name, ProposalSpace.name == space, Profile.name == g.profile).first_or_404()

To support this syntax, load_models could change the tuple format it accepts slightly. The second item in the tuple is a mapping of a model attribute to a view parameter, for use with the filter_by filter. This could instead be a column (a SQLAlchemy Instrumented Attribute) that is applied with a filter filter. Since the column-containing models are also required to perform a join, the wrapping tuple could accept a fourth item, a model or a tuple of models that must be joined (L-to-R) (or this could be guessed by looking up the models to which the given filter columns are attached).

Coaster's scoped SQLAlchemy models depend on a parent attribute to refer to the containing object within which they have a unique name or id. Unfortunately, the term 'parent' can mean at least two things:

1. Parent as the container
2. Parent as the entity this item follows, such as with threaded comments.

In the latter case, the parent is not a container and there is no scoping to the parent. The container is distinct from the parent.

Faced with this situation with Comment in Hacknight, we chose to use these column names:

• parent to refer to the CommentSpace that contained the comments.
• reply_to to refer to the Comment that this Comment was a response to.

This resolution is unsatisfactory because reply_to is not a generic name. I therefore propose that we replace all current use of parent with container and free up the parent keyword for other uses.

This change will break backward compatibility and requires synchronized changes to all apps that use Coaster's scoped models.

SQLAlchemy's had a native one_or_none since release 1.0.9, so we can deprecate our version.

Since render_with varies its response based on the Accept header, it must set the Vary: Accept header in the response (appending to Vary if it already exists).

Coaster's SQLAlchemy base classes should provide default .get classmethods

The contents of request, g and session will be most useful when debugging a failed request.

Line 20 in tests/test_models.py has config that is already available in [tests/settings.py]. So instead of hardcoding/repeating this info, can we not import the required variable from settings.py?

diff --git a/tests/test_models.py b/tests/test_models.py
index ff804b6..4aa0628 100644
--- a/tests/test_models.py
+++ b/tests/test_models.py
@@ -12,12 +12,12 @@ from sqlalchemy import Column, Integer, Unicode, UniqueConstraint, ForeignKey
 from sqlalchemy.orm import relationship, synonym
 from sqlalchemy.exc import IntegrityError
 from sqlalchemy.orm.exc import MultipleResultsFound
-
+import settings

 app1 = Flask(__name__)
 app1.config['SQLALCHEMY_DATABASE_URI'] = 'sqlite://'
 app2 = Flask(__name__)
-app2.config['SQLALCHEMY_DATABASE_URI'] = 'postgresql://:@localhost:5432/coaster_test'
+app2.config['SQLALCHEMY_DATABASE_URI'] = settings.SQLALCHEMY_DATABASE_URI
 db.init_app(app1)
 db.init_app(app2)

Python version: 3.4.3

When installing this package:

Collecting coaster
Using cached coaster-0.4.3.tar.gz
Complete output from command python setup.py egg_info:
Traceback (most recent call last):
File "", line 20, in
File "/private/var/folders/m2/rrqxjb3n5gscyfm4dbdn1kp80000gn/T/pip-build-x_2sybji/coaster/setup.py", line 7, in
README = unicode(open(os.path.join(here, 'README.rst')).read(), 'utf-8')
NameError: name 'unicode' is not defined

Command "python setup.py egg_info" failed with error code 1 in /private/var/folders/m2/rrqxjb3n5gscyfm4dbdn1kp80000gn/T/pip-build-x_2sybji/coaster

After making the code change in coaster, all projects will need to move from a a python file to a YAML based config file. If we are paranoid about breakages, we could do a gradual switchover, whereby we allow both config systems for a while, with a deprecation warning.

Also add ability to handle status codes.