yelp / bravado Goto Github PK

/swaggerpy/response.py#handle_response_errors is not preserving the stacktrace when reraising the errors, making debugging difficult.

Moving to the 3 parameters version of raise would solve this :

if hasattr(e, 'response') and hasattr(e.response, 'text'):
        ....

exc_info = sys.exc_info()
raise sys.exc_info[0], e.args, sys.exc_info[2]

We're seeing an issue where daemonized processes that use swagger-py fail. I believe this is due to swagger-py importing from twisted.internet import reactor, which causes the twisted reactor to be initialzied. The twisted reactor uses file handles which are closed during daemonize, so the reactor needs to be initialized only when the first call is made (not at import time).

There may be other imports that trigger this behaviour as well.

Backwards compatibility with swaggerpy breaks as _request_options is not accepted in bravado:

swagger_client.pet.addPet(
    body=pet,
    _request_options={"headers": {"foo": "bar"}},
).result()

should add the additional header to the request.

There is no way to add custom headers to the request right now.

It's a bit weird to have our single HTTPFuture that has methods that don't do anything for the synchronous case.

In addition, our "clients" are actually more of request factories at this point, so they don't really have the responsibility to wait or cancel the request.

See pull #69 for a bit of chat about this. We probably should rename our clients to factories and split up HTTPFuture for the cases it's used.

Valid:

consumes": [
    "application/json",
    "application/xml"
],

Swagger-py's HTTPError is an exact copy of request's one
https://github.com/Yelp/swagger-py/blob/master/swaggerpy/exception.py#L9-L21
https://github.com/kennethreitz/requests/blob/master/requests/exceptions.py#L13-L31

but the SynchronousHttpClient throws requests.exceptions.HTTPError, and AsynchronousHttpClient throws swaggerpy.exception.HTTPError which makes error handling closely tied to the client used.

replacing the HTTPError code in swaggerpy/exception.py by a from requests.exceptions import HTTPError could solve this and by backward compatible.

See pull #79.

"I think this should handle date as well (since those are both supported types in 1.2), and the datetime format this is using is not correct. I believe it should be using date-time from http://xml2rfc.ietf.org/public/rfc/html/rfc3339.html#anchor14, which is explicitly called out in the 2.0 spec. "

Need to be able to pass similar _request_options to api docs requests

Since building up the resources is expensive, but async and sync clients share everything else, make a way to switch between using a sync and an async http_client without having 2 completely separate swaggerpy clients. Initial thought is instead of client.user.method you'd call client.sync.user.method or client.async.user.method. from @dnephin: "It's probably just a matter of exposing the "build resources" interface"

swaggerpy seems to have some 'support' for files, but doesn't appear to be doing it quite the right way, as swagger is pretty specific as to the conditions.

https://github.com/swagger-api/swagger-spec/blob/master/versions/1.2.md#435-file

Seems like strange behavior to me. Might be worth taking a look at again at some point.

Edit: Actually, just ignores it always I'm pretty sure, though comments suggest otherwise = |

This is what is scoped:

Validations -

• if any param's paramType is form, consumes MUST contain application/x-www-form-urlencoded OR multipart/form-data
• form parameters MUST be only of primitive types or File type.
• if any param's paramType is form, no param with paramType body SHOULD be present.

Tasks -

Synchronous (requests library)

• Create a dict of all form parameters, something like request['data'][pname] = value
• If any parameter is of type file:
• `request['files'][pname] = value
• Set headers['content-type'] = 'multipart/form-data'
• else :
  • Set headers['content-type'] = 'application/x-www-form-urlencoded'

Asynchronous (twisted library)

• Create a dict of all form parameters, something like request['data'][pname] = value
• request['data'] = urllib.urlencode(request['data'])
• Set headers similar to Synchronous client.
• If any parameter is of type file:
  • ?? something like FileProducer(StringIO(file.read()))
  • related 1, 2, 3
  • limitations - just 1 file upload ??

Probably with the name swagger-py.

I tried this:

swagger_client = client.get_client('http://petstore.swagger.io/v2/swagger.json')
swagger_client.pet.findPetsByStatus(status=['available']).result()

Which throws back:

---> 33     jsonschema.validate(value, spec)
     34
     35

/usr/lib/pymodules/python2.6/jsonschema/validators.pyc in validate(instance, schema, cls, *args, **kwargs)
    504     """
    505     if cls is None:
    506         cls = validator_for(schema)
--> 507     cls.check_schema(schema)
    508     cls(schema, *args, **kwargs).validate(instance)

/usr/lib/pymodules/python2.6/jsonschema/validators.pyc in check_schema(cls, schema)
     74         def check_schema(cls, schema):
     75             for error in cls(cls.META_SCHEMA).iter_errors(schema):
---> 76                 raise SchemaError.create_from(error)
     77
     78         def iter_errors(self, instance, _schema=None):

SchemaError: False is not of type u'array'

Failed validating u'type' in schema[u'properties'][u'required']:
    {u'items': {u'type': u'string'},
     u'minItems': 1,
     u'type': u'array',
     u'uniqueItems': True}

On instance[u'required']:
    False

It looks like swagger_spec can not be directly treated as json_schema to validate the request.

On removing required field from the spec, and retrying gives:

ValidationError: ['available'] is not one of [u'available', u'pending', u'sold']

Failed validating u'enum' in schema:
    {u'collectionFormat': u'multi',
     u'default': u'available',
     u'description': u'Status values that need to be considered for filter',
     u'enum': [u'available', u'pending', u'sold'],
     u'in': u'query',
     u'items': {u'type': u'string'},
     u'name': u'status',
     u'type': u'array'}

On instance:
    ['available']

In other words, jsonschema.validate will not work in these cases.

We're seeing that building the client for large APIs (~30 api docs) can be very slow (900ms!). This ticket is to track some possible improvements.

From the profile:

1. making requests for documents is ~700ms. We should do these async and all at once
2. create_model_docstring() is 15ms, which is significant, and is only going to be used in very rare situations. It would be great to find a way to avoid this time, maybe with an optional parameter to enable them?

There are many use-cases for decorating a client to provide additional functionality. Some common ones that have come up before are:

• wrapping exceptions, either to cast them to another type, log them, or catch them and prevent them from raising
• providing defaults for _request_options (especially headers) to every service call. This is useful for things like tracking ids, authentication headers, force-master-read, etc.
• retry and timeout behaviour. Within a single application it's likely that these will remain pretty consistent, so providing a decorator allows you to re-use the values for all calls

Since our service calls aren't exposed directly from the SwaggerClient there is a bit more work involved to create a decorator (the SwaggerClient only exposes resources, the resources expose operation calls, which are what we want to decorate).

I think we should provide some support for this in bravado (potentially swagger-py as well, they should be reusable for both since the external interface is basically identical).

I think there are two potential options.

• provide a ClientDecorator that knows how to decorate both the resources and operations. This option is less coupled with swagger-py itself, but requires more code.
• have SwaggerClient accept a list of IOperationDecorator objects which conform to some interface and apply the decorators directly to the operation. More tightly coupled with swagger-py, but easier to implement.

I prefer the second option.

The Swagger spec has a json schema. We should validate downloaded specs against this schema to make it easier to detect any errors.

Right now, api_base_path is used only if schema's basePath starts with '/'.

If the user wants to alter the basePath during making API call and the basePath in the schema is absolute (like http://localhost), there is no way to do it.

Right now, /api-docs is fetched by a simple SynchronousHttpClient with no headers. Use headers of http_client if present for fetching it.

Retrieving cached clients should definitely overwrite the headers being sent when they're retrieved.

https://github.com/swagger-api/swagger-spec/blob/master/versions/1.2.md#524-parameter-object

First, we should care about the allowMultiple flag. In addition, we should convert arrays to comma separated lists only in these correct circumstances.

Currently swagger-py passes query params directly to urllib.urlencode (swaggerpy/client.py line 169, in __call__). urllib doesn't support unicode objects; you must pass in a UTF-8 encoded string. Thus, if you pass unicode objects to swaggerpy you'll get a UnicodeEncodeError.

You could do the .encode('UTF-8') call manually in calling code, but that violates the general unicode handling rule of "decode at the start, encode at the end, and use unicodes everywhere in-between", and will result in many ad-hoc solutions when a general one is available.

swagger-py tries to create default instance of datetime, but datetime doesn't have a zero args constructor.

n [9]: client.store.getOrderById(orderId="1").result()
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-9-6265fd495d50> in <module>()
----> 1 client.store.getOrderById(orderId="1").result()

/nail/home/prateek/pg/mypy2/lib/python2.6/site-packages/swaggerpy/response.pyc in result(self, timeout)
     50         response = self._http_client.wait(timeout)
     51         response.raise_for_status()
---> 52         return self._postHTTP_callback(response)
     53
     54

/nail/home/prateek/pg/mypy2/lib/python2.6/site-packages/swaggerpy/client.pyc in py_model_convert_callback(response)
    132                 # Validate and convert API response to Python model instance
    133                 value = SwaggerResponse(
--> 134                     response.json(), type_, self._models).swagger_object
    135             return value
    136         return HTTPFuture(self._http_client,

/nail/home/prateek/pg/mypy2/lib/python2.6/site-packages/swaggerpy/response.pyc in __init__(self, response, type_, models)
     94         self.swagger_object = SwaggerResponseConstruct(response,
     95                                                        type_,
---> 96                                                        models).create_object()
     97
     98

/nail/home/prateek/pg/mypy2/lib/python2.6/site-packages/swaggerpy/response.pyc in create_object(self)
    126         if swagger_type.is_array(self._type):
    127             return self._create_array_object()
--> 128         return self._create_complex_object()
    129
    130     def _create_array_object(self):

/nail/home/prateek/pg/mypy2/lib/python2.6/site-packages/swaggerpy/response.pyc in _create_complex_object(self)
    144         """
    145         klass = getattr(self._models, self._type)
--> 146         instance = klass()
    147         for key in self._response.keys():
    148             type_ = klass._swagger_types[key]

/nail/home/prateek/pg/mypy2/lib/python2.6/site-packages/swaggerpy/swagger_model.pyc in <lambda>(self, **kwargs)
    296         __eq__=lambda self, other: compare(self, other),
    297         # Define the constructor for the type
--> 298         __init__=lambda self, **kwargs: set_props(self, **kwargs),
    299         # Define the str repr of the type
    300         __repr__=lambda self: create_model_repr(self))

/nail/home/prateek/pg/mypy2/lib/python2.6/site-packages/swaggerpy/swagger_model.pyc in set_props(model, **kwargs)
    322         swagger_py_type = swagger_type.swagger_to_py_type(
    323             property_swagger_type)
--> 324         property_value = swagger_py_type() if swagger_py_type else None
    325         # Override any property values specified in kwargs
    326         if property_name in arg_keys:

TypeError: Required argument 'year' (pos 1) not found

Swagger 2.0 Spec

The client caching seems to be effectively disabled due to the changes introduced in 46a68a4. More specifically, I think that the problem manifests itself in line 65:

key = repr(args) + repr(kwargs)

For example, when get_client(...) gets called with the kwarg http_client=AsynchronousHttpClient(), then the return value of repr(kwargs) will include the specific memory address of the AsynchronousHttpClient object:
'http_client': <swaggerpy.async_http_client.AsynchronousHttpClient object at 0x397ed10>

Therefore, actually a unique key is stored in the cache dict for every get_client(...) function call.

SynchronousHttpClient shouldn't need the .request method. We should handle the api docs requests through the same requests code that we handle model requests.

We've observed that the SynchronousHttpClient and AsynchronousHttpClient raise different exceptions (requests.exceptions.HTTPError vs swaggerpy.exception.HTTPError).

We need to ensure that all exceptions at least have the same base class, regardless of client implementation.

We may be able to wait for sprockets, but we should still address this.

This only occurs for swaggerpy clients that use the AsynchronousHttpClient

The response is being returned and raising an crochet._eventloop.TimeoutError

I have a swagger schema that has an object with type "any". It passes pyramid_swagger validation but didn't pass swagger-py validation. So maybe swagger-py should add "any" to the supported types?

When creating an asynchronous client with get_client(...) and passing in an api_doc_request_headers object, that objects gets mutated. More specifically, the string values of the dict get wrapped in a list:

In [1]: from swaggerpy import client
In [2]: from swaggerpy.async_http_client import AsynchronousHttpClient
In [3]: headers = {'Host': 'test'}
In [4]: swagger_client = client.get_client(
   ...:         "http://petstore.swagger.wordnik.com/api/api-docs",
   ...:         http_client=AsynchronousHttpClient(),
   ...:         api_doc_request_headers=headers,
   ...: )
In [5]: headers
Out[5]: {'Host': ['test']}

This only happens for the asynchronous client and does not happen for the synchronous one:

In [6]: headers = {'Host': 'test'}
In [7]: swagger_client = client.get_client(
        "http://petstore.swagger.wordnik.com/api/api-docs",
        api_doc_request_headers=headers,
)
In [8]: headers
Out[8]: {'Host': 'test'}

While this is not a severe bug, it is definitely unexpected behavior.

Currently we use swagger-py to make a call with params like the following:

param = {
            'q': prefix.encode('utf-8'),
            'recent_locations': [location.encode('utf-8')
                                 for location in recent_locations],
        }

where each location is a string like "San Francisco, CA"

On the service side we get the recent_locations with the following:

recent_locations = request.params.getall('recent_locations')

Also note that our api docs for this parameter look like:

{
    "name": "recent_locations",
    "description": "recent locations from RECENT_LOCATIONS cookie",
    "type": "string",
    "paramType": "query",
    "required": false
},

This has worked fine for us up to version 0.7.2. However, after bumping to 0.7.3+ we get an error whenever the service endpoint is called with a message:

"dictionary update sequence element #0 has length 16; 2 is required"

This must have been caused by how swagger-py handles / encodes lists changing between 0.7.2 and 0.7.3 or 0.7.4

See https://jira.yelpcorp.com/browse/SUGGEST-284 for more context and a full stack trace.

raise_for_status() message is not carrying the server's error response.

We chatted a bit about this today. I still want to write a test for it, but I won't have time to do that for the next couple of weeks, so I wanted to get it documented here.

A service will need to change its API. API changes need to be backwards compatible, or increase the version number (while still providing the old version). In the case where an endpoint is only adding more data to the response, we shouldn't need to increase the endpoint version number. Clients should be fine to ignore any unexpected fields.

If the client receives extra data in the response, it should not raise an error. A service should be able to add extra fields to its response object without having to do a api version bump, as long as the old fields are unmodified.

This has the potential to create a lot of failures. I don't believe there is a test for this in swagger-py, but we should verify it works correct in bravado before release.

I added swagger schema with latitude as a request query parameter. I used 'type':'number' and 'format':'double'

eg: "parameters": [
{
"name": "latitude",
"description": "Latitude.",
"required": true,
"type": "number",
"format": "double",
"paramType": "query"
}
]

Validation passed. But when I make the request with latitude=37.7485, it fails saying
HTTPError: 400 Client Error : 400 Bad Request

The server could not comply with the request since it is either malformed or otherwise incorrect.

u'37.7485' is not of type 'number'

The link to http://bravado.readthedocs.org produces a 404 page.

In #44 and #45 we added a raise_with param to the client. I believe we should remove support for this param:

• it can be handled without any code in swagger-py (see below)
• it's an attribute that gets added to the http_client, but it not actually part of the class. This makes it mutable, and we would run into errors like those we addressed in #68

Instead we can provide this (either as part of swagger-py or in documentation):

class ErrorHandler(object):

    def __init__(self, client, handler):
        self.client = client
        self.handler = handler

    def __getattr__(self, name):
        def operation(*args, **kwargs):
            try:
                return getattr(self.swagger_client, name)(*args, **kwargs)
            exception Exception as e:
                self.handler(name, e)

client = ErrorHandler(SwaggerClient(...), my_error_handler)

Instead of providing a default value, remove the model field if not present in the response.

Changes should be around here

If you declare a list of futures, and then later call result() on each future, swaggerpy will return a list of identical values corresponding to the last future created.

ex:
myfutures = [swaggerclient.myendpoint for elem in elem_list]
myresults = [future.result() for future in myfutures]

myresults will return a list of identical values, all equal to myfutures[-1].result()

See the comments from #127

Inconsistent with versioning format. I'll be doing this on 10/20/2014 unless someone protests.

'raw_response' can be passed as a parameter to result() which will ignore any validations on response and return it raw.

Refactor bravado/mapping/operation.py such that response_future is all done by specific HttpClient and we can get rid of type checking. Refer #117 comments for details.