lazr.restful

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

  1. multiversion-destructors
  2. Merge into trunk
Proposed by Leonard Richardson
Status: Merged
Merged at revision: not available
Proposed branch: lp:~leonardr/lazr.restful/multiversion-destructors
Merge into: lp:lazr.restful
Diff against target: 227 lines (+94/-29)
2 files modified
src/lazr/restful/declarations.py (+37/-23)
src/lazr/restful/docs/webservice-declarations.txt (+57/-6)
To merge this branch: bzr merge lp:~leonardr/lazr.restful/multiversion-destructors
Reviewer Review Type Date Requested Status
Paul Hummer (community) code Approve
Review via email: mp+18643@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Leonard Richardson (leonardr) wrote :

This branch adds multi-version error checking for destructor methods. A destructor method cannot have any arguments that are not fixed to specific values. The previous code was not version-aware, so it was only checking for the most recent version of the web service. In this branch I check every version in which the destructor method is published.

I also defined a tiny helper method, _version_name, which takes care of this logic which I was putting all over the place when printing out error messages:

    if version is None:
        version = "(earliest version)"

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

Based on previous knowledge from reviewing the dependent branch, this branch is a bit easier to grok. Once again, your doctests make this patch easier to understand.

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 21:50:06 +0000
+++ src/lazr/restful/declarations.py 2010-02-04 21:53:10 +0000
@@ -285,11 +285,10 @@
285 'collection_default_content', {})285 'collection_default_content', {})
286286
287 if version in default_content_methods:287 if version in default_content_methods:
288 if version is None:
289 version = "(earliest version)"
290 raise TypeError(288 raise TypeError(
291 "Only one method can be marked with "289 "Only one method can be marked with "
292 "@collection_default_content for version '%s'." % version)290 "@collection_default_content for version '%s'." % (
291 _version_name(version)))
293 self.version = version292 self.version = version
294 self.params = params293 self.params = params
295294
@@ -424,11 +423,6 @@
424 # Make sure that each version of the web service defines423 # Make sure that each version of the web service defines
425 # a self-consistent view of this method.424 # a self-consistent view of this method.
426 for version, annotations in annotation_stack.stack:425 for version, annotations in annotation_stack.stack:
427 if version is None:
428 # Create a human-readable name for the earliest version
429 # without trying to look it up.
430 version = "(earliest version)"
431
432 if annotations['type'] == REMOVED_OPERATION_TYPE:426 if annotations['type'] == REMOVED_OPERATION_TYPE:
433 # The method is published in other versions of the web427 # The method is published in other versions of the web
434 # service, but not in this one. Don't try to validate this428 # service, but not in this one. Don't try to validate this
@@ -456,15 +450,15 @@
456 raise TypeError(450 raise TypeError(
457 'method "%s" doesn\'t have the following exported '451 'method "%s" doesn\'t have the following exported '
458 'parameters in version "%s": %s.' % (452 'parameters in version "%s": %s.' % (
459 method.__name__, version,453 method.__name__, _version_name(version),
460 ", ".join(sorted(undefined_params))))454 ", ".join(sorted(undefined_params))))
461 missing_params = set(455 missing_params = set(
462 info['required']).difference(exported_params)456 info['required']).difference(exported_params)
463 if missing_params:457 if missing_params:
464 raise TypeError(458 raise TypeError(
465 'method "%s" is missing exported parameter definitions '459 'method "%s" is missing definitions for parameter(s) '
466 'in version "%s": %s' % (460 'exported in version "%s": %s' % (
467 method.__name__, version,461 method.__name__, _version_name(version),
468 ", ".join(sorted(missing_params))))462 ", ".join(sorted(missing_params))))
469463
470 _update_default_and_required_params(annotations['params'], info)464 _update_default_and_required_params(annotations['params'], info)
@@ -784,21 +778,29 @@
784 """778 """
785 type = "destructor"779 type = "destructor"
786780
787 def annotate_method(self, method, annotations):781 def annotate_method(self, method, annotation_stack):
788 """See `_method_annotator`.782 """See `_method_annotator`.
789783
790 Store information about the mutator method with the method.784 Store information about the mutator method with the method.
785
786 Every version must have a self-consistent set of annotations.
791 """787 """
788 super(export_destructor_operation, self).annotate_method(
789 method, annotation_stack)
792 # The mutator method must take no arguments, not counting790 # The mutator method must take no arguments, not counting
793 # arguments with values fixed by call_with().791 # arguments with values fixed by call_with().
794 signature = fromFunction(method).getSignatureInfo()792 signature = fromFunction(method).getSignatureInfo()
795 free_params = _free_parameters(method, annotations)793 for version, annotations in annotation_stack.stack:
796 if len(free_params) != 0:794 if annotations['type'] == REMOVED_OPERATION_TYPE:
797 raise TypeError("A destructor method must take no "795 continue
798 "non-fixed arguments; %s takes %d." %796 free_params = _free_parameters(method, annotations)
799 (method.__name__, len(free_params)))797 if len(free_params) != 0:
800 super(export_destructor_operation, self).annotate_method(798 raise TypeError(
801 method, annotations)799 "A destructor method must take no non-fixed arguments. "
800 'In version %s, the "%s" method takes %d: "%s".' % (
801 _version_name(version), method.__name__,
802 len(free_params), '", "'.join(free_params))
803 )
802804
803805
804def _check_tagged_interface(interface, type):806def _check_tagged_interface(interface, type):
@@ -845,12 +847,11 @@
845 if annotations.get('type') == export_destructor_operation.type:847 if annotations.get('type') == export_destructor_operation.type:
846 destructor = destructor_for_version.get(version)848 destructor = destructor_for_version.get(version)
847 if destructor is not None:849 if destructor is not None:
848 if version is None:
849 version = "(earliest)"
850 raise TypeError(850 raise TypeError(
851 'An entry can only have one destructor method for '851 'An entry can only have one destructor method for '
852 'version %s; %s and %s make two.' % (852 'version %s; %s and %s make two.' % (
853 version, method.__name__, destructor.__name__))853 _version_name(version), method.__name__,
854 destructor.__name__))
854 destructor_for_version[version] = method855 destructor_for_version[version] = method
855856
856 # Next, we'll normalize each published field. A normalized field857 # Next, we'll normalize each published field. A normalized field
@@ -1267,6 +1268,19 @@
1267 versioned_dict.stack = new_stack1268 versioned_dict.stack = new_stack
1268 return field1269 return field
12691270
1271
1272def _version_name(version):
1273 """Return a human-readable version name.
1274
1275 If `version` is None (indicating the as-yet-unknown earliest
1276 version), returns "(earliest version)". Otherwise returns the version
1277 name.
1278 """
1279 if version is None:
1280 return "(earliest version)"
1281 return version
1282
1283
1270def _versioned_class_name(base_name, version):1284def _versioned_class_name(base_name, version):
1271 """Create a class name incorporating the given version string."""1285 """Create a class name incorporating the given version string."""
1272 if version is None:1286 if version is None:
12731287
=== modified file 'src/lazr/restful/docs/webservice-declarations.txt'
--- src/lazr/restful/docs/webservice-declarations.txt 2010-02-03 20:59:22 +0000
+++ src/lazr/restful/docs/webservice-declarations.txt 2010-02-04 21:53:10 +0000
@@ -534,8 +534,8 @@
534 ... def a_method(param1, param2, param3, param4): pass534 ... def a_method(param1, param2, param3, param4): pass
535 Traceback (most recent call last):535 Traceback (most recent call last):
536 ...536 ...
537 TypeError: method "a_method" is missing exported parameter definitions537 TypeError: method "a_method" is missing definitions for parameter(s)
538 in version "(earliest version)": param3, param4538 exported in version "(earliest version)": param3, param4
539539
540Defining a parameter not available on the method also results in an540Defining a parameter not available on the method also results in an
541error:541error:
@@ -1281,12 +1281,14 @@
1281 ... text = exported(TextLine(readonly=True))1281 ... text = exported(TextLine(readonly=True))
1282 ...1282 ...
1283 ... @export_destructor_operation()1283 ... @export_destructor_operation()
1284 ... @operation_parameters(argument=TextLine())
1284 ... def destroy(argument):1285 ... def destroy(argument):
1285 ... pass1286 ... pass
1286 Traceback (most recent call last):1287 Traceback (most recent call last):
1287 ...1288 ...
1288 TypeError: A destructor method must take no non-fixed arguments;1289 TypeError: A destructor method must take no non-fixed arguments.
1289 destroy takes 1.1290 In version (earliest version), the "destroy" method takes 1:
1291 "argument".
12901292
1291 >>> class IHasText(Interface):1293 >>> class IHasText(Interface):
1292 ... export_as_webservice_entry()1294 ... export_as_webservice_entry()
@@ -1298,7 +1300,6 @@
1298 ... pass1300 ... pass
1299 >>> ignored = generate_entry_interfaces(IHasText, 'beta')1301 >>> ignored = generate_entry_interfaces(IHasText, 'beta')
13001302
1301
1302An entry cannot have more than one destructor.1303An entry cannot have more than one destructor.
13031304
1304 >>> from lazr.restful.declarations import export_destructor_operation1305 >>> from lazr.restful.declarations import export_destructor_operation
@@ -1317,7 +1318,7 @@
1317 Traceback (most recent call last):1318 Traceback (most recent call last):
1318 ...1319 ...
1319 TypeError: An entry can only have one destructor method for1320 TypeError: An entry can only have one destructor method for
1320 version (earliest); destroy and destroy2 make two.1321 version (earliest version); destroy and destroy2 make two.
13211322
13221323
1323Mutators1324Mutators
@@ -2247,6 +2248,56 @@
2247[XXX leonardr mutator_for modifies the field, not the method, so it2248[XXX leonardr mutator_for modifies the field, not the method, so it
2248won't work until I add multiversion support for fields.]2249won't work until I add multiversion support for fields.]
22492250
2251Destructor operations
2252*********************
2253
2254A destructor can be published in different ways in different versions,
2255but the restrictions on destructor arguments are enforced separately
2256for each version.
2257
2258Here, the destructor fixes a value for the 'fixed2' argument in the
2259earliest version, but not in '1.0'. This is fine: the 1.0 value for
2260'fixed2' will be inherited from the previous version.
2261
2262 >>> class IGoodDestructorEntry(Interface):
2263 ... export_as_webservice_entry()
2264 ...
2265 ... @call_with(fixed1="value3")
2266 ... @operation_for_version('1.0')
2267 ... @export_destructor_operation()
2268 ... @call_with(fixed1="value1", fixed2="value")
2269 ... @operation_parameters(fixed1=TextLine(), fixed2=TextLine())
2270 ... def destructor(fixed1, fixed2):
2271 ... """Another destructor method."""
2272
2273 >>> ignore = generate_entry_interfaces(
2274 ... IGoodDestructorEntry, 'beta', '1.0')
2275
2276This is not fine. In this case, the destructor is removed in 1.0 and
2277added back in 2.0. The 2.0 version does not inherit any values from
2278its prior incarnation, so the fact that it does not fix any value for
2279'fixed2' is a problem.
2280
2281 >>> class IBadDestructorEntry(Interface):
2282 ... export_as_webservice_entry()
2283 ...
2284 ... @export_destructor_operation()
2285 ... @operation_parameters(fixed1=TextLine(), fixed2=TextLine())
2286 ... @call_with(fixed1="value3")
2287 ... @operation_for_version('2.0')
2288 ... @operation_removed_in_version('1.0')
2289 ... @export_destructor_operation()
2290 ... @call_with(fixed1="value1", fixed2="value")
2291 ... @operation_parameters(fixed1=TextLine(), fixed2=TextLine())
2292 ... def destructor(fixed1, fixed2):
2293 ... """Another destructor method."""
2294 Traceback (most recent call last):
2295 ...
2296 TypeError: A destructor method must take no non-fixed
2297 arguments. In version 2.0, the "destructor" method takes 1:
2298 "fixed2".
2299
2300
2250Security2301Security
2251========2302========
22522303

Subscribers

People subscribed via source and target branches