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

Make Blaze server `/add` endpoint more flexible #1481

Merged
merged 8 commits into from Apr 26, 2016

Conversation

Projects
None yet
3 participants
@necaris

necaris commented Apr 15, 2016

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' 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.resource constructor.

For instance, in the test case the encoding argument is passed through
to allow reading a CSV in a non-default encoding.

if isinstance(resource_info, dict):
# Extract resource creation arguments
source = resource_info['source']
extra = resource_info.get('extra', {})

This comment has been minimized.

@kwmsmith

kwmsmith Apr 15, 2016

Member

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)})
blob = serial.dumps({
'iris': {
'source': iris_path,
'extra': {'delimiter': ','}

This comment has been minimized.

@kwmsmith

kwmsmith Apr 15, 2016

Member

As mentioned above, can you rename this to kwargs rather than extra?

'source': iris_path,
'extra': csv_kwargs
}
})

This comment has been minimized.

@kwmsmith

kwmsmith Apr 15, 2016

Member

Style nit (here and above) please reformat with the following style:

blob = serial.dumps({'iris': {'source': iris_path,
                              'extra': csv_kwargs}})

We've settled on this indentation style for Blaze going forward, and are gradually updating files to conform.

Rami Chowdhury
Make Blaze server `/add` endpoint more flexible
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.

@necaris necaris force-pushed the necaris:expand-add-endpoint branch from 0fd06fa to f1be029 Apr 15, 2016

@necaris

This comment has been minimized.

necaris commented Apr 15, 2016

@kwmsmith Thanks for the quick review! Addressed your comments -- added args, renamed extra, and updated styles 😄

@kwmsmith

This comment has been minimized.

Member

kwmsmith commented Apr 15, 2016

While we're on the topic, @necaris, one gap here is that there's no .add() method on the client side, which is a natural extension to this. Could you look into adding that method as well? It looks to be straightforward, but if not, feel free to say so and we can defer to later.

Rami Chowdhury added some commits Apr 16, 2016

Rami Chowdhury
Add `add` method to ``blaze.server.client.Client``
Add a method to ``blaze.server.client.Client`` to allow programmatic
addition of datasets to the Blaze server, using the new `/add` endpoint.
@necaris

This comment has been minimized.

necaris commented Apr 16, 2016

@kwmsmith Is 74416c6 what you had in mind?

@necaris

This comment has been minimized.

necaris commented Apr 19, 2016

@kwmsmith FYI, I've added more tests as well as support for an imports key in the payload dictionary, to bring it up to full parity with the server YAML file.

@llllllllll

This comment has been minimized.

Member

llllllllll commented Apr 19, 2016

I am sort of afraid of having /add enabled by defaut, especially now that users can send imports. This adds a lot of attack surface to the blaze server. In our blaze server deployment we specifically needed to wrap out dataset in a mappingproxy to prevent datasets from being added, I think it might be nice to have a flag to Server/blaze_api.` that turns this feature off.

@kwmsmith

This comment has been minimized.

Member

kwmsmith commented Apr 19, 2016

I think it might be nice to have a flag to Server/blaze_api.` that turns this feature off.

@llllllllll good to know, and good input. So would an allow_add flag that's by default False, and that's overridable when firing up the blaze server instance, address this adequately?

@necaris

This comment has been minimized.

necaris commented Apr 19, 2016

@llllllllll @kwmsmith I've added an allow_add flag to the blaze.server.Server constructor -- it defaults to False, and if it's not set then the /add route is not defined at all. This results in requests for /add returning a 404 Not Found error.

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?

Rami Chowdhury added some commits Apr 19, 2016

Rami Chowdhury
Allow specifying modules that must be imported
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.
Rami Chowdhury
Isolate `/add` endpoint behind a flag
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 necaris force-pushed the necaris:expand-add-endpoint branch from 614120e to 1b462d4 Apr 19, 2016

Rami Chowdhury
@kwmsmith

This comment has been minimized.

Member

kwmsmith commented Apr 22, 2016

@necaris looks good -- the only addition is to add a commandline argument to the blaze-server entry point to flip the allow_add bit:

https://github.com/blaze/blaze/blob/master/blaze/server/spider.py#L148-L169
@necaris

This comment has been minimized.

necaris commented Apr 22, 2016

@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',

This comment has been minimized.

@kwmsmith

kwmsmith Apr 22, 2016

Member

It's good this is a long option name to make it less likely to trigger accidentally.

This comment has been minimized.

@necaris

necaris Apr 22, 2016

😄 I considered -a but I figured that would be too easy to do by mistake!

@kwmsmith kwmsmith added this to the 0.11 milestone Apr 22, 2016

@kwmsmith

This comment has been minimized.

Member

kwmsmith commented Apr 26, 2016

@necaris LGTM, thanks!

@kwmsmith kwmsmith merged commit a6b9f99 into blaze:master Apr 26, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.03%) to 89.278%
Details

@necaris necaris deleted the necaris:expand-add-endpoint branch Apr 26, 2016

@kwmsmith kwmsmith modified the milestones: 0.11, 0.10.2 Jun 8, 2016

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