Merge lp:~rackspace-titan/glance/wsgi-refactor into lp:~hudson-openstack/glance/trunk
- wsgi-refactor
- Merge into trunk
Status: | Merged |
---|---|
Approved by: | Rick Harris |
Approved revision: | 148 |
Merged at revision: | 140 |
Proposed branch: | lp:~rackspace-titan/glance/wsgi-refactor |
Merge into: | lp:~hudson-openstack/glance/trunk |
Diff against target: |
1199 lines (+558/-226) 10 files modified
glance/api/v1/__init__.py (+4/-4) glance/api/v1/images.py (+154/-86) glance/common/exception.py (+26/-0) glance/common/wsgi.py (+133/-101) glance/registry/client.py (+12/-2) glance/registry/server.py (+20/-14) glance/utils.py (+0/-18) tests/functional/test_curl_api.py (+1/-1) tests/unit/test_api.py (+26/-0) tests/unit/test_wsgi.py (+182/-0) |
To merge this branch: | bzr merge lp:~rackspace-titan/glance/wsgi-refactor |
Related bugs: | |
Related blueprints: |
Port Nova Updated WSGI Code
(High)
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Rick Harris (community) | Approve | ||
Brian Lamar (community) | Approve | ||
Jay Pipes (community) | Approve | ||
Review via email: mp+64311@code.launchpad.net |
Commit message
Description of the change
- refactoring wsgi code to divide deserialization, controller, serialization among different objects
- Resource object acts as coordinator
- tests are coming, this is for review purposes
Brian Waldon (bcwaldon) wrote : | # |
Jay Pipes (jaypipes) wrote : | # |
Hey Brian,
The reason that test is failing is because of this change:
148 - image_id = image_meta['id']
149 - content_type = req.headers.
150 - if content_type != 'application/
151 - self._safe_
152 - msg = ("Content-Type must be application/
153 + try:
154 + req.get_
155 + except exception.
156 + msg = "Content-Type must be application/
The issue is that the image is registered and set to a 'queued' status before this code is run. The call to _safe_kill() is what (correctly) sets the image status to 'killed' and returns the error message. You need to add the call to _safe_kill() back to the new except InvalidContentType block.
Hope that helps,
jay
Brian Waldon (bcwaldon) wrote : | # |
Fixed the test case.
Jay Pipes (jaypipes) wrote : | # |
Hey again Brian, here's a full review...
Overall, excellent and clean work. As mentioned on IRC, in the future, try to submit style/formattin
Design Discussion
-----------------
The one big design/architecture thing that I'd like to suggest a change for is making the responsibility of the Serializer subclasses very specific to transforming the dicts that may compose the body of a Response object. As they currently stand, the Serializer subclasses look to be doing more work that may more appropriately belong in the Controller.
I'll work through an example.
Consider the ImageSerializer class (shortened for brevity):
357 +class ImageSerializer
...
360 + def _inject_
361 + location = self._get_
362 + response.
363 +
364 + def _inject_
365 + response.
366 +
367 + def _inject_
...
379 + headers = utils.image_
380 +
381 + for k, v in headers.items():
382 + response.
383 +
384 + def _get_image_
...
386 + return "/v1/images/%s" % image_meta['id']
...
396 + def show(self, result):
397 + image_meta = result[
398 +
399 + response = webob.Response(
400 + # Using app_iter blanks content-length, so we set it here...
401 + response.
402 + response.
403 +
404 + self._inject_
405 + self._inject_
406 + self._inject_
...
419 + def create(self, result):
420 + image_meta = result[
421 + response = webob.Response()
422 + response.status = httplib.CREATED
423 + response.
424 + response.body = self.to_
425 + self._inject_
426 + self._inject_
427 + return response
I think that I would prefer that the ImageSerializer class NOT be responsible for:
a) creating a webob.Response object
b) modifying any headers of the Response object other than Content-Type
I think that all the Serializer subclasses should be doing is transforming the Response object's body attribute to the necessary serialized format, and setting the Content-Type header appropriately.
All the other activities that are happening in ImageSerializer I believe should be in the Controller object, since the Controller is responsible for setting various headers (ETag, Location, etc) and for creating the Response object itself...
So, I think I'd prefer the ImageSerializer to look like this:
class ImageSerializer
def _serialize(self, response):
meta = response....
Mark Washenberger (markwash) wrote : | # |
>
> I think that I would prefer that the ImageSerializer class NOT be responsible
> for:
>
> a) creating a webob.Response object
> b) modifying any headers of the Response object other than Content-Type
>
> I think that all the Serializer subclasses should be doing is transforming the
> Response object's body attribute to the necessary serialized format, and
> setting the Content-Type header appropriately.
>
> All the other activities that are happening in ImageSerializer I believe
> should be in the Controller object, since the Controller is responsible for
> setting various headers (ETag, Location, etc) and for creating the Response
> object itself...
>
I think it is fitting for the Serializer to be responsible for creating the Webob Response. It seems like part of the idea here is to take some of the wsgi- and http api- specific responsibilities away from the controller so that it can focus on its responsibilities rather than its context. In my mind I see the ImageSerializer as responsible for translating image responses (which are really domain-model objects) into something that a wsgi server can present.
>
> Also, have you thought about how you will deal with multiple serializers? How
> do you plan to support a resource that can be translated into both JSON and
> XML?
I believe Waldon has given this a good bit of thought, but I actually encouraged him to leave out code going towards this direction. Let's XP it up: do only the minimum required for the features needed today. I believe the design to incorporate xml will be cleaner if we do it when we are actually trying to support it.
Cheers :-)
Jay Pipes (jaypipes) wrote : | # |
> >
> > I think that I would prefer that the ImageSerializer class NOT be
> responsible
> > for:
> >
> > a) creating a webob.Response object
> > b) modifying any headers of the Response object other than Content-Type
> >
> > I think that all the Serializer subclasses should be doing is transforming
> the
> > Response object's body attribute to the necessary serialized format, and
> > setting the Content-Type header appropriately.
> >
> > All the other activities that are happening in ImageSerializer I believe
> > should be in the Controller object, since the Controller is responsible for
> > setting various headers (ETag, Location, etc) and for creating the Response
> > object itself...
> >
>
> I think it is fitting for the Serializer to be responsible for creating the
> Webob Response. It seems like part of the idea here is to take some of the
> wsgi- and http api- specific responsibilities away from the controller so that
> it can focus on its responsibilities rather than its context. In my mind I see
> the ImageSerializer as responsible for translating image responses (which are
> really domain-model objects) into something that a wsgi server can present.
I strongly disagree :) Serializers should serialize. Nothing more. Controllers *control* things. They have the requisite information to set non-content-type headers, to create the appropriate Response or HTTPException objects. Moving that code into the Serializer class just doesn't make much sense to me...
> > Also, have you thought about how you will deal with multiple serializers?
> How
> > do you plan to support a resource that can be translated into both JSON and
> > XML?
>
> I believe Waldon has given this a good bit of thought, but I actually
> encouraged him to leave out code going towards this direction. Let's XP it up:
> do only the minimum required for the features needed today. I believe the
> design to incorporate xml will be cleaner if we do it when we are actually
> trying to support it.
Sure, no probs. I was just wondering if Brian had considered it :)
-jay
Mark Washenberger (markwash) wrote : | # |
> I strongly disagree :) Serializers should serialize. Nothing more. Controllers
> *control* things. They have the requisite information to set non-content-type
> headers, to create the appropriate Response or HTTPException objects. Moving
> that code into the Serializer class just doesn't make much sense to me...
>
It seems that there are two issues: proper naming, and a proper division of responsibility.
Perhaps Serializer is not the right name for this set of functionality. But I hope you will agree that "Controller" is a bit too broad of a term to fit into a system with a good division of responsibility. I am certainly open to renaming both. But maybe we ought not rename Controller, so that we remain consistent with other such units across Openstack. However, our naming mistake doesn't require us to make a similar mistake in the division of responsibility just to conform.
Although the naming is extremely important, if we set it aside I believe that leaving it to the "postprocessor" of the controller's output (currently the serializer) to create the WebOb response is a powerful approach--it lets the Controller focus on the problem domain rather than on wsgi details.
I hope some of that makes sense! :-)
Jay Pipes (jaypipes) wrote : | # |
> Perhaps Serializer is not the right name for this set of functionality. But I
> hope you will agree that "Controller" is a bit too broad of a term to fit into
> a system with a good division of responsibility.
Not really... it's called MVC for a reason... The Controller deals with logic and business rules, and that's what *most* of the stuff that was placed into the ImageSerializer was doing. And I think that is incorrect. Logic should stay in the Controller. The Controller should create and populate a Response object and return it. Middleware (both paste-constructed and manually created as in the case of serializers) should modify the Response object. That's it. No logic.
> I am certainly open to
> renaming both. But maybe we ought not rename Controller, so that we remain
> consistent with other such units across Openstack.
I wasn't suggesting we rename Controller :) Like I said, it's an integral part of the MVC pattern.
> However, our naming mistake
> doesn't require us to make a similar mistake in the division of responsibility
> just to conform.
I don't understand this, sorry. I think I've explained why a piece of middleware should not be constructing the Response object. That's the responsibility of the center-most object in the chain: the Controller.
> Although the naming is extremely important, if we set it aside I believe that
> leaving it to the "postprocessor" of the controller's output (currently the
> serializer) to create the WebOb response is a powerful approach--it lets the
> Controller focus on the problem domain rather than on wsgi details.
Postprocessing what? Oh, that's right... postprocessing the *Response* object :) Postprocessing implies that you are processing a previously-
> I hope some of that makes sense! :-)
Not really, but I still dig you man :)
-jay
Brian Waldon (bcwaldon) wrote : | # |
> I think that I would prefer that the ImageSerializer class NOT be responsible
> for:
>
> a) creating a webob.Response object
I moved this into wsgi.Resource
> b) modifying any headers of the Response object other than Content-Type
I'm going to take some time to figure this out. I'll have another commit for you later today.
> Tiny Nits/Fixes
> ---------------
>
> Here are some little nits to correct before an approve.
>
> 1) docstring stuff
>
> 445 + """Base Nova Exception
> 446 +
> 447 + To correctly use this class, inherit from it and define
> 448 + a 'message' property. That message will get printf'd
> 449 + with the keyword arguments provided to the constructor.
> 450 +
> 451 + """
>
> Please use this style instead:
>
> """
> Base Nova Exception
>
> To correctly use this class, inherit from it and define
> a 'message' property. That message will get printf'd
> with the keyword arguments provided to the constructor.
> """
>
> No need for extra newline at the end, and please put the short description on
> a new line after the opening """
>
> Some of the new docstrings are correct, others aren't... please do a quick
> run-through to check they are all consistent like the above style.
>
> Also, we're Glance, not Nova ;P
Copy/paste error. Sorry for that. I fixed the problems I introduced. There are a bunch of docstrings that don't follow your suggestion in registry/server.py (probably elsewhere as well) that I did not fix because I did not introduce them in this merge prop. Maybe a style cleanup is in order?
> 2)
>
> 475 import datetime
> 476 -
> 477 import eventlet
>
> eventlet is a 3rd party library. We separate stdlib modules from 3rd party
> modules with a single newline.
Ah, I thought it was just separation of non-glance and glance modules. Fixed that.
> 3) few little typos
>
> 67 + :retval similiar to 'show' method but without image_data
>
> s/similiar/similar
Fixed. I do that all the time :(
> 4)
>
> 133 + if 'location' in image_meta and image_meta[
> 264 + if 'location' in image_meta and image_meta[
>
> can be shortened to:
>
> if image_meta.
Fixed. I took it a step farther and stored location in a variable before the conditional. We needed to use it again immediately anyways, so I removed the duplication of reading from the image_meta dict.
> 5)
>
> 342 + def create(self, request):
> 343 + result = {}
> 344 + result[
> 345 + data = request.body if self.has_
> 346 + result[
> 347 + return result
> 348 +
> 349 + def update(self, request):
> 350 + result = {}
> 351 + result[
> 352 + data = request.body if self.has_
> 353 + result[
> 354 + return result
>
> Little bit of DRY cleanup needed there... something like this would do:
Good catch. I didn't even notice that.
> 6) the wsgi.Resource constructor
>
> 665 + def __init__(self, des...
Brian Waldon (bcwaldon) wrote : | # |
Looking for suggestions on separating out header "serialization" code.
Jay Pipes (jaypipes) wrote : | # |
Let's work on cleaning up the header serialization code in a later patch. Approved from me.
Rick Harris (rconradharris) wrote : | # |
Overall looks good. Some thoughts:
> 425 + self._inject_
> 426 + self._inject_
> 427 + return response
Super nit-picky here, but might be worth DRY'ing up some of this code since
it's repeated quite a bit.
> 582 + return (request.
> 583 + or 'transfer-encoding' in request.headers
This statement was a little hard to read at-first. Once I understood it, it
was clear enough. That said, given that we're, for the most part, optimizing
for readability, maybe it would be clearer as:
if 'transfer-encoding' in request.headers:
return True
elif request.
raise Exception(
return request.
> 683 + if type(action_result) is dict:
Duck-typing might be preferable here. Do you know specifically what dict
methods the serializer is using; if you're not sure, you could always check
for presence of `keys()`
if hasattr(
blah
(This is a really small nit, but if we ever need to do something tricky by
subclassing `dict`, this would keep the code working)
> + """Find action-spefic method on self and call it."""
s/spefic/specific/
> 695 + except Exception:
> 696 + method = getattr(obj, 'default')
Should this handler be this broad? Would AttributeError work here?
If that's the case, we could do:
method = getattr(obj, action, 'default')
Brian Waldon (bcwaldon) wrote : | # |
> Overall looks good. Some thoughts:
>
> > 425 + self._inject_
> > 426 + self._inject_
> > 427 + return response
>
> Super nit-picky here, but might be worth DRY'ing up some of this code since
> it's repeated quite a bit.
I would factor this out a bit more if it were clear exactly how to do it. Now, I could make a method called inject_
> > 582 + return (request.
> 0) \
> > 583 + or 'transfer-encoding' in request.headers
>
> This statement was a little hard to read at-first. Once I understood it, it
> was clear enough. That said, given that we're, for the most part, optimizing
> for readability, maybe it would be clearer as:
>
>
> if 'transfer-encoding' in request.headers:
> return True
> elif request.
> raise Exception(
> "Transfer-
>
> return request.
I cleaned it up with a longer if/elif block, while keeping the same logic. This is a boolean check, so I didn't want to throw an exception.
> > 683 + if type(action_result) is dict:
>
> Duck-typing might be preferable here. Do you know specifically what dict
> methods the serializer is using; if you're not sure, you could always check
> for presence of `keys()`
>
> if hasattr(
> blah
>
> (This is a really small nit, but if we ever need to do something tricky by
> subclassing `dict`, this would keep the code working)
How about I just assume I can serialize it, and if there is an exception I will just return the original results? I checked in a possible implementation of this. Tell me what you think.
> > + """Find action-spefic method on self and call it."""
> s/spefic/specific/
Good catch. Fixed.
> > 695 + except Exception:
> > 696 + method = getattr(obj, 'default')
>
> Should this handler be this broad? Would AttributeError work here?
>
> If that's the case, we could do:
>
> method = getattr(obj, action, 'default')
I added AttributeError to the except statement. I can't use 'default' as a third argument there because it expects a callable. I would have to pass in obj.default, and I can't assume that exists.
I also caught some issues in the wsgi.Resource docstring and cleaned that up.
- 147. By Brian Waldon
-
refactoring from Rick's comments
Brian Lamar (blamar) wrote : | # |
164: HTTPBadRequest needs explanation=msg to actually display the passed-in message.
680: Not sure if it will hinder readability, but this blank line seems out of place.
716 + try:
717 + del args['controller']
718 + except KeyError:
719 + pass
I've been playing with this replacement logic/syntax:
args.pop(
It's not as explicit, which I don't like, but it gets the job done and isn't too cryptic.
Starting at 1064, should those each be it's own test?
I like your style/spacing! :)
Brian Waldon (bcwaldon) wrote : | # |
> 164: HTTPBadRequest needs explanation=msg to actually display the passed-in
> message.
Fixed.
> 680: Not sure if it will hinder readability, but this blank line seems out of
> place.
Fixed.
> 716 + try:
> 717 + del args['controller']
> 718 + except KeyError:
> 719 + pass
>
> I've been playing with this replacement logic/syntax:
>
> args.pop(
>
> It's not as explicit, which I don't like, but it gets the job done and isn't
> too cryptic.
I could go either way, and originally I did have it this way. When I merge prop'd this code in Nova, I was asked to change it to the try/except style. I hope it's okay to leave it.
> Starting at 1064, should those each be it's own test?
Yes, sir.
> I like your style/spacing! :)
I like your style, too.
- 148. By Brian Waldon
-
refactoring for Brian
Brian Lamar (blamar) : | # |
Rick Harris (rconradharris) wrote : | # |
Looks good, nice job Brian.
Preview Diff
1 | === modified file 'glance/api/v1/__init__.py' |
2 | --- glance/api/v1/__init__.py 2011-05-11 23:03:51 +0000 |
3 | +++ glance/api/v1/__init__.py 2011-06-17 20:46:38 +0000 |
4 | @@ -32,11 +32,11 @@ |
5 | def __init__(self, options): |
6 | self.options = options |
7 | mapper = routes.Mapper() |
8 | - controller = images.Controller(options) |
9 | - mapper.resource("image", "images", controller=controller, |
10 | + resource = images.create_resource(options) |
11 | + mapper.resource("image", "images", controller=resource, |
12 | collection={'detail': 'GET'}) |
13 | - mapper.connect("/", controller=controller, action="index") |
14 | - mapper.connect("/images/{id}", controller=controller, action="meta", |
15 | + mapper.connect("/", controller=resource, action="index") |
16 | + mapper.connect("/images/{id}", controller=resource, action="meta", |
17 | conditions=dict(method=["HEAD"])) |
18 | super(API, self).__init__(mapper) |
19 | |
20 | |
21 | === modified file 'glance/api/v1/images.py' |
22 | --- glance/api/v1/images.py 2011-05-17 13:34:42 +0000 |
23 | +++ glance/api/v1/images.py 2011-06-17 20:46:38 +0000 |
24 | @@ -24,7 +24,7 @@ |
25 | import logging |
26 | import sys |
27 | |
28 | -from webob import Response |
29 | +import webob |
30 | from webob.exc import (HTTPNotFound, |
31 | HTTPConflict, |
32 | HTTPBadRequest) |
33 | @@ -46,7 +46,7 @@ |
34 | 'size_min', 'size_max'] |
35 | |
36 | |
37 | -class Controller(wsgi.Controller): |
38 | +class Controller(object): |
39 | |
40 | """ |
41 | WSGI controller for images resource in Glance v1 API |
42 | @@ -80,7 +80,7 @@ |
43 | * checksum -- MD5 checksum of the image data |
44 | * size -- Size of image data in bytes |
45 | |
46 | - :param request: The WSGI/Webob Request object |
47 | + :param req: The WSGI/Webob Request object |
48 | :retval The response body is a mapping of the following form:: |
49 | |
50 | {'images': [ |
51 | @@ -107,7 +107,7 @@ |
52 | """ |
53 | Returns detailed information for all public, available images |
54 | |
55 | - :param request: The WSGI/Webob Request object |
56 | + :param req: The WSGI/Webob Request object |
57 | :retval The response body is a mapping of the following form:: |
58 | |
59 | {'images': [ |
60 | @@ -155,29 +155,22 @@ |
61 | Returns metadata about an image in the HTTP headers of the |
62 | response object |
63 | |
64 | - :param request: The WSGI/Webob Request object |
65 | + :param req: The WSGI/Webob Request object |
66 | :param id: The opaque image identifier |
67 | + :retval similar to 'show' method but without image_data |
68 | |
69 | :raises HTTPNotFound if image metadata is not available to user |
70 | """ |
71 | - image = self.get_image_meta_or_404(req, id) |
72 | - |
73 | - res = Response(request=req) |
74 | - utils.inject_image_meta_into_headers(res, image) |
75 | - res.headers.add('Location', "/v1/images/%s" % id) |
76 | - res.headers.add('ETag', image['checksum']) |
77 | - |
78 | - return req.get_response(res) |
79 | + return { |
80 | + 'image_meta': self.get_image_meta_or_404(req, id), |
81 | + } |
82 | |
83 | def show(self, req, id): |
84 | """ |
85 | - Returns an iterator as a Response object that |
86 | - can be used to retrieve an image's data. The |
87 | - content-type of the response is the content-type |
88 | - of the image, or application/octet-stream if none |
89 | - is known or found. |
90 | + Returns an iterator that can be used to retrieve an image's |
91 | + data along with the image metadata. |
92 | |
93 | - :param request: The WSGI/Webob Request object |
94 | + :param req: The WSGI/Webob Request object |
95 | :param id: The opaque image identifier |
96 | |
97 | :raises HTTPNotFound if image is not available to user |
98 | @@ -192,31 +185,26 @@ |
99 | for chunk in chunks: |
100 | yield chunk |
101 | |
102 | - res = Response(app_iter=image_iterator(), |
103 | - content_type="application/octet-stream") |
104 | - # Using app_iter blanks content-length, so we set it here... |
105 | - res.headers.add('Content-Length', image['size']) |
106 | - utils.inject_image_meta_into_headers(res, image) |
107 | - res.headers.add('Location', "/v1/images/%s" % id) |
108 | - res.headers.add('ETag', image['checksum']) |
109 | - return req.get_response(res) |
110 | + return { |
111 | + 'image_iterator': image_iterator(), |
112 | + 'image_meta': image, |
113 | + } |
114 | |
115 | - def _reserve(self, req): |
116 | + def _reserve(self, req, image_meta): |
117 | """ |
118 | Adds the image metadata to the registry and assigns |
119 | an image identifier if one is not supplied in the request |
120 | - headers. Sets the image's status to `queued` |
121 | + headers. Sets the image's status to `queued`. |
122 | |
123 | - :param request: The WSGI/Webob Request object |
124 | + :param req: The WSGI/Webob Request object |
125 | :param id: The opaque image identifier |
126 | |
127 | :raises HTTPConflict if image already exists |
128 | :raises HTTPBadRequest if image metadata is not valid |
129 | """ |
130 | - image_meta = utils.get_image_meta_from_headers(req) |
131 | - |
132 | - if 'location' in image_meta: |
133 | - store = get_store_from_location(image_meta['location']) |
134 | + location = image_meta.get('location') |
135 | + if location: |
136 | + store = get_store_from_location(location) |
137 | # check the store exists before we hit the registry, but we |
138 | # don't actually care what it is at this point |
139 | self.get_store_or_400(req, store) |
140 | @@ -250,28 +238,27 @@ |
141 | will attempt to use that store, if not, Glance will use the |
142 | store set by the flag `default_store`. |
143 | |
144 | - :param request: The WSGI/Webob Request object |
145 | + :param req: The WSGI/Webob Request object |
146 | :param image_meta: Mapping of metadata about image |
147 | |
148 | :raises HTTPConflict if image already exists |
149 | :retval The location where the image was stored |
150 | """ |
151 | - image_id = image_meta['id'] |
152 | - content_type = req.headers.get('content-type', 'notset') |
153 | - if content_type != 'application/octet-stream': |
154 | - self._safe_kill(req, image_id) |
155 | - msg = ("Content-Type must be application/octet-stream") |
156 | + try: |
157 | + req.get_content_type('application/octet-stream') |
158 | + except exception.InvalidContentType: |
159 | + self._safe_kill(req, image_meta['id']) |
160 | + msg = "Content-Type must be application/octet-stream" |
161 | logger.error(msg) |
162 | - raise HTTPBadRequest(msg, content_type="text/plain", |
163 | - request=req) |
164 | + raise HTTPBadRequest(explanation=msg) |
165 | |
166 | - store_name = req.headers.get( |
167 | - 'x-image-meta-store', self.options['default_store']) |
168 | + store_name = req.headers.get('x-image-meta-store', |
169 | + self.options['default_store']) |
170 | |
171 | store = self.get_store_or_400(req, store_name) |
172 | |
173 | - logger.debug("Setting image %s to status 'saving'" |
174 | - % image_id) |
175 | + image_id = image_meta['id'] |
176 | + logger.debug("Setting image %s to status 'saving'" % image_id) |
177 | registry.update_image_metadata(self.options, image_id, |
178 | {'status': 'saving'}) |
179 | try: |
180 | @@ -304,11 +291,13 @@ |
181 | 'size': size}) |
182 | |
183 | return location |
184 | + |
185 | except exception.Duplicate, e: |
186 | msg = ("Attempt to upload duplicate image: %s") % str(e) |
187 | logger.error(msg) |
188 | self._safe_kill(req, image_id) |
189 | raise HTTPConflict(msg, request=req) |
190 | + |
191 | except Exception, e: |
192 | msg = ("Error uploading image: %s") % str(e) |
193 | logger.error(msg) |
194 | @@ -320,8 +309,8 @@ |
195 | Sets the image status to `active` and the image's location |
196 | attribute. |
197 | |
198 | - :param request: The WSGI/Webob Request object |
199 | - :param image_meta: Mapping of metadata about image |
200 | + :param req: The WSGI/Webob Request object |
201 | + :param image_id: Opaque image identifier |
202 | :param location: Location of where Glance stored this image |
203 | """ |
204 | image_meta = {} |
205 | @@ -333,9 +322,9 @@ |
206 | |
207 | def _kill(self, req, image_id): |
208 | """ |
209 | - Marks the image status to `killed` |
210 | + Marks the image status to `killed`. |
211 | |
212 | - :param request: The WSGI/Webob Request object |
213 | + :param req: The WSGI/Webob Request object |
214 | :param image_id: Opaque image identifier |
215 | """ |
216 | registry.update_image_metadata(self.options, |
217 | @@ -349,7 +338,7 @@ |
218 | Since _kill is meant to be called from exceptions handlers, it should |
219 | not raise itself, rather it should just log its error. |
220 | |
221 | - :param request: The WSGI/Webob Request object |
222 | + :param req: The WSGI/Webob Request object |
223 | :param image_id: Opaque image identifier |
224 | """ |
225 | try: |
226 | @@ -364,7 +353,7 @@ |
227 | and activates the image in the registry after a successful |
228 | upload. |
229 | |
230 | - :param request: The WSGI/Webob Request object |
231 | + :param req: The WSGI/Webob Request object |
232 | :param image_meta: Mapping of metadata about image |
233 | |
234 | :retval Mapping of updated image data |
235 | @@ -373,7 +362,7 @@ |
236 | location = self._upload(req, image_meta) |
237 | return self._activate(req, image_id, location) |
238 | |
239 | - def create(self, req): |
240 | + def create(self, req, image_meta, image_data): |
241 | """ |
242 | Adds a new image to Glance. Three scenarios exist when creating an |
243 | image: |
244 | @@ -399,32 +388,27 @@ |
245 | containing metadata about the image is returned, including its |
246 | opaque identifier. |
247 | |
248 | - :param request: The WSGI/Webob Request object |
249 | + :param req: The WSGI/Webob Request object |
250 | + :param image_meta: Mapping of metadata about image |
251 | + :param image_data: Actual image data that is to be stored |
252 | |
253 | - :raises HTTPBadRequest if no x-image-meta-location is missing |
254 | + :raises HTTPBadRequest if x-image-meta-location is missing |
255 | and the request body is not application/octet-stream |
256 | image data. |
257 | """ |
258 | - image_meta = self._reserve(req) |
259 | + image_meta = self._reserve(req, image_meta) |
260 | image_id = image_meta['id'] |
261 | |
262 | - if utils.has_body(req): |
263 | + if image_data is not None: |
264 | image_meta = self._upload_and_activate(req, image_meta) |
265 | else: |
266 | - if 'x-image-meta-location' in req.headers: |
267 | - location = req.headers['x-image-meta-location'] |
268 | + location = image_meta.get('location') |
269 | + if location: |
270 | image_meta = self._activate(req, image_id, location) |
271 | |
272 | - # APP states we should return a Location: header with the edit |
273 | - # URI of the resource newly-created. |
274 | - res = Response(request=req, body=json.dumps(dict(image=image_meta)), |
275 | - status=httplib.CREATED, content_type="text/plain") |
276 | - res.headers.add('Location', "/v1/images/%s" % image_id) |
277 | - res.headers.add('ETag', image_meta['checksum']) |
278 | - |
279 | - return req.get_response(res) |
280 | - |
281 | - def update(self, req, id): |
282 | + return {'image_meta': image_meta} |
283 | + |
284 | + def update(self, req, id, image_meta, image_data): |
285 | """ |
286 | Updates an existing image with the registry. |
287 | |
288 | @@ -433,29 +417,17 @@ |
289 | |
290 | :retval Returns the updated image information as a mapping |
291 | """ |
292 | - has_body = utils.has_body(req) |
293 | - |
294 | orig_image_meta = self.get_image_meta_or_404(req, id) |
295 | orig_status = orig_image_meta['status'] |
296 | |
297 | - if has_body and orig_status != 'queued': |
298 | + if image_data is not None and orig_status != 'queued': |
299 | raise HTTPConflict("Cannot upload to an unqueued image") |
300 | |
301 | - new_image_meta = utils.get_image_meta_from_headers(req) |
302 | try: |
303 | - image_meta = registry.update_image_metadata(self.options, |
304 | - id, |
305 | - new_image_meta, |
306 | - True) |
307 | - if has_body: |
308 | + image_meta = registry.update_image_metadata(self.options, id, |
309 | + image_meta, True) |
310 | + if image_data is not None: |
311 | image_meta = self._upload_and_activate(req, image_meta) |
312 | - |
313 | - res = Response(request=req, |
314 | - body=json.dumps(dict(image=image_meta)), |
315 | - content_type="text/plain") |
316 | - res.headers.add('Location', "/images/%s" % id) |
317 | - res.headers.add('ETag', image_meta['checksum']) |
318 | - return res |
319 | except exception.Invalid, e: |
320 | msg = ("Failed to update image metadata. Got error: %(e)s" |
321 | % locals()) |
322 | @@ -463,11 +435,13 @@ |
323 | logger.error(line) |
324 | raise HTTPBadRequest(msg, request=req, content_type="text/plain") |
325 | |
326 | + return {'image_meta': image_meta} |
327 | + |
328 | def delete(self, req, id): |
329 | """ |
330 | Deletes the image and all its chunks from the Glance |
331 | |
332 | - :param request: The WSGI/Webob Request object |
333 | + :param req: The WSGI/Webob Request object |
334 | :param id: The opaque image identifier |
335 | |
336 | :raises HttpBadRequest if image registry is invalid |
337 | @@ -527,3 +501,97 @@ |
338 | logger.error(msg) |
339 | raise HTTPBadRequest(msg, request=request, |
340 | content_type='text/plain') |
341 | + |
342 | + |
343 | +class ImageDeserializer(wsgi.JSONRequestDeserializer): |
344 | + """Handles deserialization of specific controller method requests.""" |
345 | + |
346 | + def _deserialize(self, request): |
347 | + result = {} |
348 | + result['image_meta'] = utils.get_image_meta_from_headers(request) |
349 | + data = request.body if self.has_body(request) else None |
350 | + result['image_data'] = data |
351 | + return result |
352 | + |
353 | + def create(self, request): |
354 | + return self._deserialize(request) |
355 | + |
356 | + def update(self, request): |
357 | + return self._deserialize(request) |
358 | + |
359 | + |
360 | +class ImageSerializer(wsgi.JSONResponseSerializer): |
361 | + """Handles serialization of specific controller method responses.""" |
362 | + |
363 | + def _inject_location_header(self, response, image_meta): |
364 | + location = self._get_image_location(image_meta) |
365 | + response.headers.add('Location', location) |
366 | + |
367 | + def _inject_checksum_header(self, response, image_meta): |
368 | + response.headers.add('ETag', image_meta['checksum']) |
369 | + |
370 | + def _inject_image_meta_headers(self, response, image_meta): |
371 | + """ |
372 | + Given a response and mapping of image metadata, injects |
373 | + the Response with a set of HTTP headers for the image |
374 | + metadata. Each main image metadata field is injected |
375 | + as a HTTP header with key 'x-image-meta-<FIELD>' except |
376 | + for the properties field, which is further broken out |
377 | + into a set of 'x-image-meta-property-<KEY>' headers |
378 | + |
379 | + :param response: The Webob Response object |
380 | + :param image_meta: Mapping of image metadata |
381 | + """ |
382 | + headers = utils.image_meta_to_http_headers(image_meta) |
383 | + |
384 | + for k, v in headers.items(): |
385 | + response.headers.add(k, v) |
386 | + |
387 | + def _get_image_location(self, image_meta): |
388 | + """Build a relative url to reach the image defined by image_meta.""" |
389 | + return "/v1/images/%s" % image_meta['id'] |
390 | + |
391 | + def meta(self, response, result): |
392 | + image_meta = result['image_meta'] |
393 | + self._inject_image_meta_headers(response, image_meta) |
394 | + self._inject_location_header(response, image_meta) |
395 | + self._inject_checksum_header(response, image_meta) |
396 | + return response |
397 | + |
398 | + def show(self, response, result): |
399 | + image_meta = result['image_meta'] |
400 | + |
401 | + response.app_iter = result['image_iterator'] |
402 | + # Using app_iter blanks content-length, so we set it here... |
403 | + response.headers.add('Content-Length', image_meta['size']) |
404 | + response.headers.add('Content-Type', 'application/octet-stream') |
405 | + |
406 | + self._inject_image_meta_headers(response, image_meta) |
407 | + self._inject_location_header(response, image_meta) |
408 | + self._inject_checksum_header(response, image_meta) |
409 | + |
410 | + return response |
411 | + |
412 | + def update(self, response, result): |
413 | + image_meta = result['image_meta'] |
414 | + response.body = self.to_json(dict(image=image_meta)) |
415 | + response.headers.add('Content-Type', 'application/json') |
416 | + self._inject_location_header(response, image_meta) |
417 | + self._inject_checksum_header(response, image_meta) |
418 | + return response |
419 | + |
420 | + def create(self, response, result): |
421 | + image_meta = result['image_meta'] |
422 | + response.status = httplib.CREATED |
423 | + response.headers.add('Content-Type', 'application/json') |
424 | + response.body = self.to_json(dict(image=image_meta)) |
425 | + self._inject_location_header(response, image_meta) |
426 | + self._inject_checksum_header(response, image_meta) |
427 | + return response |
428 | + |
429 | + |
430 | +def create_resource(options): |
431 | + """Images resource factory method""" |
432 | + deserializer = ImageDeserializer() |
433 | + serializer = ImageSerializer() |
434 | + return wsgi.Resource(Controller(options), deserializer, serializer) |
435 | |
436 | === modified file 'glance/common/exception.py' |
437 | --- glance/common/exception.py 2011-02-02 01:43:16 +0000 |
438 | +++ glance/common/exception.py 2011-06-17 20:46:38 +0000 |
439 | @@ -96,3 +96,29 @@ |
440 | raise |
441 | _wrap.func_name = f.func_name |
442 | return _wrap |
443 | + |
444 | + |
445 | +class GlanceException(Exception): |
446 | + """ |
447 | + Base Glance Exception |
448 | + |
449 | + To correctly use this class, inherit from it and define |
450 | + a 'message' property. That message will get printf'd |
451 | + with the keyword arguments provided to the constructor. |
452 | + """ |
453 | + message = "An unknown exception occurred" |
454 | + |
455 | + def __init__(self, **kwargs): |
456 | + try: |
457 | + self._error_string = self.message % kwargs |
458 | + |
459 | + except Exception: |
460 | + # at least get the core message out if something happened |
461 | + self._error_string = self.message |
462 | + |
463 | + def __str__(self): |
464 | + return self._error_string |
465 | + |
466 | + |
467 | +class InvalidContentType(GlanceException): |
468 | + message = "Invalid content type %(content_type)s" |
469 | |
470 | === modified file 'glance/common/wsgi.py' |
471 | --- glance/common/wsgi.py 2011-05-18 00:11:46 +0000 |
472 | +++ glance/common/wsgi.py 2011-06-17 20:46:38 +0000 |
473 | @@ -34,6 +34,8 @@ |
474 | import webob.dec |
475 | import webob.exc |
476 | |
477 | +from glance.common import exception |
478 | + |
479 | |
480 | class WritableLogger(object): |
481 | """A thin wrapper that responds to `write` and logs.""" |
482 | @@ -205,73 +207,55 @@ |
483 | return app |
484 | |
485 | |
486 | -class Controller(object): |
487 | - """ |
488 | - WSGI app that reads routing information supplied by RoutesMiddleware |
489 | - and calls the requested action method upon itself. All action methods |
490 | - must, in addition to their normal parameters, accept a 'req' argument |
491 | - which is the incoming webob.Request. They raise a webob.exc exception, |
492 | - or return a dict which will be serialized by requested content type. |
493 | - """ |
494 | - |
495 | - @webob.dec.wsgify |
496 | - def __call__(self, req): |
497 | - """ |
498 | - Call the method specified in req.environ by RoutesMiddleware. |
499 | - """ |
500 | - arg_dict = req.environ['wsgiorg.routing_args'][1] |
501 | - action = arg_dict['action'] |
502 | - method = getattr(self, action) |
503 | - del arg_dict['controller'] |
504 | - del arg_dict['action'] |
505 | - arg_dict['req'] = req |
506 | - result = method(**arg_dict) |
507 | - if type(result) is dict: |
508 | - return self._serialize(result, req) |
509 | - else: |
510 | - return result |
511 | - |
512 | - def _serialize(self, data, request): |
513 | - """ |
514 | - Serialize the given dict to the response type requested in request. |
515 | - Uses self._serialization_metadata if it exists, which is a dict mapping |
516 | - MIME types to information needed to serialize to that type. |
517 | - """ |
518 | - _metadata = getattr(type(self), "_serialization_metadata", {}) |
519 | - serializer = Serializer(request.environ, _metadata) |
520 | - return serializer.to_content_type(data) |
521 | - |
522 | - |
523 | -class Serializer(object): |
524 | - """ |
525 | - Serializes a dictionary to a Content Type specified by a WSGI environment. |
526 | - """ |
527 | - |
528 | - def __init__(self, environ, metadata=None): |
529 | - """ |
530 | - Create a serializer based on the given WSGI environment. |
531 | - 'metadata' is an optional dict mapping MIME types to information |
532 | - needed to serialize a dictionary to that type. |
533 | - """ |
534 | - self.environ = environ |
535 | - self.metadata = metadata or {} |
536 | - self._methods = { |
537 | - 'application/json': self._to_json, |
538 | - 'application/xml': self._to_xml} |
539 | - |
540 | - def to_content_type(self, data): |
541 | - """ |
542 | - Serialize a dictionary into a string. The format of the string |
543 | - will be decided based on the Content Type requested in self.environ: |
544 | - by Accept: header, or by URL suffix. |
545 | - """ |
546 | - # FIXME(sirp): for now, supporting json only |
547 | - #mimetype = 'application/xml' |
548 | - mimetype = 'application/json' |
549 | - # TODO(gundlach): determine mimetype from request |
550 | - return self._methods.get(mimetype, repr)(data) |
551 | - |
552 | - def _to_json(self, data): |
553 | +class Request(webob.Request): |
554 | + """Add some Openstack API-specific logic to the base webob.Request.""" |
555 | + |
556 | + def best_match_content_type(self): |
557 | + """Determine the requested response content-type.""" |
558 | + supported = ('application/json',) |
559 | + bm = self.accept.best_match(supported) |
560 | + return bm or 'application/json' |
561 | + |
562 | + def get_content_type(self, allowed_content_types): |
563 | + """Determine content type of the request body.""" |
564 | + if not "Content-Type" in self.headers: |
565 | + raise exception.InvalidContentType(content_type=None) |
566 | + |
567 | + content_type = self.content_type |
568 | + |
569 | + if content_type not in allowed_content_types: |
570 | + raise exception.InvalidContentType(content_type=content_type) |
571 | + else: |
572 | + return content_type |
573 | + |
574 | + |
575 | +class JSONRequestDeserializer(object): |
576 | + def has_body(self, request): |
577 | + """ |
578 | + Returns whether a Webob.Request object will possess an entity body. |
579 | + |
580 | + :param request: Webob.Request object |
581 | + """ |
582 | + if 'transfer-encoding' in request.headers: |
583 | + return True |
584 | + elif request.content_length > 0: |
585 | + return True |
586 | + |
587 | + return False |
588 | + |
589 | + def from_json(self, datastring): |
590 | + return json.loads(datastring) |
591 | + |
592 | + def default(self, request): |
593 | + if self.has_body(request): |
594 | + return {'body': self.from_json(request.body)} |
595 | + else: |
596 | + return {} |
597 | + |
598 | + |
599 | +class JSONResponseSerializer(object): |
600 | + |
601 | + def to_json(self, data): |
602 | def sanitizer(obj): |
603 | if isinstance(obj, datetime.datetime): |
604 | return obj.isoformat() |
605 | @@ -279,37 +263,85 @@ |
606 | |
607 | return json.dumps(data, default=sanitizer) |
608 | |
609 | - def _to_xml(self, data): |
610 | - metadata = self.metadata.get('application/xml', {}) |
611 | - # We expect data to contain a single key which is the XML root. |
612 | - root_key = data.keys()[0] |
613 | - from xml.dom import minidom |
614 | - doc = minidom.Document() |
615 | - node = self._to_xml_node(doc, metadata, root_key, data[root_key]) |
616 | - return node.toprettyxml(indent=' ') |
617 | - |
618 | - def _to_xml_node(self, doc, metadata, nodename, data): |
619 | - """Recursive method to convert data members to XML nodes.""" |
620 | - result = doc.createElement(nodename) |
621 | - if type(data) is list: |
622 | - singular = metadata.get('plurals', {}).get(nodename, None) |
623 | - if singular is None: |
624 | - if nodename.endswith('s'): |
625 | - singular = nodename[:-1] |
626 | - else: |
627 | - singular = 'item' |
628 | - for item in data: |
629 | - node = self._to_xml_node(doc, metadata, singular, item) |
630 | - result.appendChild(node) |
631 | - elif type(data) is dict: |
632 | - attrs = metadata.get('attributes', {}).get(nodename, {}) |
633 | - for k, v in data.items(): |
634 | - if k in attrs: |
635 | - result.setAttribute(k, str(v)) |
636 | - else: |
637 | - node = self._to_xml_node(doc, metadata, k, v) |
638 | - result.appendChild(node) |
639 | - else: # atom |
640 | - node = doc.createTextNode(str(data)) |
641 | - result.appendChild(node) |
642 | - return result |
643 | + def default(self, response, result): |
644 | + response.headers.add('Content-Type', 'application/json') |
645 | + response.body = self.to_json(result) |
646 | + |
647 | + |
648 | +class Resource(object): |
649 | + """ |
650 | + WSGI app that handles (de)serialization and controller dispatch. |
651 | + |
652 | + Reads routing information supplied by RoutesMiddleware and calls |
653 | + the requested action method upon its deserializer, controller, |
654 | + and serializer. Those three objects may implement any of the basic |
655 | + controller action methods (create, update, show, index, delete) |
656 | + along with any that may be specified in the api router. A 'default' |
657 | + method may also be implemented to be used in place of any |
658 | + non-implemented actions. Deserializer methods must accept a request |
659 | + argument and return a dictionary. Controller methods must accept a |
660 | + request argument. Additionally, they must also accept keyword |
661 | + arguments that represent the keys returned by the Deserializer. They |
662 | + may raise a webob.exc exception or return a dict, which will be |
663 | + serialized by requested content type. |
664 | + """ |
665 | + def __init__(self, controller, deserializer, serializer): |
666 | + """ |
667 | + :param controller: object that implement methods created by routes lib |
668 | + :param deserializer: object that supports webob request deserialization |
669 | + through controller-like actions |
670 | + :param serializer: object that supports webob response serialization |
671 | + through controller-like actions |
672 | + """ |
673 | + self.controller = controller |
674 | + self.serializer = serializer |
675 | + self.deserializer = deserializer |
676 | + |
677 | + @webob.dec.wsgify(RequestClass=Request) |
678 | + def __call__(self, request): |
679 | + """WSGI method that controls (de)serialization and method dispatch.""" |
680 | + action_args = self.get_action_args(request.environ) |
681 | + action = action_args.pop('action', None) |
682 | + |
683 | + deserialized_request = self.dispatch(self.deserializer, |
684 | + action, request) |
685 | + action_args.update(deserialized_request) |
686 | + |
687 | + action_result = self.dispatch(self.controller, action, |
688 | + request, **action_args) |
689 | + try: |
690 | + response = webob.Response() |
691 | + self.dispatch(self.serializer, action, response, action_result) |
692 | + return response |
693 | + |
694 | + # return unserializable result (typically a webob exc) |
695 | + except Exception: |
696 | + return action_result |
697 | + |
698 | + def dispatch(self, obj, action, *args, **kwargs): |
699 | + """Find action-specific method on self and call it.""" |
700 | + try: |
701 | + method = getattr(obj, action) |
702 | + except AttributeError: |
703 | + method = getattr(obj, 'default') |
704 | + |
705 | + return method(*args, **kwargs) |
706 | + |
707 | + def get_action_args(self, request_environment): |
708 | + """Parse dictionary created by routes library.""" |
709 | + try: |
710 | + args = request_environment['wsgiorg.routing_args'][1].copy() |
711 | + except Exception: |
712 | + return {} |
713 | + |
714 | + try: |
715 | + del args['controller'] |
716 | + except KeyError: |
717 | + pass |
718 | + |
719 | + try: |
720 | + del args['format'] |
721 | + except KeyError: |
722 | + pass |
723 | + |
724 | + return args |
725 | |
726 | === modified file 'glance/registry/client.py' |
727 | --- glance/registry/client.py 2011-05-27 19:48:51 +0000 |
728 | +++ glance/registry/client.py 2011-06-17 20:46:38 +0000 |
729 | @@ -94,10 +94,16 @@ |
730 | """ |
731 | Tells registry about an image's metadata |
732 | """ |
733 | + headers = { |
734 | + 'Content-Type': 'application/json', |
735 | + } |
736 | + |
737 | if 'image' not in image_metadata.keys(): |
738 | image_metadata = dict(image=image_metadata) |
739 | + |
740 | body = json.dumps(image_metadata) |
741 | - res = self.do_request("POST", "/images", body) |
742 | + |
743 | + res = self.do_request("POST", "/images", body, headers=headers) |
744 | # Registry returns a JSONified dict(image=image_info) |
745 | data = json.loads(res.read()) |
746 | return data['image'] |
747 | @@ -111,9 +117,13 @@ |
748 | |
749 | body = json.dumps(image_metadata) |
750 | |
751 | - headers = {} |
752 | + headers = { |
753 | + 'Content-Type': 'application/json', |
754 | + } |
755 | + |
756 | if purge_props: |
757 | headers["X-Glance-Registry-Purge-Props"] = "true" |
758 | + |
759 | res = self.do_request("PUT", "/images/%s" % image_id, body, headers) |
760 | data = json.loads(res.read()) |
761 | image = data['image'] |
762 | |
763 | === modified file 'glance/registry/server.py' |
764 | --- glance/registry/server.py 2011-05-18 00:19:44 +0000 |
765 | +++ glance/registry/server.py 2011-06-17 20:46:38 +0000 |
766 | @@ -42,7 +42,7 @@ |
767 | MAX_ITEM_LIMIT = 25 |
768 | |
769 | |
770 | -class Controller(wsgi.Controller): |
771 | +class Controller(object): |
772 | """Controller for the reference implementation registry server""" |
773 | |
774 | def __init__(self, options): |
775 | @@ -167,7 +167,7 @@ |
776 | """ |
777 | Deletes an existing image with the registry. |
778 | |
779 | - :param req: Request body. Ignored. |
780 | + :param req: wsgi Request object |
781 | :param id: The opaque internal identifier for the image |
782 | |
783 | :retval Returns 200 if delete was successful, a fault if not. |
784 | @@ -179,19 +179,19 @@ |
785 | except exception.NotFound: |
786 | return exc.HTTPNotFound() |
787 | |
788 | - def create(self, req): |
789 | + def create(self, req, body): |
790 | """ |
791 | Registers a new image with the registry. |
792 | |
793 | - :param req: Request body. A JSON-ified dict of information about |
794 | - the image. |
795 | + :param req: wsgi Request object |
796 | + :param body: Dictionary of information about the image |
797 | |
798 | :retval Returns the newly-created image information as a mapping, |
799 | which will include the newly-created image's internal id |
800 | in the 'id' field |
801 | |
802 | """ |
803 | - image_data = json.loads(req.body)['image'] |
804 | + image_data = body['image'] |
805 | |
806 | # Ensure the image has a status set |
807 | image_data.setdefault('status', 'active') |
808 | @@ -209,18 +209,17 @@ |
809 | logger.error(msg) |
810 | return exc.HTTPBadRequest(msg) |
811 | |
812 | - def update(self, req, id): |
813 | + def update(self, req, id, body): |
814 | """Updates an existing image with the registry. |
815 | |
816 | - :param req: Request body. A JSON-ified dict of information about |
817 | - the image. This will replace the information in the |
818 | - registry about this image |
819 | + :param req: wsgi Request object |
820 | + :param body: Dictionary of information about the image |
821 | :param id: The opaque internal identifier for the image |
822 | |
823 | :retval Returns the updated image information as a mapping, |
824 | |
825 | """ |
826 | - image_data = json.loads(req.body)['image'] |
827 | + image_data = body['image'] |
828 | |
829 | purge_props = req.headers.get("X-Glance-Registry-Purge-Props", "false") |
830 | context = None |
831 | @@ -244,15 +243,22 @@ |
832 | content_type='text/plain') |
833 | |
834 | |
835 | +def create_resource(options): |
836 | + """Images resource factory method.""" |
837 | + deserializer = wsgi.JSONRequestDeserializer() |
838 | + serializer = wsgi.JSONResponseSerializer() |
839 | + return wsgi.Resource(Controller(options), deserializer, serializer) |
840 | + |
841 | + |
842 | class API(wsgi.Router): |
843 | """WSGI entry point for all Registry requests.""" |
844 | |
845 | def __init__(self, options): |
846 | mapper = routes.Mapper() |
847 | - controller = Controller(options) |
848 | - mapper.resource("image", "images", controller=controller, |
849 | + resource = create_resource(options) |
850 | + mapper.resource("image", "images", controller=resource, |
851 | collection={'detail': 'GET'}) |
852 | - mapper.connect("/", controller=controller, action="index") |
853 | + mapper.connect("/", controller=resource, action="index") |
854 | super(API, self).__init__(mapper) |
855 | |
856 | |
857 | |
858 | === modified file 'glance/utils.py' |
859 | --- glance/utils.py 2011-04-12 21:45:16 +0000 |
860 | +++ glance/utils.py 2011-06-17 20:46:38 +0000 |
861 | @@ -43,24 +43,6 @@ |
862 | return headers |
863 | |
864 | |
865 | -def inject_image_meta_into_headers(response, image_meta): |
866 | - """ |
867 | - Given a response and mapping of image metadata, injects |
868 | - the Response with a set of HTTP headers for the image |
869 | - metadata. Each main image metadata field is injected |
870 | - as a HTTP header with key 'x-image-meta-<FIELD>' except |
871 | - for the properties field, which is further broken out |
872 | - into a set of 'x-image-meta-property-<KEY>' headers |
873 | - |
874 | - :param response: The Webob Response object |
875 | - :param image_meta: Mapping of image metadata |
876 | - """ |
877 | - headers = image_meta_to_http_headers(image_meta) |
878 | - |
879 | - for k, v in headers.items(): |
880 | - response.headers.add(k, v) |
881 | - |
882 | - |
883 | def get_image_meta_from_headers(response): |
884 | """ |
885 | Processes HTTP headers from a supplied response that |
886 | |
887 | === modified file 'tests/functional/test_curl_api.py' |
888 | --- tests/functional/test_curl_api.py 2011-05-27 19:48:51 +0000 |
889 | +++ tests/functional/test_curl_api.py 2011-06-17 20:46:38 +0000 |
890 | @@ -773,7 +773,7 @@ |
891 | with tempfile.NamedTemporaryFile() as test_data_file: |
892 | test_data_file.write("XXX") |
893 | test_data_file.flush() |
894 | - cmd = ("curl -i -X POST --upload-file %s " |
895 | + cmd = ("curl -i -X POST --upload-file %s -H 'Expect: ' " |
896 | "http://0.0.0.0:%d/v1/images") % (test_data_file.name, |
897 | api_port) |
898 | |
899 | |
900 | === modified file 'tests/unit/test_api.py' |
901 | --- tests/unit/test_api.py 2011-05-31 19:32:17 +0000 |
902 | +++ tests/unit/test_api.py 2011-06-17 20:46:38 +0000 |
903 | @@ -678,6 +678,7 @@ |
904 | req = webob.Request.blank('/images') |
905 | |
906 | req.method = 'POST' |
907 | + req.content_type = 'application/json' |
908 | req.body = json.dumps(dict(image=fixture)) |
909 | |
910 | res = req.get_response(self.api) |
911 | @@ -706,6 +707,7 @@ |
912 | req = webob.Request.blank('/images') |
913 | |
914 | req.method = 'POST' |
915 | + req.content_type = 'application/json' |
916 | req.body = json.dumps(dict(image=fixture)) |
917 | |
918 | res = req.get_response(self.api) |
919 | @@ -723,6 +725,7 @@ |
920 | req = webob.Request.blank('/images') |
921 | |
922 | req.method = 'POST' |
923 | + req.content_type = 'application/json' |
924 | req.body = json.dumps(dict(image=fixture)) |
925 | |
926 | res = req.get_response(self.api) |
927 | @@ -739,6 +742,7 @@ |
928 | req = webob.Request.blank('/images') |
929 | |
930 | req.method = 'POST' |
931 | + req.content_type = 'application/json' |
932 | req.body = json.dumps(dict(image=fixture)) |
933 | |
934 | res = req.get_response(self.api) |
935 | @@ -758,6 +762,7 @@ |
936 | req = webob.Request.blank('/images') |
937 | |
938 | req.method = 'POST' |
939 | + req.content_type = 'application/json' |
940 | req.body = json.dumps(dict(image=fixture)) |
941 | |
942 | res = req.get_response(self.api) |
943 | @@ -772,6 +777,7 @@ |
944 | req = webob.Request.blank('/images/2') |
945 | |
946 | req.method = 'PUT' |
947 | + req.content_type = 'application/json' |
948 | req.body = json.dumps(dict(image=fixture)) |
949 | |
950 | res = req.get_response(self.api) |
951 | @@ -791,6 +797,7 @@ |
952 | req = webob.Request.blank('/images/3') |
953 | |
954 | req.method = 'PUT' |
955 | + req.content_type = 'application/json' |
956 | req.body = json.dumps(dict(image=fixture)) |
957 | |
958 | res = req.get_response(self.api) |
959 | @@ -804,6 +811,7 @@ |
960 | req = webob.Request.blank('/images/2') |
961 | |
962 | req.method = 'PUT' |
963 | + req.content_type = 'application/json' |
964 | req.body = json.dumps(dict(image=fixture)) |
965 | |
966 | res = req.get_response(self.api) |
967 | @@ -817,6 +825,7 @@ |
968 | req = webob.Request.blank('/images/2') |
969 | |
970 | req.method = 'PUT' |
971 | + req.content_type = 'application/json' |
972 | req.body = json.dumps(dict(image=fixture)) |
973 | |
974 | res = req.get_response(self.api) |
975 | @@ -830,6 +839,7 @@ |
976 | req = webob.Request.blank('/images/2') |
977 | |
978 | req.method = 'PUT' |
979 | + req.content_type = 'application/json' |
980 | req.body = json.dumps(dict(image=fixture)) |
981 | |
982 | res = req.get_response(self.api) |
983 | @@ -844,6 +854,7 @@ |
984 | req = webob.Request.blank('/images/2') # Image 2 has disk format 'vhd' |
985 | |
986 | req.method = 'PUT' |
987 | + req.content_type = 'application/json' |
988 | req.body = json.dumps(dict(image=fixture)) |
989 | |
990 | res = req.get_response(self.api) |
991 | @@ -972,6 +983,21 @@ |
992 | res_body = json.loads(res.body)['image'] |
993 | self.assertEquals('queued', res_body['status']) |
994 | |
995 | + def test_add_image_no_location_no_content_type(self): |
996 | + """Tests creates a queued image for no body and no loc header""" |
997 | + fixture_headers = {'x-image-meta-store': 'file', |
998 | + 'x-image-meta-disk-format': 'vhd', |
999 | + 'x-image-meta-container-format': 'ovf', |
1000 | + 'x-image-meta-name': 'fake image #3'} |
1001 | + |
1002 | + req = webob.Request.blank("/images") |
1003 | + req.body = "chunk00000remainder" |
1004 | + req.method = 'POST' |
1005 | + for k, v in fixture_headers.iteritems(): |
1006 | + req.headers[k] = v |
1007 | + res = req.get_response(self.api) |
1008 | + self.assertEquals(res.status_int, 400) |
1009 | + |
1010 | def test_add_image_bad_store(self): |
1011 | """Tests raises BadRequest for invalid store header""" |
1012 | fixture_headers = {'x-image-meta-store': 'bad', |
1013 | |
1014 | === added file 'tests/unit/test_wsgi.py' |
1015 | --- tests/unit/test_wsgi.py 1970-01-01 00:00:00 +0000 |
1016 | +++ tests/unit/test_wsgi.py 2011-06-17 20:46:38 +0000 |
1017 | @@ -0,0 +1,182 @@ |
1018 | +# vim: tabstop=4 shiftwidth=4 softtabstop=4 |
1019 | + |
1020 | +# Copyright 2010-2011 OpenStack, LLC |
1021 | +# All Rights Reserved. |
1022 | +# |
1023 | +# Licensed under the Apache License, Version 2.0 (the "License"); you may |
1024 | +# not use this file except in compliance with the License. You may obtain |
1025 | +# a copy of the License at |
1026 | +# |
1027 | +# http://www.apache.org/licenses/LICENSE-2.0 |
1028 | +# |
1029 | +# Unless required by applicable law or agreed to in writing, software |
1030 | +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT |
1031 | +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the |
1032 | +# License for the specific language governing permissions and limitations |
1033 | +# under the License. |
1034 | + |
1035 | +import unittest |
1036 | +import webob |
1037 | + |
1038 | +from glance.common import wsgi |
1039 | +from glance.common import exception |
1040 | + |
1041 | + |
1042 | +class RequestTest(unittest.TestCase): |
1043 | + def test_content_type_missing(self): |
1044 | + request = wsgi.Request.blank('/tests/123') |
1045 | + request.body = "<body />" |
1046 | + self.assertRaises(exception.InvalidContentType, |
1047 | + request.get_content_type, ('application/xml')) |
1048 | + |
1049 | + def test_content_type_unsupported(self): |
1050 | + request = wsgi.Request.blank('/tests/123') |
1051 | + request.headers["Content-Type"] = "text/html" |
1052 | + request.body = "asdf<br />" |
1053 | + self.assertRaises(exception.InvalidContentType, |
1054 | + request.get_content_type, ('application/xml')) |
1055 | + |
1056 | + def test_content_type_with_charset(self): |
1057 | + request = wsgi.Request.blank('/tests/123') |
1058 | + request.headers["Content-Type"] = "application/json; charset=UTF-8" |
1059 | + result = request.get_content_type(('application/json')) |
1060 | + self.assertEqual(result, "application/json") |
1061 | + |
1062 | + def test_content_type_from_accept_xml(self): |
1063 | + request = wsgi.Request.blank('/tests/123') |
1064 | + request.headers["Accept"] = "application/xml" |
1065 | + result = request.best_match_content_type() |
1066 | + self.assertEqual(result, "application/json") |
1067 | + |
1068 | + def test_content_type_from_accept_json(self): |
1069 | + request = wsgi.Request.blank('/tests/123') |
1070 | + request.headers["Accept"] = "application/json" |
1071 | + result = request.best_match_content_type() |
1072 | + self.assertEqual(result, "application/json") |
1073 | + |
1074 | + def test_content_type_from_accept_xml_json(self): |
1075 | + request = wsgi.Request.blank('/tests/123') |
1076 | + request.headers["Accept"] = "application/xml, application/json" |
1077 | + result = request.best_match_content_type() |
1078 | + self.assertEqual(result, "application/json") |
1079 | + |
1080 | + def test_content_type_from_accept_json_xml_quality(self): |
1081 | + request = wsgi.Request.blank('/tests/123') |
1082 | + request.headers["Accept"] = \ |
1083 | + "application/json; q=0.3, application/xml; q=0.9" |
1084 | + result = request.best_match_content_type() |
1085 | + self.assertEqual(result, "application/json") |
1086 | + |
1087 | + def test_content_type_accept_default(self): |
1088 | + request = wsgi.Request.blank('/tests/123.unsupported') |
1089 | + request.headers["Accept"] = "application/unsupported1" |
1090 | + result = request.best_match_content_type() |
1091 | + self.assertEqual(result, "application/json") |
1092 | + |
1093 | + |
1094 | +class ResourceTest(unittest.TestCase): |
1095 | + def test_get_action_args(self): |
1096 | + env = { |
1097 | + 'wsgiorg.routing_args': [ |
1098 | + None, |
1099 | + { |
1100 | + 'controller': None, |
1101 | + 'format': None, |
1102 | + 'action': 'update', |
1103 | + 'id': 12, |
1104 | + }, |
1105 | + ], |
1106 | + } |
1107 | + |
1108 | + expected = {'action': 'update', 'id': 12} |
1109 | + actual = wsgi.Resource(None, None, None).get_action_args(env) |
1110 | + |
1111 | + self.assertEqual(actual, expected) |
1112 | + |
1113 | + def test_dispatch(self): |
1114 | + class Controller(object): |
1115 | + def index(self, shirt, pants=None): |
1116 | + return (shirt, pants) |
1117 | + |
1118 | + resource = wsgi.Resource(None, None, None) |
1119 | + actual = resource.dispatch(Controller(), 'index', 'on', pants='off') |
1120 | + expected = ('on', 'off') |
1121 | + self.assertEqual(actual, expected) |
1122 | + |
1123 | + def test_dispatch_default(self): |
1124 | + class Controller(object): |
1125 | + def default(self, shirt, pants=None): |
1126 | + return (shirt, pants) |
1127 | + |
1128 | + resource = wsgi.Resource(None, None, None) |
1129 | + actual = resource.dispatch(Controller(), 'index', 'on', pants='off') |
1130 | + expected = ('on', 'off') |
1131 | + self.assertEqual(actual, expected) |
1132 | + |
1133 | + def test_dispatch_no_default(self): |
1134 | + class Controller(object): |
1135 | + def show(self, shirt, pants=None): |
1136 | + return (shirt, pants) |
1137 | + |
1138 | + resource = wsgi.Resource(None, None, None) |
1139 | + self.assertRaises(AttributeError, resource.dispatch, Controller(), |
1140 | + 'index', 'on', pants='off') |
1141 | + |
1142 | + |
1143 | +class JSONResponseSerializerTest(unittest.TestCase): |
1144 | + def test_to_json(self): |
1145 | + fixture = {"key": "value"} |
1146 | + expected = '{"key": "value"}' |
1147 | + actual = wsgi.JSONResponseSerializer().to_json(fixture) |
1148 | + self.assertEqual(actual, expected) |
1149 | + |
1150 | + def test_default(self): |
1151 | + fixture = {"key": "value"} |
1152 | + response = webob.Response() |
1153 | + wsgi.JSONResponseSerializer().default(response, fixture) |
1154 | + self.assertEqual(response.status_int, 200) |
1155 | + self.assertEqual(response.content_type, 'application/json') |
1156 | + self.assertEqual(response.body, '{"key": "value"}') |
1157 | + |
1158 | + |
1159 | +class JSONRequestDeserializerTest(unittest.TestCase): |
1160 | + def test_has_body_no_content_length(self): |
1161 | + request = wsgi.Request.blank('/') |
1162 | + request.body = 'asdf' |
1163 | + request.headers.pop('Content-Length') |
1164 | + self.assertFalse(wsgi.JSONRequestDeserializer().has_body(request)) |
1165 | + |
1166 | + def test_has_body_zero_content_length(self): |
1167 | + request = wsgi.Request.blank('/') |
1168 | + request.body = 'asdf' |
1169 | + request.headers['Content-Length'] = 0 |
1170 | + self.assertFalse(wsgi.JSONRequestDeserializer().has_body(request)) |
1171 | + |
1172 | + def test_has_body_has_content_length(self): |
1173 | + request = wsgi.Request.blank('/') |
1174 | + request.body = 'asdf' |
1175 | + self.assertTrue('Content-Length' in request.headers) |
1176 | + self.assertTrue(wsgi.JSONRequestDeserializer().has_body(request)) |
1177 | + |
1178 | + def test_no_body_no_content_length(self): |
1179 | + request = wsgi.Request.blank('/') |
1180 | + self.assertFalse(wsgi.JSONRequestDeserializer().has_body(request)) |
1181 | + |
1182 | + def test_from_json(self): |
1183 | + fixture = '{"key": "value"}' |
1184 | + expected = {"key": "value"} |
1185 | + actual = wsgi.JSONRequestDeserializer().from_json(fixture) |
1186 | + self.assertEqual(actual, expected) |
1187 | + |
1188 | + def test_default_no_body(self): |
1189 | + request = wsgi.Request.blank('/') |
1190 | + actual = wsgi.JSONRequestDeserializer().default(request) |
1191 | + expected = {} |
1192 | + self.assertEqual(actual, expected) |
1193 | + |
1194 | + def test_default_with_body(self): |
1195 | + request = wsgi.Request.blank('/') |
1196 | + request.body = '{"key": "value"}' |
1197 | + actual = wsgi.JSONRequestDeserializer().default(request) |
1198 | + expected = {"body": {"key": "value"}} |
1199 | + self.assertEqual(actual, expected) |
I can't figure out why tests/functiona l/test_ curl_api. py:TestCurlApi. test_traceback_ not_consumed is failing. It is "turned off" in my merge prop but I would like to fix it before I send it in.