OpenStack Object Storage (swift)

Merge lp:~notmyname/swift/copy_content_type_fix into lp:~hudson-openstack/swift/trunk

  1. copy_content_type_fix
  2. Merge into trunk
Proposed by John Dickinson
Status: Merged
Approved by: gholt
Approved revision: 159
Merged at revision: 159
Proposed branch: lp:~notmyname/swift/copy_content_type_fix
Merge into: lp:~hudson-openstack/swift/trunk
Diff against target: 156 lines (+87/-0)
2 files modified
swift/proxy/server.py (+5/-0)
test/unit/proxy/test_server.py (+82/-0)
To merge this branch: bzr merge lp:~notmyname/swift/copy_content_type_fix
Reviewer Review Type Date Requested Status
gholt (community) Approve
Review via email: mp+45307@code.launchpad.net

Description of the change

server-side object copy now also copies the content-type of the source object if the content type is not given in the copy request

To post a comment you must log in.
lp:~notmyname/swift/copy_content_type_fix updated
156. By John Dickinson

changed the order of setting the charset and content_type to ensure that charset isn't added to the object

Revision history for this message
gholt (gholt) wrote :

The tests pass whether the fixes are in place or not. Shouldn't there be some new tests or something? Also, the logic with content_type_manually_set just seems a bit hard to read and seems incorrect anyhow. If we're never going to use the guessed content-type, why compute it?

review: Needs Fixing
Revision history for this message
gholt (gholt) wrote :

Oh nm on the weird logic with content_type_manually_set, I see now. Stupid me not remembering that is the PUT function that doubles as COPY. The logic still seems a little off, content_type_manually_set = not not req.headers.get('content-type') and then later we check if not content_type_manually_set:

The tests still need work though. :)

review: Needs Fixing
Revision history for this message
John Dickinson (notmyname) wrote :

after discovering test_chunked_put_and_a_bit_more, tests have been added

lp:~notmyname/swift/copy_content_type_fix updated
157. By John Dickinson

I did not know the wonders of test_chunked_put_and_a_bit_more

158. By John Dickinson

fixed and tested charset on content types

159. By John Dickinson

merged with trunk

Revision history for this message
gholt (gholt) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'swift/proxy/server.py'
2--- swift/proxy/server.py 2011-01-11 03:57:26 +0000
3+++ swift/proxy/server.py 2011-01-11 06:15:10 +0000
4@@ -911,12 +911,14 @@
5 self.account_name, self.container_name, self.object_name)
6 req.headers['X-Timestamp'] = normalize_timestamp(time.time())
7 # Sometimes the 'content-type' header exists, but is set to None.
8+ content_type_manually_set = True
9 if not req.headers.get('content-type'):
10 guessed_type, _junk = mimetypes.guess_type(req.path_info)
11 if not guessed_type:
12 req.headers['Content-Type'] = 'application/octet-stream'
13 else:
14 req.headers['Content-Type'] = guessed_type
15+ content_type_manually_set = False
16 error_response = check_object_creation(req, self.object_name)
17 if error_response:
18 return error_response
19@@ -961,6 +963,9 @@
20 new_req.etag = source_resp.etag
21 # we no longer need the X-Copy-From header
22 del new_req.headers['X-Copy-From']
23+ if not content_type_manually_set:
24+ new_req.headers['Content-Type'] = \
25+ source_resp.headers['Content-Type']
26 for k, v in source_resp.headers.items():
27 if k.lower().startswith('x-object-meta-'):
28 new_req.headers[k] = v
29
30=== modified file 'test/unit/proxy/test_server.py'
31--- test/unit/proxy/test_server.py 2011-01-11 03:57:26 +0000
32+++ test/unit/proxy/test_server.py 2011-01-11 06:15:10 +0000
33@@ -1264,6 +1264,7 @@
34 with save_globals():
35 controller = proxy_server.ObjectController(self.app, 'account',
36 'container', 'object')
37+ # initial source object PUT
38 req = Request.blank('/a/c/o', environ={'REQUEST_METHOD': 'PUT'},
39 headers={'Content-Length': '0'})
40 self.app.update_request(req)
41@@ -1273,6 +1274,7 @@
42 resp = controller.PUT(req)
43 self.assertEquals(resp.status_int, 201)
44
45+ # basic copy
46 req = Request.blank('/a/c/o', environ={'REQUEST_METHOD': 'PUT'},
47 headers={'Content-Length': '0',
48 'X-Copy-From': 'c/o'})
49@@ -1285,6 +1287,7 @@
50 self.assertEquals(resp.status_int, 201)
51 self.assertEquals(resp.headers['x-copied-from'], 'c/o')
52
53+ # non-zero content length
54 req = Request.blank('/a/c/o', environ={'REQUEST_METHOD': 'PUT'},
55 headers={'Content-Length': '5',
56 'X-Copy-From': 'c/o'})
57@@ -1296,6 +1299,7 @@
58 resp = controller.PUT(req)
59 self.assertEquals(resp.status_int, 400)
60
61+ # extra source path parsing
62 req = Request.blank('/a/c/o', environ={'REQUEST_METHOD': 'PUT'},
63 headers={'Content-Length': '0',
64 'X-Copy-From': 'c/o/o2'})
65@@ -1308,6 +1312,7 @@
66 self.assertEquals(resp.status_int, 201)
67 self.assertEquals(resp.headers['x-copied-from'], 'c/o/o2')
68
69+ # space in soure path
70 req = Request.blank('/a/c/o', environ={'REQUEST_METHOD': 'PUT'},
71 headers={'Content-Length': '0',
72 'X-Copy-From': 'c/o%20o2'})
73@@ -2037,6 +2042,83 @@
74 self.assert_('Content-Length: 25\r' in headers)
75 body = fd.read()
76 self.assertEquals(body, '1234 1234 1234 1234 1234 ')
77+ # Check copy content type
78+ sock = connect_tcp(('localhost', prolis.getsockname()[1]))
79+ fd = sock.makefile()
80+ fd.write('PUT /v1/a/c/obj HTTP/1.1\r\nHost: '
81+ 'localhost\r\nConnection: close\r\nX-Storage-Token: '
82+ 't\r\nContent-Length: 0\r\nContent-Type: text/jibberish'
83+ '\r\n\r\n')
84+ fd.flush()
85+ headers = readuntil2crlfs(fd)
86+ exp = 'HTTP/1.1 201'
87+ self.assertEquals(headers[:len(exp)], exp)
88+ sock = connect_tcp(('localhost', prolis.getsockname()[1]))
89+ fd = sock.makefile()
90+ fd.write('PUT /v1/a/c/obj2 HTTP/1.1\r\nHost: '
91+ 'localhost\r\nConnection: close\r\nX-Storage-Token: '
92+ 't\r\nContent-Length: 0\r\nX-Copy-From: c/obj\r\n\r\n')
93+ fd.flush()
94+ headers = readuntil2crlfs(fd)
95+ exp = 'HTTP/1.1 201'
96+ self.assertEquals(headers[:len(exp)], exp)
97+ # Ensure getting the copied file gets original content-type
98+ sock = connect_tcp(('localhost', prolis.getsockname()[1]))
99+ fd = sock.makefile()
100+ fd.write('GET /v1/a/c/obj2 HTTP/1.1\r\nHost: '
101+ 'localhost\r\nConnection: close\r\nX-Auth-Token: '
102+ 't\r\n\r\n')
103+ fd.flush()
104+ headers = readuntil2crlfs(fd)
105+ exp = 'HTTP/1.1 200'
106+ self.assertEquals(headers[:len(exp)], exp)
107+ self.assert_('Content-Type: text/jibberish' in headers)
108+ # Check set content type
109+ sock = connect_tcp(('localhost', prolis.getsockname()[1]))
110+ fd = sock.makefile()
111+ fd.write('PUT /v1/a/c/obj3 HTTP/1.1\r\nHost: '
112+ 'localhost\r\nConnection: close\r\nX-Storage-Token: '
113+ 't\r\nContent-Length: 0\r\nContent-Type: foo/bar'
114+ '\r\n\r\n')
115+ fd.flush()
116+ headers = readuntil2crlfs(fd)
117+ exp = 'HTTP/1.1 201'
118+ self.assertEquals(headers[:len(exp)], exp)
119+ # Ensure getting the copied file gets original content-type
120+ sock = connect_tcp(('localhost', prolis.getsockname()[1]))
121+ fd = sock.makefile()
122+ fd.write('GET /v1/a/c/obj3 HTTP/1.1\r\nHost: '
123+ 'localhost\r\nConnection: close\r\nX-Auth-Token: '
124+ 't\r\n\r\n')
125+ fd.flush()
126+ headers = readuntil2crlfs(fd)
127+ exp = 'HTTP/1.1 200'
128+ self.assertEquals(headers[:len(exp)], exp)
129+ self.assert_('Content-Type: foo/bar' in
130+ headers.split('\r\n'), repr(headers.split('\r\n')))
131+ # Check set content type with charset
132+ sock = connect_tcp(('localhost', prolis.getsockname()[1]))
133+ fd = sock.makefile()
134+ fd.write('PUT /v1/a/c/obj4 HTTP/1.1\r\nHost: '
135+ 'localhost\r\nConnection: close\r\nX-Storage-Token: '
136+ 't\r\nContent-Length: 0\r\nContent-Type: foo/bar'
137+ '; charset=UTF-8\r\n\r\n')
138+ fd.flush()
139+ headers = readuntil2crlfs(fd)
140+ exp = 'HTTP/1.1 201'
141+ self.assertEquals(headers[:len(exp)], exp)
142+ # Ensure getting the copied file gets original content-type
143+ sock = connect_tcp(('localhost', prolis.getsockname()[1]))
144+ fd = sock.makefile()
145+ fd.write('GET /v1/a/c/obj4 HTTP/1.1\r\nHost: '
146+ 'localhost\r\nConnection: close\r\nX-Auth-Token: '
147+ 't\r\n\r\n')
148+ fd.flush()
149+ headers = readuntil2crlfs(fd)
150+ exp = 'HTTP/1.1 200'
151+ self.assertEquals(headers[:len(exp)], exp)
152+ self.assert_('Content-Type: foo/bar; charset=UTF-8' in
153+ headers.split('\r\n'), repr(headers.split('\r\n')))
154 finally:
155 prospa.kill()
156 acc1spa.kill()