Skip to content
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

[python] required fields #6291

Open
InoMurko opened this issue Aug 11, 2017 · 16 comments
Open

[python] required fields #6291

InoMurko opened this issue Aug 11, 2017 · 16 comments

Comments

@InoMurko
Copy link

Python generated client model does not appear to be working for required parameters and reports that None is being passed for required parameters name and catalog_id:

`ERROR: test_backoffice_api_merchant_ad_controller_show (test.test_ad_api.TestAdApi)

Traceback (most recent call last):
File "/opt/priv/backoffice/swagger/test/test_ad_api.py", line 99, in test_backoffice_api_merchant_ad_controller_show
api_response = api_instance.backoffice_api_merchant_ad_controller_show(merchant_id=5, authorization=auth, ad_id=id)
File "/opt/priv/backoffice/swagger/swagger_client/apis/ad_api.py", line 298, in backoffice_api_merchant_ad_controller_show
(data) = self.backoffice_api_merchant_ad_controller_show_with_http_info(merchant_id, ad_id, authorization, **kwargs)
File "/opt/priv/backoffice/swagger/swagger_client/apis/ad_api.py", line 383, in backoffice_api_merchant_ad_controller_show_with_http_info
collection_formats=collection_formats)
File "/opt/priv/backoffice/swagger/swagger_client/api_client.py", line 327, in call_api
_return_http_data_only, collection_formats, _preload_content, _request_timeout)
File "/opt/priv/backoffice/swagger/swagger_client/api_client.py", line 158, in __call_api
return_data = self.deserialize(response_data, response_type)
File "/opt/priv/backoffice/swagger/swagger_client/api_client.py", line 240, in deserialize
return self.__deserialize(data, response_type)
File "/opt/priv/backoffice/swagger/swagger_client/api_client.py", line 280, in __deserialize
return self.__deserialize_model(data, klass)
File "/opt/priv/backoffice/swagger/swagger_client/api_client.py", line 621, in __deserialize_model
instance = klass()
File "/opt/priv/backoffice/swagger/swagger_client/models/ad_show_response.py", line 93, in init
self.name = name
File "/opt/priv/backoffice/swagger/swagger_client/models/ad_show_response.py", line 275, in name
raise ValueError("Invalid value for name, must not be None")
ValueError: Invalid value for name, must not be None
`

Swagger-codegen version

v2.2.3

https://gist.github.com/InoMurko/ce6aa8b938eca3124fc46f3cdddaf8f1

Command line used for generation

python -m unittest discover -s "priv/backoffice/swagger"

The generated model in Python:
`
def init(self, type=None, thumb=None, strength=None, status=None, start_date=None, product_id=None, name=None, id=None, expires=None, description=None, category_id=None, auto_renew=None):
"""
AdShowResponse - a model defined in Swagger
"""

    self._type = None
    self._thumb = None
    self._strength = None
    self._status = None
    self._start_date = None
    self._product_id = None
    self._name = None
    self._id = None
    self._expires = None
    self._description = None
    self._category_id = None
    self._auto_renew = None

    if type is not None:
      self.type = type
    if thumb is not None:
      self.thumb = thumb
    if strength is not None:
      self.strength = strength
    if status is not None:
      self.status = status
    if start_date is not None:
      self.start_date = start_date
    if product_id is not None:
      self.product_id = product_id
    self.name = name
    if id is not None:
      self.id = id
    if expires is not None:
      self.expires = expires
    if description is not None:
      self.description = description
    self.category_id = category_id
    if auto_renew is not None:
      self.auto_renew = auto_renew

`
The generated model in PHP:

/** * Constructor * @param mixed[] $data Associated array of property values initializing the model */ public function __construct(array $data = null) { $this->container['type'] = isset($data['type']) ? $data['type'] : null; $this->container['thumb'] = isset($data['thumb']) ? $data['thumb'] : null; $this->container['strength'] = isset($data['strength']) ? $data['strength'] : null; $this->container['status'] = isset($data['status']) ? $data['status'] : null; $this->container['start_date'] = isset($data['start_date']) ? $data['start_date'] : null; $this->container['product_id'] = isset($data['product_id']) ? $data['product_id'] : null; $this->container['name'] = isset($data['name']) ? $data['name'] : null; $this->container['id'] = isset($data['id']) ? $data['id'] : null; $this->container['expires'] = isset($data['expires']) ? $data['expires'] : null; $this->container['description'] = isset($data['description']) ? $data['description'] : null; $this->container['category_id'] = isset($data['category_id']) ? $data['category_id'] : null; $this->container['auto_renew'] = isset($data['auto_renew']) ? $data['auto_renew'] : null; }

it looks like theres a discrepancy on how required parameters are handled which is causing weird behaviour. I have indeed checked whether the server responds with the correct data (and it does).

{'category_id': 1, 'description': 'none', 'id': 623, 'name': 'my ad for fucks sake', 'product_id': 1, 'type': 'Image watermark'}

@InoMurko InoMurko changed the title [python] [python] required fields Aug 11, 2017
@wing328
Copy link
Contributor

wing328 commented Aug 12, 2017

File "/opt/priv/backoffice/swagger/test/test_ad_api.py", line 99, in test_backoffice_api_merchant_ad_controller_show
api_response = api_instance.backoffice_api_merchant_ad_controller_show(merchant_id=5, authorization=auth, ad_id=id)

Am I correct in saying that the auto-generated test case (without any modification) result in the error? There were reports that the auto-generated test cases do not work perfectly so we are considering to address the issue by commenting out the test cases

If you actually use the Python API client, does it work? (I think it should)

@InoMurko
Copy link
Author

Apis work fine. The generated models don't.

@brucesears
Copy link

brucesears commented Nov 14, 2017

I am trying to use the python modules auto generated by swagger, and I am running into an issue I think is a bug in ApiClient.deserialize()

`
def deserialize(self, response, response_type):
"""
Deserializes response into an object.

    :param response: RESTResponse object to be deserialized.
    :param response_type: class literal for
        deserialized object, or string of class name.

    :return: deserialized object.
    """
    # handle file downloading
    # save response body into a tmp file and return the instance
    if response_type == "file":
        return self.__deserialize_file(response)

    # fetch data from response object
    try:
        data = json.loads(response.data)
    except ValueError:
        data = response.data

    return self.__deserialize(data, response_type)

`

the problem here seems to be that at least for response_type == list[Host], that the data param ends up looking like this before it is passed into self.__deserialize(...)

{ 'data': [ { <various host attributes and values > }, { < various host attributes and values > }, ... ] }

but it appears that what self.__deserialize(...) is expecting to be passed in its 'data' param should just be this:

[ { <various host attributes and values > }, { < various host attributes and values > }, ... ]

(where Host is one of the model data types.)

my terrifically ugly "fix" may better define the problem:

`class _VsphereClient(ApiClient):
# overrides ApiClient.deserialize()
def deserialize(self, response, response_type):

    # handle file downloading
    # save response body into a tmp file and return the instance
    if response_type == "file":
        return self.__deserialize_file(response)

    # (btw, this looks scary.  why the vaguery about what response.data is supposed to be?)
    try:
        data = json.loads(response.data)
    except ValueError:
        data = response.data

    # here is the fix
    if 'data' in data:
        data = data['data']

    # and this is __extremely__ ugly.  here we are forcing a call to the
    # superclass' private __deserialize() method.  terrible.  terrible.
    return super()._ApiClient__deserialize(data, response_type)

`

i also don't know what evil side-effects lurk in my hacky subclass fix.

@brucesears
Copy link

Well my fix then still fails when the model class, say Host, in list[Host], itself has attributes of further depth, say Host attribute has something like 'nics' = list[NIC], then my fix fails to read the deeper content in the json data structure to create the list of NIC objects...

I think to fix, i'll need to subclass ApiClient (as I do above), and then override both deserialize() and __deserialize(). (okay, the latter is not really an override, but just a new private method in the _VsphereClient subclass of ApiClient. point is, super()._ApiClient__deserialize(...) will not be accessed anymore (which is a good thing anyways...)

@brucesears
Copy link

i may be incorrect about that last post. i now see embedded lists of specs properly being converted into the list of model objects as they should be. apologies for that not-really-an-issue. the original issue is still there, though.

@wing328
Copy link
Contributor

wing328 commented Nov 30, 2017

@brucesears are you using the latest master (2.3.0), which has many enhancements and bug fixes?

@brucesears
Copy link

yikes. i see User-Agent: Swagger-Codegen/1.0.0

i assume that is a problem then. but i just built this code-generation facility recently, and I'm not sure how i would have ended up with such an out of date version.

do you have a recommended set of pre-reqs and instructions on how to go about building an up to date swagger python code generation mechanism? where should I look for better instructions on this matter? thanks.

@brucesears
Copy link

nevermind. found the page here...

@brucesears
Copy link

okay, so the header i find that the swagger client posts: User-Agent: Swagger-Codegen/1.0.0 is either not affiliated with the version of the python code generator (which appears to be the latest stable version 2.2.3), or has another meaning entirely (confusing name that, then.) would be nice if the swagger-codegen version appeared at the top of all such generated code...

at any rate, i've fallen back to using the _preload_content = False parameter for reads in general, and poking a hole through to directly access ApiClient.__deserialize(...), as yucky as that is. my problems seem to stem around what ApiClient.deserialize(...) does with single model object specs vs lists of such.

to be honest, i don't know if i am running into problems in ApiClient around deserialization or if there are oddities in the API definition. i'd say i'm 80% sure it is the former, but some or all could be issues in the latter.

@wing328
Copy link
Contributor

wing328 commented Dec 11, 2017

okay, so the header i find that the swagger client posts: User-Agent: Swagger-Codegen/1.0.0 is either not affiliated with the version of the python code generator (which appears to be the latest stable version 2.2.3), or has another meaning entirely (confusing name that, then.) would be nice if the swagger-codegen version appeared at the top of all such generated code...

1.0.0 refers to the version of the Python API client, not Swagger Codegen.

You can find the Swagger Codegen version in the auto-generated .swagger-codegen/VERSION

@brucesears
Copy link

.swagger-codegen/VERSION contains:

2.3.0-SNAPSHOT

@brucesears
Copy link

now that we have established that i am using the latest, the primary issue i am hitting is that ApiClient.deserialize() seems to correctly unwrap the response data for 'list[model_class]' response types, but not for singular model_class response types. my "fix" has me using [_preload_content = False] and then determining and submitting the 'data' parameter (and type) directly to ApiClient.__deserialize() (via super()._ApiClient__deserialize() called on a subclass of ApiClient. and yes, i understand this is an awful hack to access a private method, but it is there to try to write a wrapper which can statically sit atop arbitrary regenerations of the base client code. i.e., my custom code does not get put into the auto-gen'd client, but harnesses it with a workaround of the issues i'm either encountering or inadvertently creating by my misuse. at any rate, it can and does work. at least until ApiClient.__deserialize() changes, which is specifically marked as not a public API... :-}

in general summary, these issues could be my misuse, some problem with the API def'n, or an issue in the autogenerated code. at this point, i'd say the odds were, in order, 20% / 25% / 55% with the latter 2 being quite speculative as i don't really know how badly the API spec can muck up the autogen code without itself being flagged for issues...

@wing328
Copy link
Contributor

wing328 commented Dec 13, 2017

ApiClient.deserialize() seems to correctly unwrap the response data for 'list[model_class]' response types, but not for singular model_class response types

Yup, it could be a bug.

Can you please verify by adding test case for tags and see if the Python petstore integration tests pass?

Pet contains an array of Tag (also a model): https://github.com/swagger-api/swagger-codegen/blob/master/modules/swagger-codegen/src/test/resources/2_0/petstore.yaml#L677

@wing328 wing328 modified the milestones: v2.3.0, v2.4.0 Dec 18, 2017
@ewenger
Copy link

ewenger commented Apr 29, 2019

I just got burned by this issue as well.

@spacether
Copy link

spacether commented Nov 14, 2019

@InoMurko if using another python client generator is an option, required values are in the python-experimental generator.

Spec definition of Pet with required properties

For example the Pet class has the required parameters name, and photo_urls per:
https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator/src/test/resources/2_0/petstore-with-fake-endpoints-models-for-testing.yaml#L1224
Because the required arguments are positional, they do not have None defaults.
Python lets us pass positional arguments as keyword arguments so we can do either:

p1 = Pet(name='first last', photo_urls=['http://www.blah.com/hi.png'])
# or
p1 = Pet('first last', ['http://www.blah.com/hi.png'])

Model generated with required properties

Model definition requires name and photo_urls, optional arguments are **kwargs
https://github.com/OpenAPITools/openapi-generator/blob/master/samples/client/petstore/python-experimental/petstore_api/models/pet.py#L102

Deserialization Verification Test

We have a deserialization example of it here: https://github.com/OpenAPITools/openapi-generator/blob/master/samples/client/petstore/python-experimental/tests/test_deserialization.py#L101

@spacether
Copy link

spacether commented Nov 15, 2019

@brucesears It looks like your response back is including the data key which you are not accounting for in your spec. Can you try editing your spec to be something like

"get": {
        "tags": [
          "Ad"
        ],
        "summary": "",
        "responses": {
          "404": {
            "description": "Not Found"
          },
          "200": {
            "schema": {
              "$ref": "#/definitions/DataResponse"
            },
            "description": "OK"
          }
        },

"DataResponse": {
      "type": "object",
      "required": [
        "data"
      ],
    properties:
      data:
        type: array
        items:
          $ref: '#/definitions/Host'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants