Actor API Updating #449

mgoffin · 2015-04-07T03:21:55Z

This change enables updating Actors through the API.

In order to perform an update, you need to send a PATCH request. The request will go into a core API function from tastypie. The request needs to have an "action" parameter set to the action you want to take. From here there will be a lookup to determine if the action is supported for the TLO you are looking to modify. If so, it will take all of the rest of the parameters provides and pass them directly to the handler which handles that action.

In order to support this, handlers need to be modified in a few ways:

Any argument which is the ObjectId of the TLO being worked on needs to be standardized to use the same name (which will be id_).
The reason this needs to be standardized is so we can be 100% accurate in ensuring that a user cannot pass in another ObjectId and make changes, and so we can ensure the user's request works directly on the TLO in question.
Any argument which is the username of the user performing the action needs to be standardized to use the same name (which will be user).
The reason this needs to be standardized is so we can be 100% accurate in ensuring that the user modifying the TLO is the same user who is making the request (instead of passing in an analyst or username in the parameters and it being overwritten).
**kwargs needs to be added to each handler. This ensures any oddball parameters which get passed along (whether from tastypie internals or from the user's request) are dropped on the floor and ignored.

As such, the API for what to update and how becomes the docstrings for all of the handlers we enabled PATCH for.

There is also a "Common" action group which are things each TLO can have updated (like bucket list, campaign, etc.). There are some interesting things that can happen here depending on what we expose. For example, there's a handler for deleting a TLO but it would make more sense to use the DELETE request. You can add a relationship as an action but we have an API for that already. So there will need to be some design decisions about what we do and do not restrict from the handler side.

The patch API needs to be able to determine the type and id automatically from the request URL and not rely on the user providing that information in the request data. This requires us to know where in the data dictionary to stuff the id once we've derived it. To do this handlers need to be standardized so that common argument names are always the same. This currently includes: - id_ - user The user is important for patch as we don't want someone to be able to pass in a different user. We need to be able to overwrite any user they might try to pass in with the authenticated user making the request.

Common actions are ones that apply to any TLO. Had to change the logic such that if we don't find the action in the TLO's specific actions, we check Common.

mgoffin · 2015-04-07T03:22:43Z

I should also note that this doesn't expose every handler for updating everything about an Actor (like campaign, bucket lists, etc.). A lot of those are in core or in another TLO directory so they haven't been added yet.

ghost · 2015-04-09T15:18:20Z

crits/core/api.py

+            },
+        }
+
+        path = bundle.request.path[:-1].split('/')


Is there a programmatic way to get the part of the API being accessed instead of string mangling? Valid (though odd) paths such as "crits/api/v1/emails///" would probably cause issues here. Maybe looking at the meta attribute of bundle.obj for its type?

Conflicts: crits/actors/handlers.py crits/actors/views.py

mgoffin · 2015-04-16T22:06:18Z

DO NOT ACCEPT THIS YET.

For anyone who is up for testing this, I need a hand. Here's the rub:

I modified this PR slightly to remove the campaign handler exposure since I didn't actually modify those handlers yet. More importantly, I exposed the "run_service" handler so you can execute services against a TLO through the API. I added 'patch' to all of the TLOs so that this can be used.

Here's where stuff goes nuts...

I attempted to test this with a PATCH request, both via command-line cURL and with python requests. I was greeted with the following:

>>> r = requests.patch(url)
>>> r.text
u'{"error_message": "PATCH",
   "traceback": "Traceback (most recent call last):
File "/usr/local/Cellar/python/2.7.8_2/Frameworks/Python.framework/Versions/2.7/lib/python2.7/site-packages/tastypie/resources.py", line 201, in wrapper
    response = callback(request, *args, **kwargs)
File "/usr/local/Cellar/python/2.7.8_2/Frameworks/Python.framework/Versions/2.7/lib/python2.7/site-packages/tastypie/resources.py", line 441, in dispatch_detail
    return self.dispatch('detail', request, **kwargs)
File "/usr/local/Cellar/python/2.7.8_2/Frameworks/Python.framework/Versions/2.7/lib/python2.7/site-packages/django_tastypie_mongoengine-0.4.5-py2.7.egg/tastypie_mongoengine/resources.py", line 385, in dispatch
    assert request.method.lower() not in ('put', 'post', 'patch'), request.method
AssertionError: PATCH"}'
>>> r.request.method.lower()
'patch'
>>>

This seemed very odd to me so I went back to master and attempted a POST and I got the same thing!. I suspect we would have heard by now if others have issues with master, so I can't imagine the code is bad. The only thing different on my laptop is the latest round of OSX patches.

If anyone has a chance and can test this branch, please do so and let me know how it goes. I'm still trying to track down this bug. I have a SO question out as well as a comment in a Github issue for django-tastypie-mongoengine. No bites yet. I've tried newer versions of said python library and nothing changes.

ghost · 2015-04-20T13:37:25Z

Digging through some old brain cells here from working with this library...

If you don't provide a body/data in the request (you didn't in your example) the only valid method is a GET request. All the others expect data to be sent with the request. So you fail the validation check.

Update:
There it is. If there's no body then the dispatch function fails you on your request and returns an error: https://github.com/wlanslovenija/django-tastypie-mongoengine/blob/v0.4.5/tastypie_mongoengine/resources.py#L382

mgoffin · 2015-04-20T13:57:22Z

@dsnellgrove that is quite annoying. It winds up working if you pass everything except for the username and api_key as data. I found a code issue after you pointed me in the right direction though, and now you can execute services through the API! :D Thanks!

If we don't find the key in the actions dict, default to 'Common' and fail if we don't find the action in there.

Using proper django methods, get the resource name and pk to lookup the object instead of parsing strings and hoping for the best.

Actor API Updating

Lambdanaut · 2015-05-14T19:17:29Z

A bump in the road I hit when experimenting was that tastypie won't authenticate with form parameters for PATCH requests. You have to put the username and api_key in the query string.

Lambdanaut · 2015-05-15T20:00:01Z

You get a strange error if you don't supply an actor id in the request:

{
    "error_message": "'DatabaseWrapper' object has no attribute 'Database'",
    "traceback":
Traceback (most recent call last):
  File "/usr/lib/python2.7/wsgiref/handlers.py", line 86, in run
    self.finish_response()
  File "/usr/lib/python2.7/wsgiref/handlers.py", line 131, in finish_response
    self.close()
  File "/usr/lib/python2.7/wsgiref/simple_server.py", line 36, in close
    SimpleHandler.close(self)
  File "/usr/lib/python2.7/wsgiref/handlers.py", line 259, in close
    self.result.close()
  File "/usr/local/lib/python2.7/dist-packages/django/http/response.py", line 305, in close
    signals.request_finished.send(sender=self._handler_class)
  File "/usr/local/lib/python2.7/dist-packages/django/dispatch/dispatcher.py", line 185, in send
    response = receiver(signal=self, sender=sender, **named)
  File "/usr/local/lib/python2.7/dist-packages/django/db/__init__.py", line 91, in close_old_connections
    conn.abort()
  File "/usr/local/lib/python2.7/dist-packages/django/db/backends/__init__.py", line 383, in abort
    self.leave_transaction_management()
  File "/usr/local/lib/python2.7/dist-packages/django/db/backends/__init__.py", line 324, in leave_transaction_management
    if managed == self.get_autocommit():
  File "/usr/local/lib/python2.7/dist-packages/django/db/backends/__init__.py", line 331, in get_autocommit
    self.ensure_connection()
  File "/usr/local/lib/python2.7/dist-packages/django/db/backends/__init__.py", line 127, in ensure_connection
    self.connect()
  File "/usr/local/lib/python2.7/dist-packages/django/db/utils.py", line 86, in __exit__
    db_exc_type = getattr(self.wrapper.Database, dj_exc_type.__name__)
AttributeError: 'DatabaseWrapper' object has no attribute 'Database'

"}

Not really a bug but just something to keep in mind.

mgoffin · 2015-05-15T20:05:21Z

Would you be willing to share the request you are making? Seems odd as it's not hitting any of our code yet in that traceback.

Lambdanaut · 2015-05-15T20:18:10Z

Sure thing: curl --request PATCH 'http://127.0.0.1:8080/api/v1/actors/?username=admin&api_key=c188e07b477156ba8747613d0a14891242e9657b' --data 'x:x'

I've figured out that it happens after a call to CRITsApiKeyAuthentication.is_authenticated.

On another note, shouldn't line 648 of core/api.py be 'Actors': { to fit the /api/v1/actors/ route? Otherwise I can't call any of the actors actions because it looks for the Actors key rather than Actor

mgoffin · 2015-05-15T20:59:06Z

Ahh. If you are doing a PATCH you use the details URL (/api/v1/actors//) not the one for GET or POST. See if that helps at all!

mgoffin · 2015-05-15T21:05:04Z

I can't check at the moment from mobile but line 648 is that the dict for the update actions? If so that's just a key and an internal thing that you shouldn't need to know to use the API. You just need to know the value and we do the lookup.

Lambdanaut · 2015-05-15T21:16:34Z

@mgoffin right, it's the actions dict. The dict uses the resource name(capitlized with .title()) as the key. In this case the resource name is actors, which gets capitalized as Actors. The bug is that the dictionary key is Actor, not Actors, so it sets content['message'] to %s is not a valid action.

When I change the actions Actor key to Actors, it works.

I've been able to use the PATCH api to access Actors after making that code change :)

mgoffin · 2015-05-15T21:18:48Z

Ahh. Got it. If you throw up a PR I can try to test it out when j get some time and get that accepted :) thanks for checking it out!

Lambdanaut · 2015-05-15T21:31:54Z

Done. No problem.

I'd like to write a patch to allow the setting of releasability using the add_releasability handler in core. Is this something I can throw under the 'Common' actions?

brlogan · 2015-05-15T21:50:31Z

Issue #361 covers setting releasability via the API, but I don't think anyone has done any work towards it yet.

mgoffin · 2015-05-15T21:51:57Z

Yea so my goal is to redo the code base to make it so you can add handlers quickly. Here's the process:

find a handler you want to expose
if it takes an objectid make sure the argument is named in the new convention (look at the changes I made to Actors handlers) then make appropriate adjustments of the variable in the handler and make sure any views that use it pass it in properly.
it it takes an object type make sure of the same thing.
if it takes a username make sure the argument is renamed to "user" and check the same thing.
add a **kwargs to the handler so any errant parameters are dropped (again look at actors handlers)
expose the handler in the core API mapping dictionary.

If you want to take a stab go for it :) id love to have help!

Lambdanaut · 2015-05-18T15:00:20Z

Thanks :) I'll give it a shot.
Should the variable inputs for the handler always be of the form (_id, x, y, z, a, b, c, _user, **kwargs) with _id in the front and _user at the end, or is the ordering arbitrary?

mgoffin · 2015-05-18T15:15:22Z

I don't think we've decided on order (keeping it as-is for the sake of breaking as little as possible). As long as the new arguments are id_, type_, and user, we are good.

In the "Roles" branch, I'm doing something similar but the user argument is how the user object, not a string, so having it be user already would make things a lot easier once Roles gets this merged into it.

mgoffin added 7 commits March 17, 2015 00:29

Start of updating using PATCH via the API.

ac7248d

Merge branch 'master' into api_updating

05b8005

Merge branch 'master' into api_updating

eae4569

Add common actions for API updating.

6d3a112

Common actions are ones that apply to any TLO. Had to change the logic such that if we don't find the action in the TLO's specific actions, we check Common.

Merge branch 'master' into api_updating

78ef66c

Merge branch 'master' into api_updating

33617ce

ghost reviewed Apr 9, 2015
View reviewed changes

Merge branch 'master' into api_updating

c00ec0d

Conflicts: crits/actors/handlers.py crits/actors/views.py

mgoffin added 3 commits April 20, 2015 06:58

Fixing bug in action discovery.

9440486

If we don't find the key in the actions dict, default to 'Common' and fail if we don't find the action in there.

Merge branch 'master' into api_updating

06b0feb

Cleaner, more reliable way to get resource and pk from uri.

8ffa473

Using proper django methods, get the resource name and pk to lookup the object instead of parsing strings and hoping for the best.

mgoffin added a commit that referenced this pull request Apr 24, 2015

Merge pull request #449 from crits/api_updating

d8d4234

Actor API Updating

mgoffin merged commit d8d4234 into master Apr 24, 2015

mgoffin deleted the api_updating branch April 24, 2015 02:59

Lambdanaut mentioned this pull request May 15, 2015

Changes actions API key from 'Actor' => 'Actors' #504

Merged

armtash mentioned this pull request Dec 1, 2015

API Enhancement #664

Merged

dreardon mentioned this pull request Dec 30, 2015

Update Signature TLO, Take Advantage of the PATCH API #675

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Actor API Updating #449

Actor API Updating #449

mgoffin commented Apr 7, 2015

mgoffin commented Apr 7, 2015

ghost Apr 9, 2015

mgoffin commented Apr 16, 2015

ghost commented Apr 20, 2015

mgoffin commented Apr 20, 2015

Lambdanaut commented May 14, 2015

Lambdanaut commented May 15, 2015

mgoffin commented May 15, 2015

Lambdanaut commented May 15, 2015

mgoffin commented May 15, 2015

mgoffin commented May 15, 2015

Lambdanaut commented May 15, 2015

mgoffin commented May 15, 2015

Lambdanaut commented May 15, 2015

brlogan commented May 15, 2015

mgoffin commented May 15, 2015

Lambdanaut commented May 18, 2015

mgoffin commented May 18, 2015

Actor API Updating #449

Actor API Updating #449

Conversation

mgoffin commented Apr 7, 2015

mgoffin commented Apr 7, 2015

ghost Apr 9, 2015

Choose a reason for hiding this comment

mgoffin commented Apr 16, 2015

ghost commented Apr 20, 2015

mgoffin commented Apr 20, 2015

Lambdanaut commented May 14, 2015

Lambdanaut commented May 15, 2015

mgoffin commented May 15, 2015

Lambdanaut commented May 15, 2015

mgoffin commented May 15, 2015

mgoffin commented May 15, 2015

Lambdanaut commented May 15, 2015

mgoffin commented May 15, 2015

Lambdanaut commented May 15, 2015

brlogan commented May 15, 2015

mgoffin commented May 15, 2015

Lambdanaut commented May 18, 2015

mgoffin commented May 18, 2015