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

Error checking when adding to blaze server. #1459

Merged
merged 5 commits into from Mar 23, 2016

Conversation

Projects
None yet
2 participants
@kwmsmith
Member

kwmsmith commented Mar 23, 2016

Does basic error checking when adding a new data set to a blaze server (/add endpoint):

  • prevent adding a dataset when there's a name collision with existing dataset;
  • check that the data is discoverable to verify the data is loadable and queryable;
  • capture any other exception and return appropriate error code and traceback string.

I also reformatted the indentation in other tests for consistency.

@kwmsmith kwmsmith added this to the 0.10 milestone Mar 23, 2016

_get_auth.cache[app] = (
_get_option('authorization', options, None) or (lambda a: True)
)
_get_format.cache[app] = dict((f.name, f) for f in _get_option('formats', options))

This comment has been minimized.

@llllllllll

llllllllll Mar 23, 2016

Member

this should probably be a dict comprehension now

@llllllllll

This comment has been minimized.

Member

llllllllll commented Mar 23, 2016

It is hard to see what actually changed here, could you split this into two commits?

@llllllllll

This comment has been minimized.

Member

llllllllll commented Mar 23, 2016

Also, since this is mainly a style change, I would like to say that I don't find dict(key=value) as clear as {'key': value}. For dictionary literals I think this violates the "one obvious way to do something" principle.

resp = empty_server.post('/add',
headers=mimetype(serial),
data=payload)
assert resp.status_code == 409 # CONFLICT

This comment has been minimized.

@llllllllll

llllllllll Mar 23, 2016

Member

are these error codes not defined somewhere, and if not, should we make a small namespace for them like: rc.CONFLICT

@kwmsmith kwmsmith force-pushed the kwmsmith:bugfix/blaze-server-add-errors branch from ea78b32 to 1a7fda0 Mar 23, 2016

@kwmsmith

This comment has been minimized.

Member

kwmsmith commented Mar 23, 2016

@llllllllll I split the commit up; 1a7fda0 is the core of the bugfixes, while 80da240 is the style and indentation fixup.

@pytest.mark.parametrize('serial', all_formats)
def test_add_data_twice_error(empty_server, serial):
with temp_server():

This comment has been minimized.

@llllllllll

llllllllll Mar 23, 2016

Member

should this be a pytest.yield_fixture?

# Try to add to existing 'iris'
resp = empty_server.post('/add',
headers=mimetype(serial),
data=payload)

This comment has been minimized.

@llllllllll

llllllllll Mar 23, 2016

Member

should we use different payloads and assert that even though we got a CONFLICT that we are still served the data from the original payload?

This comment has been minimized.

@kwmsmith

kwmsmith Mar 23, 2016

Member

The CONFLICT is triggered on the name of the dataset, not the contents; I'm fine if we keep the payload the same.

I'll add the test that the server still serves the original.

" it is not a mutable mapping (dictionary like).")
if not isinstance(payload, collections.MutableMapping):
return (payload_not_mm_msg % type(payload), 422)
if len(payload) > 1:

This comment has been minimized.

@llllllllll

llllllllll Mar 23, 2016

Member

Why is this blocked, could we not change the code below to be:

for name, resource_uri in payload.items():
    ....

edit: I guess this makes error handling harder, maybe we could do it transactionally?

This comment has been minimized.

@kwmsmith

kwmsmith Mar 23, 2016

Member

I blocked this since for now, our only usecases are adding a single dataset at a time. Generalizing to multiple additions in one /add would complicate things, as you point out.

I'm fine generalizing once there's a clear need, but for now, I'm pleading YAGNI :)

This comment has been minimized.

@llllllllll

llllllllll Mar 23, 2016

Member

That sounds good, I haven't had a use for /add yet so I don't mind keeping it minimal

kwmsmith added some commits Mar 23, 2016

@kwmsmith kwmsmith merged commit 640fe88 into blaze:master Mar 23, 2016

1 of 2 checks passed

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

@kwmsmith kwmsmith deleted the kwmsmith:bugfix/blaze-server-add-errors branch Mar 23, 2016

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