Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Redesign register_output_renderer callback #581

Closed
simonw opened this issue Oct 5, 2019 · 24 comments
Closed

Redesign register_output_renderer callback #581

simonw opened this issue Oct 5, 2019 · 24 comments

Comments

@simonw
Copy link
Owner

simonw commented Oct 5, 2019

In building https://github.com/simonw/datasette-atom it became clear that the callback function (which currently accepts just args, data and view_name) would also benefit from access to a mechanism to render templates and a datasette instance so it can execute SQL.

To maintain backwards compatibility with existing plugins, we can introspect the callback function to see if it wants those new arguments or not.

At a minimum I want to make datasette and ASGI scope available.

@simonw
Copy link
Owner Author

simonw commented Oct 5, 2019

The request object should be made available too.

@simonw
Copy link
Owner Author

simonw commented Dec 26, 2019

To solve simonw/datasette-atom#6 the name of the current canned query should be made available somehow.

That way the plugin configuration could specify that the title for browse/feed should be X.

@simonw
Copy link
Owner Author

simonw commented Feb 28, 2020

Rather than pass a request object (hence promoting that object into part of the documented, stable API) I think I'll pass the ASGI scope - that's already a stable, documented standard.

UPDATE: changed my mind since request is used by other plugins too, see #706.

@simonw simonw pinned this issue Apr 30, 2020
@simonw
Copy link
Owner Author

simonw commented May 27, 2020

Here's the code that calls the renderers - this needs to be expanded to check for those extra optional arguments:

if _format in self.ds.renderers.keys():
# Dispatch request to the correct output format renderer
# (CSV is not handled here due to streaming)
result = self.ds.renderers[_format](request.args, data, self.name)
if result is None:
raise NotFound("No data")
r = Response(
body=result.get("body"),
status=result.get("status_code", 200),
content_type=result.get("content_type", "text/plain"),
)

@simonw
Copy link
Owner Author

simonw commented May 27, 2020

While I'm doing this, another feature I would like is the ability for renderers to opt-in / opt-out of being displayed as options on the page.

https://www.niche-museums.com/browse/museums for example shows a atom link because the datasette-atom plugin is installed... but clicking it will give you a 400 error because the correct columns are not present:

browse__museums__102_rows

@simonw
Copy link
Owner Author

simonw commented May 27, 2020

Here's the code that passes a list of renderers to the template:

renderers = {
key: path_with_format(request, key, {**url_labels_extra})
for key in self.ds.renderers.keys()
}
url_csv_args = {"_size": "max", **url_labels_extra}
url_csv = path_with_format(request, "csv", url_csv_args)
url_csv_path = url_csv.split("?")[0]
context = {
**data,
**extras,
**{
"renderers": renderers,
"url_csv": url_csv,

A renderer is currently defined as a two-key dictionary:

@hookimpl
def register_output_renderer(datasette):
    return {
        'extension': 'test',
        'callback': render_test
    }

I can add a third key, "should_suggest" which is a function that returns True or False for a given query. If that key is missing it is assumed to return True.

One catch: what arguments should be passed to the should_suggest(...) function?

@simonw
Copy link
Owner Author

simonw commented May 27, 2020

I'll use #770 for the should_suggest mechanism - this issue is for the extra arguments passed to the rendering callback.

@simonw simonw changed the title Extra arguments for register_output_renderer callback Redesign register_output_renderer callback May 27, 2020
@simonw
Copy link
Owner Author

simonw commented May 27, 2020

The existing render callback takes the following arguments:

args - dictionary
The GET parameters of the request

data - dictionary
The data to be rendered

view_name - string
The name of the view where the renderer is being called. (index, database, table, and row are the most important ones.)

The data argument is a bit of a problem, because it tightly couples plugins to a currently undocumented datastructure within Datasette. Here's how datasette-atom picks that apart for example: https://github.com/simonw/datasette-atom/blob/095941c23c81b70c4787cdeef873c556b573b5fa/datasette_atom/__init__.py#L15-L66 - it does things like access data["query"]["sql"] to figure out the SQL query that was used.

I'm going to change the design of part of this ticket. I won't break the old data value just yet, but I'll mark it to be deprecated by Datasette 1.0.

I think the only plugins using it right now are my datasette-atom and datasette-ics and @russss's datasette-geo so hopefully changing this won't cause any wider damage.

@simonw
Copy link
Owner Author

simonw commented May 27, 2020

OK, the new design: your callback function can take any of the following arguments:

  • datasette - a Datasette instance
  • columns - the list of columns
  • rows - the list of rows (each one a SQLite Row object)
  • sql - the SQL query being executed
  • query_name - the name of the canned query, if this is one
  • database - the database name
  • table - the table or view name
  • request - the request object (to be documented in Documentation for the "request" object #706)
  • view_name - the name of the view

We will also continue to support the existing args and data arguments, but these will be undocumented and will be deprecated in Datasette 1.0. UPDATE: Decided against this, see #581 (comment)

@simonw
Copy link
Owner Author

simonw commented May 27, 2020

The should_suggest callback will take the same arguments: #770 (comment)

@simonw
Copy link
Owner Author

simonw commented May 27, 2020

I think I need a utility function for "call this function with this dictionary of arguments, but only pass the arguments which are inspected by the function".

@simonw
Copy link
Owner Author

simonw commented May 27, 2020

https://docs.python.org/3/library/inspect.html#introspecting-callables-with-the-signature-object

New in version 3.3.

The Signature object represents the call signature of a callable object and its return annotation. To retrieve a Signature object, use the signature() function.

inspect.``signature(callable, ***, follow_wrapped=True)

Return a Signature object for the given callable:

>>> from inspect import signature
>>> def foo(a, *, b:int, **kwargs):
...     pass

>>> sig = signature(foo)

>>> str(sig)
'(a, *, b:int, **kwargs)'

>>> str(sig.parameters['b'])
'b:int'

>>> sig.parameters['b'].annotation
<class 'int'>

@simonw
Copy link
Owner Author

simonw commented May 27, 2020

In [1]: import inspect                                                                                                                                                             

In [2]: def foo(view, sql, inspect): 
   ...:     pass 
   ...:                                                                                                                                                                            

In [3]: inspect.signature(foo)                                                                                                                                                     
Out[3]: <Signature (view, sql, inspect)>

In [4]: inspect.signature(foo).parameters                                                                                                                                          
Out[4]: 
mappingproxy({'view': <Parameter "view">,
              'sql': <Parameter "sql">,
              'inspect': <Parameter "inspect">})

In [5]: inspect.signature(foo).parameters.keys()                                                                                                                                   
Out[5]: odict_keys(['view', 'sql', 'inspect'])

In [6]: set(inspect.signature(foo).parameters.keys())                                                                                                                              
Out[6]: {'inspect', 'sql', 'view'}

@simonw
Copy link
Owner Author

simonw commented May 27, 2020

Here's the function I just wrote for this:

def call_with_supported_arguments(fn, **kwargs):
    parameters = inspect.signature(fn).parameters.keys()
    call_with = []
    for parameter in parameters:
        if parameter not in kwargs:
            raise TypeError("{} requires parameters {}".format(fn, tuple(parameters)))
        call_with.append(kwargs[parameter])
    return fn(*call_with)

@simonw
Copy link
Owner Author

simonw commented May 27, 2020

Need to figure out how best to unit test this plugin hook.

@simonw
Copy link
Owner Author

simonw commented May 27, 2020

Actually passing the request object would be OK if I document it see #706

@simonw
Copy link
Owner Author

simonw commented May 27, 2020

Since I'm passing request I won't pass scope - if people want that they can access request.scope.

@simonw
Copy link
Owner Author

simonw commented May 27, 2020

It bothers me that query_name here means the configured name of the canned query, but view_name means the name of the Datasette view class, NOT the name of an associated SQL view. That's in table.

Can I come up with clearer names for these?

@simonw
Copy link
Owner Author

simonw commented May 27, 2020

I'm going to break backwards compatibility directly here, without waiting for Datasette 1.0.

The reason is that https://github.com/russss/datasette-geo hasn't been updated in 13 months so is already broken against current Datasette, and the other two plugins using this hook are owned by me so I can upgrade them myself.

@simonw
Copy link
Owner Author

simonw commented May 27, 2020

(I used GitHub code search to find code using this plugin hook: https://github.com/search?q=register_output_renderer&type=Code )

@simonw
Copy link
Owner Author

simonw commented May 27, 2020

Right now a rendering callback returns the following:

body - string or bytes, optional
    The response body, default empty
content_type - string, optional
    The Content-Type header, default text/plain
status_code - integer, optional
    The HTTP status code, default 200 

I'm going to add an optional headers dictionary key, too.

@simonw
Copy link
Owner Author

simonw commented May 27, 2020

(I wonder if this would be enough to allow really smart plugins to implement ETag/conditional get)

@simonw
Copy link
Owner Author

simonw commented May 27, 2020

Request object is now documented: https://datasette.readthedocs.io/en/latest/internals.html#request-object

@simonw
Copy link
Owner Author

simonw commented May 28, 2020

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant