Merge lp:~leonardr/lazr.restful/multiversion-mutators into lp:lazr.restful

Proposed by Leonard Richardson
Status: Merged
Merged at revision: not available
Proposed branch: lp:~leonardr/lazr.restful/multiversion-mutators
Merge into: lp:lazr.restful
Diff against target: 303 lines (+192/-20)
2 files modified
src/lazr/restful/declarations.py (+70/-11)
src/lazr/restful/docs/webservice-declarations.txt (+122/-9)
To merge this branch: bzr merge lp:~leonardr/lazr.restful/multiversion-mutators
Reviewer Review Type Date Requested Status
Paul Hummer (community) code Approve
Review via email: mp+18702@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Leonard Richardson (leonardr) wrote :

This diff makes mutators version-aware. It lets you define different mutators for different versions, and it prevents you from defining more than one mutator for any particular version.

The complication here is that the @mutator_for annotation is attached to the mutator method, but it modifies a field. Before multi-version, @mutator_for just grabbed the field's annotation dictionary and modified it. But now, "modifying the field's annotation dictionary" means modifying the way the field is published *in the most recent version*. We need to modify the way the field is published in a *specific* version. But we have no guarantee that the field already has an existing set of annotations for that version, and if it's not in the stack already we have no idea whether that version comes before or after other versions.

Here's an example. (I don't have a test like this because it doesn't add any code coverage, and I don't think it's necessary to write a test to explain the internal implementation of something, but it's the best way to explain the problem.) Let's say I publish an entry like this:

    field = exported(TextLine(), ('foo', dict(exported_as='foofield'),
                                 ('bar', dict(exported_as='barfield'))

    @mutator_for(field)
    @export_write_operation()
    @operation_for_version('baz')
    def mutator(value):
        ...

When I process the 'mutator' method I need to modify the annotations on the 'field' field. That field's LAZR_WEBSERVICE_EXPORTED annotation stack looks the same as it did in the exported() call:

[('foo', dict(exported_as='foofield'),
 ('bar', dict(exported_as='barfield')]

Where does 'baz' go? There is *no way* to know. The decision has to be deferred until generate_entry_interfaces, when we have an ordered list of the versions. So this 'baz' thing can't go into LAZR_WEBSERVICE_EXPORTED at all. That's why I created a separate annotation dictionary just for mutators, LAZR_WEBSERVICE_MUTATORS. This is a regular dictionary (not a VersionedDict) that maps versions to (mutator, mutator_annotations) 2-tuples. When we encounter @mutator_for, the stuff that was going to go into LAZR_WEBSERVICE_EXPORTED gets put into here instead. Later on, in generate_entry_interfaces and generate_entry_adapters, when we need to do per-version error checking or generate a per-version Property, we pull the version-indexed value out of LAZR_WEBSERVICE_MUTATORS.

Revision history for this message
Paul Hummer (rockstar) wrote :

Leonard, thanks for making the pagetests so detailed. It makes reviewing easier, since I hop back and forth between test and code when I have questions. I'm also glad to see the tests that were XXX missing from a previous branch are in this one.

review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'src/lazr/restful/declarations.py'
--- src/lazr/restful/declarations.py 2010-02-04 22:23:54 +0000
+++ src/lazr/restful/declarations.py 2010-02-05 15:08:15 +0000
@@ -8,6 +8,7 @@
8 'ENTRY_TYPE',8 'ENTRY_TYPE',
9 'FIELD_TYPE',9 'FIELD_TYPE',
10 'LAZR_WEBSERVICE_EXPORTED',10 'LAZR_WEBSERVICE_EXPORTED',
11 'LAZR_WEBSERVICE_MUTATORS',
11 'OPERATION_TYPES',12 'OPERATION_TYPES',
12 'REQUEST_USER',13 'REQUEST_USER',
13 'cache_for',14 'cache_for',
@@ -63,6 +64,7 @@
63 make_identifier_safe, VersionedDict)64 make_identifier_safe, VersionedDict)
6465
65LAZR_WEBSERVICE_EXPORTED = '%s.exported' % LAZR_WEBSERVICE_NS66LAZR_WEBSERVICE_EXPORTED = '%s.exported' % LAZR_WEBSERVICE_NS
67LAZR_WEBSERVICE_MUTATORS = '%s.exported.mutators' % LAZR_WEBSERVICE_NS
66COLLECTION_TYPE = 'collection'68COLLECTION_TYPE = 'collection'
67ENTRY_TYPE = 'entry'69ENTRY_TYPE = 'entry'
68FIELD_TYPE = 'field'70FIELD_TYPE = 'field'
@@ -220,6 +222,13 @@
220222
221 # Now we can annotate the field object with the VersionedDict.223 # Now we can annotate the field object with the VersionedDict.
222 field.setTaggedValue(LAZR_WEBSERVICE_EXPORTED, annotation_stack)224 field.setTaggedValue(LAZR_WEBSERVICE_EXPORTED, annotation_stack)
225
226 # We track the field's mutator information separately because it's
227 # defined in the named operations, not in the fields. The last
228 # thing we want to do is try to insert a foreign value into an
229 # already create annotation stack.
230 field.setTaggedValue(LAZR_WEBSERVICE_MUTATORS, {})
231
223 return field232 return field
224233
225234
@@ -531,13 +540,19 @@
531 "non-fixed argument. %s takes %d." %540 "non-fixed argument. %s takes %d." %
532 (method.__name__, len(free_params)))541 (method.__name__, len(free_params)))
533542
534 field_annotations = self.field.queryTaggedValue(543 # We need to keep mutator annotations in a separate dictionary
535 LAZR_WEBSERVICE_EXPORTED)544 # from the field's main annotations because we're not
536 if 'mutated_by' in field_annotations:545 # processing the field. We're processing a named operation,
537 raise TypeError("A field can only have one mutator method; "546 # and we have no idea where the named operation's current
538 "%s makes two." % method.__name__ )547 # version fits into the field's annotations.
539 field_annotations['mutated_by'] = method548 version, method_annotations = annotations.stack[-1]
540 field_annotations['mutated_by_annotations'] = annotations549 mutator_annotations = self.field.queryTaggedValue(
550 LAZR_WEBSERVICE_MUTATORS)
551 if version in mutator_annotations:
552 raise TypeError(
553 "A field can only have one mutator method for version %s; "
554 "%s makes two." % (_version_name(version), method.__name__ ))
555 mutator_annotations[version] = (method, dict(method_annotations))
541556
542557
543def _free_parameters(method, annotations):558def _free_parameters(method, annotations):
@@ -879,8 +894,15 @@
879 tags = tags[0]894 tags = tags[0]
880 if tags.get('exported') is False:895 if tags.get('exported') is False:
881 continue896 continue
882 readonly = (field.readonly897 mutator_annotations = field.queryTaggedValue(
883 and tags.get('mutated_by', None) is None)898 LAZR_WEBSERVICE_MUTATORS)
899 error_prefix = (
900 'Duplicate mutator for interface "%s", field "%s":' % (
901 interface.__name__, field.__name__))
902 mutated_by, mutated_by_annotations = _get_by_version(
903 mutator_annotations, version, versions[0],
904 error_prefix, (None, {}))
905 readonly = (field.readonly and mutated_by is None)
884 attrs[tags['as']] = copy_field(906 attrs[tags['as']] = copy_field(
885 field, __name__=tags['as'], readonly=readonly)907 field, __name__=tags['as'], readonly=readonly)
886 class_name = _versioned_class_name(908 class_name = _versioned_class_name(
@@ -919,6 +941,7 @@
919 adapters_by_version = {}941 adapters_by_version = {}
920 for name, field in getFields(content_interface).items():942 for name, field in getFields(content_interface).items():
921 tag_stack = field.queryTaggedValue(LAZR_WEBSERVICE_EXPORTED)943 tag_stack = field.queryTaggedValue(LAZR_WEBSERVICE_EXPORTED)
944 mutator_annotations = field.queryTaggedValue(LAZR_WEBSERVICE_MUTATORS)
922 if tag_stack is None:945 if tag_stack is None:
923 continue946 continue
924 for tags_version, tags in tag_stack.stack:947 for tags_version, tags in tag_stack.stack:
@@ -931,11 +954,16 @@
931 # this version's adapter class dictionary.954 # this version's adapter class dictionary.
932 if tags.get('exported') is False:955 if tags.get('exported') is False:
933 continue956 continue
934 mutator = tags.get('mutated_by', None)957 error_prefix = (
958 'Duplicate mutator for interface "%s", field "%s":' % (
959 content_interface.__name__, field.__name__))
960 mutator, annotations = _get_by_version(
961 mutator_annotations, tags_version, webservice_interfaces[0][0],
962 error_prefix, (None, {}))
963
935 if mutator is None:964 if mutator is None:
936 property = Passthrough(name, 'context')965 property = Passthrough(name, 'context')
937 else:966 else:
938 annotations = tags['mutated_by_annotations']
939 property = PropertyWithMutator(967 property = PropertyWithMutator(
940 name, 'context', mutator, annotations)968 name, 'context', mutator, annotations)
941 adapter_dict[tags['as']] = property969 adapter_dict[tags['as']] = property
@@ -1291,3 +1319,34 @@
1291 version = "__Earliest"1319 version = "__Earliest"
1292 name = "%s_%s" % (base_name, version.encode('utf8'))1320 name = "%s_%s" % (base_name, version.encode('utf8'))
1293 return make_identifier_safe(name)1321 return make_identifier_safe(name)
1322
1323
1324def _get_by_version(dictionary, version, earliest_version,
1325 error_prefix, default=None):
1326 """Pull from a dictionary using `version` as the key.
1327
1328 Under normal circumstances, this will be an ordinary lookup. But
1329 some values for the earliest version are stored under None
1330 (because the name of the earliest version wasn't known at the
1331 time). This helper function knows to look again under None if
1332 a lookup for the earliest version fails.
1333
1334 It also knows to raise an exception signaling a conflict if a
1335 value is present under the earliest version *and* under None.
1336
1337 :param dictionary: The dictionary to use for the lookup.
1338 :param version: The dictionary key.
1339 :param earliest_version: The version that might also be filed under None.
1340 :param default: A default value to use if the lookup fails.
1341 """
1342 value = dictionary.get(version, None)
1343 if version == earliest_version:
1344 earliest_value = dictionary.get(None, None)
1345 if value is not None and earliest_value is not None:
1346 raise ValueError(
1347 error_prefix + " Both implicit and explicit definitions found "
1348 "for earliest version %s." % version)
1349 value = value or earliest_value
1350 if value is None:
1351 value = default
1352 return value
12941353
=== modified file 'src/lazr/restful/docs/webservice-declarations.txt'
--- src/lazr/restful/docs/webservice-declarations.txt 2010-02-04 21:22:50 +0000
+++ src/lazr/restful/docs/webservice-declarations.txt 2010-02-05 15:08:15 +0000
@@ -1465,8 +1465,8 @@
1465 ... pass1465 ... pass
1466 Traceback (most recent call last):1466 Traceback (most recent call last):
1467 ...1467 ...
1468 TypeError: A field can only have one mutator method; set_value_21468 TypeError: A field can only have one mutator method for version
1469 makes two.1469 (earliest version); set_value_2 makes two.
14701470
1471Caching1471Caching
1472=======1472=======
@@ -1622,6 +1622,11 @@
1622Entries1622Entries
1623-------1623-------
16241624
1625The singular and plural name of an entry never changes between
1626versions, because the names are a property of the original
1627interface. But the published fields can change or be renamed from
1628version to version.
1629
1625Here's a data model interface defining four fields which are published1630Here's a data model interface defining four fields which are published
1626in some versions and not others, and which may have different names in1631in some versions and not others, and which may have different names in
1627different versions.1632different versions.
@@ -1803,11 +1808,9 @@
1803 AttributeError: 'MultiVersionEntryEntry_3_0Adapter' object has no1808 AttributeError: 'MultiVersionEntryEntry_3_0Adapter' object has no
1804 attribute 'new_in_10'1809 attribute 'new_in_10'
18051810
18061811[XXX leonardr These tests need to be done in pagetests, since they
1807[XXX leonardr I'll add these tests in my next branch; this branch is1812 test the behavior of the webservice rather than whether certain
1808already way too large.1813 combinations of annotations are valid.
1809
1810 The singular and plural name of an entry will not change between versions.
18111814
1812 If a field has a mutator in version n, later versions will inherit the1815 If a field has a mutator in version n, later versions will inherit the
1813 same mutator.1816 same mutator.
@@ -2245,8 +2248,118 @@
2245 >>> attrs['return_type']2248 >>> attrs['return_type']
2246 <lazr.restful.fields.CollectionField object...>2249 <lazr.restful.fields.CollectionField object...>
22472250
2248[XXX leonardr mutator_for modifies the field, not the method, so it2251Mutator operations
2249won't work until I add multiversion support for fields.]2252******************
2253
2254Different versions can define different mutator methods for the same field.
2255
2256 >>> class IDifferentMutators(Interface):
2257 ... export_as_webservice_entry()
2258 ...
2259 ... field = exported(TextLine(readonly=True))
2260 ...
2261 ... @mutator_for(field)
2262 ... @export_write_operation()
2263 ... @operation_for_version('beta')
2264 ... @operation_parameters(new_value=TextLine())
2265 ... def set_value(new_value):
2266 ... pass
2267 ...
2268 ... @mutator_for(field)
2269 ... @export_write_operation()
2270 ... @operation_for_version('1.0')
2271 ... @operation_parameters(new_value=TextLine())
2272 ... def set_value_2(new_value):
2273 ... pass
2274
2275 >>> ignored = generate_entry_interfaces(
2276 ... IDifferentMutators, 'beta', '1.0')
2277
2278But you can't define two mutators for the same field in the same version.
2279
2280 >>> class IDuplicateMutator(Interface):
2281 ... export_as_webservice_entry()
2282 ...
2283 ... field = exported(TextLine(readonly=True))
2284 ...
2285 ... @mutator_for(field)
2286 ... @export_write_operation()
2287 ... @operation_for_version('1.0')
2288 ... @operation_parameters(new_value=TextLine())
2289 ... def set_value(new_value):
2290 ... pass
2291 ...
2292 ... @mutator_for(field)
2293 ... @export_write_operation()
2294 ... @operation_for_version('1.0')
2295 ... @operation_parameters(new_value=TextLine())
2296 ... def set_value_2(new_value):
2297 ... pass
2298 Traceback (most recent call last):
2299 ...
2300 TypeError: A field can only have one mutator method for version
2301 1.0; set_value_2 makes two.
2302
2303Here's a case that's a little trickier. You'll also get an error if
2304you implicitly define a mutator for the earliest version (without
2305giving its name), and then define another one explicitly (giving the
2306name of the earliest version.)
2307
2308 >>> class IImplicitAndExplicitMutator(Interface):
2309 ... export_as_webservice_entry()
2310 ...
2311 ... field = exported(TextLine(readonly=True))
2312 ...
2313 ... @mutator_for(field)
2314 ... @export_write_operation()
2315 ... @operation_for_version('beta')
2316 ... @operation_parameters(new_value=TextLine())
2317 ... def set_value_2(new_value):
2318 ... pass
2319 ...
2320 ... @mutator_for(field)
2321 ... @export_write_operation()
2322 ... @operation_parameters(new_value=TextLine())
2323 ... def set_value_2(new_value):
2324 ... pass
2325
2326 >>> generate_entry_interfaces(
2327 ... IImplicitAndExplicitMutator, 'beta', '1.0')
2328 Traceback (most recent call last):
2329 ...
2330 ValueError: Duplicate mutator for interface
2331 "IImplicitAndExplicitMutator", field "field": Both implicit and
2332 explicit definitions found for earliest version beta.
2333
2334This error isn't detected until you try to generate the entry
2335interfaces, because until that point lazr.restful doesn't know that
2336'beta' is the earliest version. If the earliest version was 'alpha',
2337the IImplicitAndExplicitMutator class would be valid.
2338
2339(Again, to test this hypothesis, we need to re-define the class,
2340because the generate_entry_interfaces call modified the original
2341class's annotations in place.)
2342
2343 >>> class IImplicitAndExplicitMutator(Interface):
2344 ... export_as_webservice_entry()
2345 ...
2346 ... field = exported(TextLine(readonly=True))
2347 ...
2348 ... @mutator_for(field)
2349 ... @export_write_operation()
2350 ... @operation_for_version('beta')
2351 ... @operation_parameters(new_value=TextLine())
2352 ... def set_value_2(new_value):
2353 ... pass
2354 ...
2355 ... @mutator_for(field)
2356 ... @export_write_operation()
2357 ... @operation_parameters(new_value=TextLine())
2358 ... def set_value_2(new_value):
2359 ... pass
2360
2361 >>> ignored = generate_entry_interfaces(
2362 ... IImplicitAndExplicitMutator, 'alpha', 'beta', '1.0')
22502363
2251Destructor operations2364Destructor operations
2252*********************2365*********************

Subscribers

People subscribed via source and target branches