Duplicate from line 12.

Instead of manually building this here, we can use a different feature of setuptools: https://python-packaging.readthedocs.io/en/latest/non-code-files.html

Basically passing include_package_data=True to the setup() function is enough and then specifying the actual non-Python files in the MANIFEST.in file (read: file manifest template). It supports globs and recursive inclusion, for our case:

include AUTHORS.rst CHANGELOG.rst README.rst
recursive-include src/redash_stmo *.html *.js

Please stay consistent with the 4 character indentation.

Linebreak after the string please.

So looking at this I noticed a problem I hadn't seen when I reviewed the extension API code into our fork, that the need to provide config values for extension_directory and frontend_content isn't actually really there and looks like redundant. Both values can be calculated from the entrypoint object that is returned when we load the redash.extensions entrypoints.

• extension_directory -- is the filesystem path of the package redash_stmo.datasource_link
• frontend_content -- has no need to be defined individually per extension and should instead be consistent for every extension

So I believe we should simplify the structure of an extension further:

An extension is a Python module or package which may or may not have a bundle subdirectory, and is defined in the redash.extensions entrypoint.

The code that loads the bundles in bundle-extensions should calculate the original source of the extension frontend files from the entry in the redash.extensions entrypoint and use the name of the loaded entrypoint (e.g. datasource_link) as the subdirectory in client/app/extensions.

That basically would remove the need for the webpack.bundles entrypoint again and consolidate frontend and backend into one location.

What about when an extension is frontend only with no python portion? Would we then have an empty python file/function for it to be loaded here: https://github.com/mozilla/redash/blob/master/redash/extensions.py?

Alternatively, extensions.py could be modified to skip over empty extensions.

I think this is originally why I had a separate entry, webpack.bundles here. Since not all frontend extensions will have a backend portion.

For pure-frontend extensions the process would still be having a package with its name, e.g.

src
└── redash_stmo
    ├── __init__.py
    ├── dockerflow.py
    ├── health.py
    ├── whimsycorn
    │   ├── __init__.py
    │   └── bundle
    │   │   ├── index.js
    │   │   ├── style.css
    │   │   └── template.html
    ├── query_runner
    │   ├── __init__.py
    │   └── activedata.py
    └── settings.py

That would allow us:

a) use the Python import system to look for the path of the packages

b) provide a location of extra extension metadata later on (e.g. verbose name, description text etc)

Skipping over extensions by checking if the loaded entrypoint is a callable would make sense. IIRC the Entrypoint object has some good APIs to see what it is.

E.g. the syntax for entrypoints only requires a modulename (read: dotted path to a module or package): https://github.com/pypa/setuptools/blob/01fd232d95ad51f784de53c0993a7b91868d269d/pkg_resources/__init__.py#L2361-L2368

You can use the Entrypoint.resolve function which will try to import it and raise an ImportError if it fails when you skip those that don't actually have an entrypoint provided.

So an entrypoint list in setup.py could look like this (where the whimsycorn extension just happens to not need a callable):

entrypoints={
    'redash.extensions': [
        'dockerflow = redash_stmo.dockerflow:dockerflow',
        'datasource_health = redash_stmo.health:datasource_health',
        'datasource_link = redash_stmo.datasource_link:datasource_link',
        'whimsycorn = redash_stmo.whimsy_corn',
   ], 
}

In the bundle-extensions script you'll repeat some of the logic of course since you'll need to check if the bundle directory exists in the extension directory.

You should use parentheses to combine those two imports in one import call.

So a few things, adding the configuration property to the runner_class makes sense, although the code is missing so I'm not sure, but the other part where it monkey-patches multiple classes to pass a few strings to a template is not a good idea maintenance wise.

I suggest to instead create a new API endpoint /api/data_sources/<data_source_id>/link like so (and do the route registration inside the datasource_link entrypoint callback):

from redash.handlers.api import api

class DataSourceLinkResource(BaseResource):
    def get(self, data_source_id):
        data_source = get_object_or_404(
            DataSource.get_by_id_and_org,
            data_source_id,
            self.current_org,
        )
        return {
            'type_name': data_source.query_runner.name()
            'doc_url': data_source.options.get('doc_url', None)
        }


api.add_org_resource(DataSourceLinkResource, '/api/data_sources/<data_source_id>/link')

Then in the frontend code we simply fetch the data from that endpoint and pass it to the template when the site is loaded. That way we separate the concerns and don't risk overlaps with other extensions in the future that also want to add stuff to the query editor page. FWIW I'm not worried about the extra call to the server since we're not really dealing with a ton of traffic.

Good point! I did not consider an API call as an alternative here, but I like it. Thank you!

To clarify, add_configuration_property() would be implemented in BaseQueryRunner and it would look something like this:

class BaseQueryRunner(object):
    ....
    configuration_properties = None
    ....
    @classmethod
    def add_configuration_property(cls, property, value):
        if cls.configuration_properties is None:
            raise NotImplementedError()
        cls.configuration_properties[property] = value

And every class that inherits from BaseQueryRunner that wants to add a URL to its config needs to have a configuration_properties field. Does that sound reasonable?

Yep, that sounds totally reasonable! Thank you :)

This needs a tiny bit of update given the difference in what the datasource link API endpoint returns.

I understand that we use Angular's resource thing here to get the data from the API, and I'm not sure how to write the actual API call to the new datasource link API endpoint. But since we'll be doing React in the future, might as well not lean too much on Angular IMO.

In fact, don't you want to try writing this in React instead? Allen could help you get up to speed how to do that and the React setup is already in Redash enabled.

Is this an issue in Redash or something that Flask-Restful does wrong?

This is an issue in Flask-Restful. There are a few things going on that led me to adding this line.

tl;dr: Flask-Restful does not play well with the Redash setup of multiple workers each having a different instance of a flask app.

1. Inside create_app() in redash/__init__.py, we cannot move handlers.init_app(app) (which calls api.init_app(app)) after extensions.init_extensions(app) because the datasource_health extension overwrites the status_api endpoint after it’s created. If extensions initialize first then an attempt is made in Redash to add status_api and it crashes. So we keep the init order so redash-stmo can override endpoints that already exist.

2. Unlike api = Api(app) (https://github.com/flask-restful/flask-restful/blob/master/flask_restful/__init__.py#L92), api.init_app() does not set api.app when it's called (https://github.com/flask-restful/flask-restful/blob/master/flask_restful/__init__.py#L196). This results in any further calls to api.add_resource() after api.init_app() to actually have no effect. It results in appending to api.resources (https://github.com/flask-restful/flask-restful/blob/master/flask_restful/__init__.py#L384) but no call to register_view()

3. Even if we did manage to work around this issue in 1., this would not resolve this issue. A Flask app is created in each worker with create_app(). If an extension tries to do api.add_org_resource(), the same resource will be added to api.resources from multiple workers. Now when api.init_app(app) is called, there is an exception thrown that there are multiple view functions with the same name.

The crux of the problem is that api.resources is shared among multiple apps as a temporary cache until api.init_app(app) is called so that resources can be attached to a given app. Since we have a different app for each worker, and each worker is adding the same resource in an extension, this model doesn't quite work.

By setting api.app = app we ensure that the resource is added to the correct app and there is no duplication by caching in api.resources.

That makes sense to me!

Any reason to have this prefixed with an underscore?

This was just to indicate it's not a built-in React function like the rest. I can remove it if it's more confusing than useful. It doesn't necessarily seem to be a common practice, I've just seen it in a couple of places.

I don't have a strong opinion either way, just noticed and wondered about the best practice in modern JS is.

Forgive my noobness with modern JS, wouldn't this code fail in case the fetch call returns an empty object because for some error happening server side?

I mean, json.message would not exist and then the variable unpacking would fail. Does this require more defensive code?

Good catch. I will fix this prior to merging, thanks!