Code review comment for lp:~leonardr/lazr.restful/enforce-version-order

Revision history for this message
Leonard Richardson (leonardr) wrote :

This branch adds two bits of multiversion-related error checking.

1. Named operations are annotated with a stack of version information. The most recent version is supposed to be at the top of the stack.

This is right:

@operation_for_version("2.0")
...
@operation_for_version("1.0")
...
@operation_for_version("beta")

This is wrong:

@operation_for_version("2.0")
...
@operation_for_version("beta")
...
@operation_for_version("1.0")

The ensure_correct_version_ordering function looks up the canonical version ordering in the IWebServiceConfiguration implementation, and makes sure every named operation is annotated with a consistent ordering. If the developer makes a mistake, ensure_correct_version_ordering raises an AssertionError explaining the problem and giving the correct ordering.

This code has to run in the second stage of ZCML processing, rather than when the annotations are initially processed, because until the IWebServiceConfiguration utility becomes available, there's no way to know whether version "foo" comes before or after version "bar".

2. If a named operation is not published in a certain version, it makes no sense to annotate it for that version.

@export_operation_as("its_gone_in_1_0") <--This annotation is stupid!
@operation_removed_in_version("1.0")
@export_as_read_operation()

The register_webservice_operations method will now make sure there are no extra annotations for a version in which a named operation is not published. If there are any extra annotations, it will raise an AssertionError and complain about them.

This code has to run in the second stage of ZCML processing because by then all the annotations have been processed. If I make each annotations in charge of complaining when it's applied to an unpublished method, it'll reject cases in which the method seems unpublished at the time but gets published later.

@export_as_read_operation()
@export_operation_as("its_redefined_in_1_0") <--This looks stupid, but turns out to be valid!
@operation_removed_in_version("1.0")
@export_as_read_operation()

This set of annotations is sloppier than this one:

@export_operation_as("its_redefined_in_1_0") <--This is clearly valid!
@export_as_read_operation()
@operation_removed_in_version("1.0")
@export_as_read_operation()

But that's not worth raising an error about.

« Back to merge proposal