Make Blaze server /add endpoint more flexible
#1481
Conversation
| if isinstance(resource_info, dict): | ||
| # Extract resource creation arguments | ||
| source = resource_info['source'] | ||
| extra = resource_info.get('extra', {}) |
There was a problem hiding this comment.
It's an edge case, but some resource() implementations in odo do take both positional and kwargs, and treat the positional args in a special way. For instance:
https://github.com/blaze/odo/blob/master/odo/backends/sql.py#L545
I also realize that the original code didn't handle this case, but this is an opportunity to do it correctly :)
Can you refactor this so that the resource_info has both an args sequence and a kwargs dict / mapping, ala:
source = ...
args = resource_info.get('args', [])
kwargs = resource_info.get('kwargs', {})
...
data.update({name: resource(source, *args, **kwargs)})
Allow the `/add` endpoint of the Blaze server to take a more complex payload. Rather than simply a name and resource URI string, the server can now accept a dictionary with 'source', 'args, and 'kwargs' fields. The 'source' is the URI string as before, but the 'args' field can be a sequence of additional positional arguments, and the 'kwargs' fields can be a dictionary of keyword arguments -- both are passed through to the `blaze.resource` constructor. For instance, in the test case the `encoding` argument is passed through to allow reading a CSV in a non-default encoding.
|
@kwmsmith Thanks for the quick review! Addressed your comments -- added |
|
While we're on the topic, @necaris, one gap here is that there's no |
Add a method to ``blaze.server.client.Client`` to allow programmatic addition of datasets to the Blaze server, using the new `/add` endpoint.
|
@kwmsmith FYI, I've added more tests as well as support for an |
|
I am sort of afraid of having |
@llllllllll good to know, and good input. So would an |
|
@llllllllll @kwmsmith I've added an It might be more appropriate to return a Forbidden error, possibly with a message body indicating that the server instance doesn't support adding datasets -- thoughts? |
Add an optional 'imports' key to the expanded payload dictionary, which functions just like the 'imports' key in the server YAML file: specifies string names for modules that must be imported before the resource can be created. In the ``blaze.server.client.Client``, this adds an optional keyword argument to the `add` method -- while it is documented specifically, it doesn't have any precedence over other kwargs.
Add a flag `allow_add` to the ``blaze.server.Server`` constructor to determine whether or not the `/add` route is available -- it defaults to `False`. Currently, if this is not set, it doesn't define the URL mapping at all, so any requests for it will result in a 404 Not Found error.
|
@necaris looks good -- the only addition is to add a commandline argument to the |
|
@kwmsmith Added! |
| @@ -164,6 +164,8 @@ def _parse_args(): | |||
| help='Call ``blaze.data`` on hidden files') | |||
| p.add_argument('--yaml-dir', action='store_true', | |||
| help='Load path-based resources relative to yaml file directory.') | |||
| p.add_argument('--allow-dynamic-addition', action='store_true', | |||
There was a problem hiding this comment.
It's good this is a long option name to make it less likely to trigger accidentally.
There was a problem hiding this comment.
😄 I considered -a but I figured that would be too easy to do by mistake!
|
@necaris LGTM, thanks! |
Allow the
/addendpoint of the Blaze server to take a more complexpayload. Rather than simply a name and resource URI string, the server
can now accept a dictionary with 'source' and 'extra' fields. The
'source' is the URI string as before, but the 'extra' field can be a
dictionary of keyword arguments that are passed through to the
blaze.resourceconstructor.For instance, in the test case the
encodingargument is passed throughto allow reading a CSV in a non-default encoding.