-
Notifications
You must be signed in to change notification settings - Fork 783
Coerce operation params according to model #147
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,4 @@ | ||
| import six | ||
| import sys | ||
| import copy | ||
| import logging | ||
|
|
@@ -18,6 +19,87 @@ | |
| LOG = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| class ParamCoercion(object): | ||
| """This class coerces string parameters into the correct type. | ||
|
|
||
| By default this converts strings to numerical values if the input | ||
| parameters model indicates that the field should be a number. This is to | ||
| compensate for the fact that values taken in from prompts will always be | ||
| strings and avoids having to create specific interactions for simple | ||
| conversions or having to specify the type in the wizard specification. | ||
| """ | ||
|
|
||
| _DEFAULT_DICT = { | ||
| 'integer': int, | ||
| 'float': float, | ||
| 'double': float, | ||
| 'long': int | ||
| } | ||
|
|
||
| def __init__(self, type_dict=_DEFAULT_DICT): | ||
| """Initialize a ParamCoercion object. | ||
|
|
||
| :type type_dict: dict | ||
| :param type_dict: (Optional) A dictionary of converstions. Keys are | ||
| strings representing the shape type name and the values are callables | ||
| that given a string will return an instance of an appropriate type for | ||
| that shape type. Defaults to only coerce numbers. | ||
| """ | ||
| self._type_dict = type_dict | ||
|
|
||
| def coerce(self, params, shape): | ||
| """Coerce the params according to the given shape. | ||
|
|
||
| :type params: dict | ||
| :param params: The parameters to be given to an operation call. | ||
|
|
||
| :type shape: :class:`botocore.model.Shape` | ||
| :param shape: The input shape for the desired operation. | ||
|
|
||
| :rtype: dict | ||
| :return: The coerced version of the params. | ||
| """ | ||
| name = shape.type_name | ||
| if isinstance(params, dict) and name == 'structure': | ||
| return self._coerce_structure(params, shape) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You also need to handle map and (maybe timestamp for completeness?) types. It might not hurt to look at the serializer and parser code in botocore if you need to do this as they have implementation of input/output to model traversers that you can copy.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps handling map will be worthwhile. Is there a situation that you can think of where a map is passed as input in a request? How does the CLI handle taking maps as input? I don't think timestamp is required as a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah a big example is dynamodb. It uses maps a fair amount. Furthermore, the shapes are recursive so this traversal of the model for a map is important. It would not be too difficult to support. It is like a mix between a structure and list. I could see it be something like: def _coerce_map(self, params, shape):
for key, value in params.items():
coerced_key = self._coerce(key, shape.key)
coereced_params[coereced_key] = self._coerce(params[value], shape.value)Also, that makes sense for timestamps. I just suggested that one because that is one that is often forgotten, along with maps, when implementing traversers. |
||
| elif isinstance(params, dict) and name == 'map': | ||
| return self._coerce_map(params, shape) | ||
| elif isinstance(params, (list, tuple)) and name == 'list': | ||
| return self._coerce_list(params, shape) | ||
| elif isinstance(params, six.string_types) and name in self._type_dict: | ||
| target_type = self._type_dict[shape.type_name] | ||
| return self._coerce_field(params, target_type) | ||
| return params | ||
|
|
||
| def _coerce_structure(self, params, shape): | ||
| members = shape.members | ||
| coerced = {} | ||
| for param in members: | ||
| if param in params: | ||
| coerced[param] = self.coerce(params[param], members[param]) | ||
| return coerced | ||
|
|
||
| def _coerce_map(self, params, shape): | ||
| coerced = {} | ||
| for key, value in params.items(): | ||
| coerced_key = self.coerce(key, shape.key) | ||
| coerced[coerced_key] = self.coerce(value, shape.value) | ||
| return coerced | ||
|
|
||
| def _coerce_list(self, list_param, shape): | ||
| member_shape = shape.member | ||
| coerced_list = [] | ||
| for item in list_param: | ||
| coerced_list.append(self.coerce(item, member_shape)) | ||
| return coerced_list | ||
|
|
||
| def _coerce_field(self, value, target_type): | ||
| try: | ||
| return target_type(value) | ||
| except ValueError: | ||
| return value | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably want to log this error.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My thought on this was that this class would attempt to coerce simple things that it knows how to do, and if for some reason this conversion fails it will do nothing. These params will then get passed to the botocore validator once the request is actually made where a more proper validation error could be thrown. I don't recall what kind of logging the validator does. Perhaps logging this here is also needed? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It does not need logging as its log is pretty much the error raised. It notes every spot where validation fails. I am fine without logging this. I think it is my confusion with the implication of coercion being best effort. I wonder if there is a better name for this. |
||
|
|
||
|
|
||
| def stage_error_handler(error, stages, confirm=confirm, prompt=select_prompt): | ||
| managed_errors = ( | ||
| ClientError, | ||
|
|
@@ -264,6 +346,8 @@ def _handle_request_retrieval(self): | |
| self._env.resolve_parameters(req.get('EnvParameters', {})) | ||
| # union of parameters and env_parameters, conflicts favor env params | ||
| parameters = dict(parameters, **env_parameters) | ||
| model = client.meta.service_model.operation_model(req['Operation']) | ||
| parameters = ParamCoercion().coerce(parameters, model.input_shape) | ||
| # if the operation supports pagination, load all results upfront | ||
| if client.can_paginate(operation_name): | ||
| # get paginator and create iterator | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,9 +3,10 @@ | |
| import botocore.session | ||
|
|
||
| from botocore.loaders import Loader | ||
| from botocore import model | ||
| from botocore.session import Session | ||
| from awsshell.utils import FileReadError | ||
| from awsshell.wizard import stage_error_handler | ||
| from awsshell.wizard import stage_error_handler, ParamCoercion | ||
| from awsshell.interaction import InteractionException | ||
| from botocore.exceptions import ClientError, BotoCoreError | ||
| from awsshell.wizard import Environment, WizardLoader, WizardException | ||
|
|
@@ -383,3 +384,60 @@ def test_stage_exception_handler_other(error_class): | |
| err = error_class() | ||
| res = stage_error_handler(err, ['stage'], confirm=confirm, prompt=prompt) | ||
| assert res is None | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def test_shape(): | ||
| shapes = { | ||
| "TestShape": { | ||
| "type": "structure", | ||
| "members": { | ||
| "Huge": {"shape": "Long"}, | ||
| "Map": {"shape": "TestMap"}, | ||
| "Scale": {"shape": "Double"}, | ||
| "Count": {"shape": "Integer"}, | ||
| "Items": {"shape": "TestList"} | ||
| } | ||
| }, | ||
| "TestList": { | ||
| "type": "list", | ||
| "member": { | ||
| "shape": "Float" | ||
| } | ||
| }, | ||
| "TestMap": { | ||
| "type": "map", | ||
| "key": {"shape": "Double"}, | ||
| "value": {"shape": "Integer"} | ||
| }, | ||
| "Long": {"type": "long"}, | ||
| "Float": {"type": "float"}, | ||
| "Double": {"type": "double"}, | ||
| "String": {"type": "string"}, | ||
| "Integer": {"type": "integer"} | ||
| } | ||
| return model.ShapeResolver(shapes).get_shape_by_name('TestShape') | ||
|
|
||
|
|
||
| def test_param_coercion_numbers(test_shape): | ||
| # verify coercion will convert strings to numbers according to shape | ||
| params = { | ||
| "Count": "5", | ||
| "Scale": "2.3", | ||
| "Items": ["5", "3.14"], | ||
| "Huge": "92233720368547758070", | ||
| "Map": {"2": "12"} | ||
| } | ||
| coerced = ParamCoercion().coerce(params, test_shape) | ||
| assert isinstance(coerced['Count'], int) | ||
| assert isinstance(coerced['Scale'], float) | ||
| assert all(isinstance(item, float) for item in coerced['Items']) | ||
| assert coerced['Map'][2] == 12 | ||
| assert coerced['Huge'] == 92233720368547758070 | ||
|
|
||
|
|
||
| def test_param_coercion_failure(test_shape): | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, so how does the error get propogated if an invalid value is passed in like "fifty" from the users prospective?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's a good point. An error will be raised when the request itself is attempted. That'll allow the user to go back to a previous stage or quit. |
||
| # verify coercion leaves the field the same when it fails | ||
| params = {"Count": "fifty"} | ||
| coerced = ParamCoercion().coerce(params, test_shape) | ||
| assert coerced["Count"] == params["Count"] | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am a little confused as to why you do the isinstance check and check the type name? If the type matches what coercion happens? Or is the coercion is mainly for the type_dict values? I feel like you would try to coerce it no matter what.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One is checking the name of the model's shape, and one is verifying that the parameter actually meets that constraint. It's not possible to continue the recursion if they don't match.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah so the question then is it better to silently pass it off to the botocore to validate or have the error happen right there. Botocore has good error messages so it may be better to delegate the issue to it. I guess in the end when you call everything
coerceit seems like it must force the correct type when it really is a best effort coercion