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

Multiple use of allOf not working in python #6796

Open
6 tasks
mwilby opened this issue Jun 27, 2020 · 2 comments
Open
6 tasks

Multiple use of allOf not working in python #6796

mwilby opened this issue Jun 27, 2020 · 2 comments

Comments

@mwilby
Copy link

mwilby commented Jun 27, 2020

Bug Report Checklist

  • Have you provided a full/minimal spec to reproduce the issue?
  • Have you validated the input using an OpenAPI validator (example)?
  • What's the version of OpenAPI Generator used?
  • Have you search for related issues/PRs?
  • What's the actual output vs expected output?
  • [Optional] Bounty to sponsor the fix (example)
Description

The use of allOf more than once, is not working in python. Specifically, models that use an allOf import on a model that also uses allOf are not correctly constructed.

using

address:      # Can be referenced via '#/components/schemas/address'
  type: object
  properties:
    al:
      type: array
      items:
        $ref: 'https://host014.etsit.upm.es/schemas/test/maitred/msg/net_point.yaml#/network_point'
    did:
      type: string
    sid:
      type: integer
      format: int32

to build

header:
  type: object
  allOf:
    - $ref: 'https://host014.etsit.upm.es/schemas/test/maitred/base/address.yaml#/address'
  properties:
    tag:
      type: string
    f:
      type: integer
      format: int32
      enum:
        - -1
        - 0
        - 1
        - 2
        - 3
        - 4
      default: 0
    ts:
      type: integer
      format: int64

The corresponding generator header class results in

class Header(Model):

    def __init__(self, tag=None, f=0, ts=None, al=None, did=None, sid=None):  # noqa: E501

Everything is fine, including the enumerator.

Now, if it is then used to build

message:
  type: object
  allOf:
    - $ref: 'https://host014.etsit.upm.es/schemas/test/maitred/base/message_header.yaml#/header'
  properties:
    pl:
      type: object

The resulting class results is

class Message(Model):

    def __init__(self, al=None, did=None, sid=None, pl=None):  # noqa: E501

Only the address field are included and components native to header have disappeared.

Its not just a simple template mistake in not including the inheritance of the base class. There is a problem in including the properties in the base model in the derived class. Something is going wrong with the generation of the vars and their variations.

Also, if inheritance was the target design, then there is an ambiguity with the imports. The message model has

from maitred.base.models.header import Header
from maitred.base.models.network_point import NetworkPoint

Where the NetworkPoint should not be necessary at this class level.

A rethink in terms of the constructor arguments will also be needed.

Note, the schema generates a model that, so far works fine in Java.

openapi-generator version

4.3.1-SNAPSHOT and 4.3.0

OpenAPI declaration file content or url

The corresponding full model files are published as:

network_point
address
header
message

Command line used for generation

N/A

Steps to reproduce

N/A

Related issues/PRs

N/A

Suggest a fix

The simplest hack would be to modify the template to inherite the parent class and live with the fact that the base properties on the deepest model will be overloaded, but clarifying the var list would be nice.

@mwilby
Copy link
Author

mwilby commented Jun 27, 2020

We had to fix this, it was preventing us merging two projects.

We modified the python-flask model.mustache. We don't use the python template because it does not handle the type's correctly.

The solution we used is the one suggested. Its very ugly, and almost certainly slower than it needs to be, but we needed something that worked. So now the Message class looks like this:

class Message(Header):
    def __init__(self, pl=None, al=None, did=None, sid=None, **oapi_ud_param_list):  # noqa: E501
        super(Message, self).__init__(oapi_ud_param_list=oapi_ud_param_list)

        self.openapi_types['pl'] = object
        self.openapi_types['al'] = List[NetworkPoint]
        self.openapi_types['did'] = str
        self.openapi_types['sid'] = int

        self.attribute_map['pl'] = 'pl'
        self.attribute_map['al'] = 'al'
        self.attribute_map['did'] = 'did'
        self.attribute_map['sid'] = 'sid'

        self._pl = pl
        self._al = al
        self._did = did
        self._sid = sid

The generated code ran inside a fairly complex app with no problems.

We went back and checked the python generator and it has similar problems. But in that case the vars list is correct, i.e. it is only the locally defined properties. So a full fix is possible just with the template.

I have posted the template (I think I took out all our vendor specific stuff) at

model.mustache

It could be used as a model for all the server specific generators.

There are additional issues due to the fact that the bundle structure in python is different to that of the server generators. The problem of having 2 branches of the python language definitions, are only going to get worse. The failed attempt to address this in #6065 is now completely defunct, because merging the PythonCodegen with server abstract class is more involved than was addressed, because the bundles generated differ dramatically. Its going to need a convergence mechanism, if you don't want the divergence to needlesly grow.

@mwilby
Copy link
Author

mwilby commented Jul 7, 2020

Because we set the parameters after instantiation, we didn't notice, but the assignment of parameters to the super class wasn't working correctly. We corrected the template, now the variable assignment looks like.

        oapi_ud_param_list = oapi_ud_param_list.get('oapi_ud_param_list')

        if oapi_ud_param_list is None:
            self._al = al
            self._did = did
            self._sid = sid
        else:
            if oapi_ud_param_list.get('al') is None:
                self._al = al
            else:
                self._al = oapi_ud_param_list.get('al')
            if oapi_ud_param_list.get('did') is None:
                self._did = did
            else:
                self._did = oapi_ud_param_list.get('did')
            if oapi_ud_param_list.get('sid') is None:
                self._sid = sid
            else:
                self._sid = oapi_ud_param_list.get('sid')

As I said, its ugly, I am sure there is a better and more efficient way of doing it, but we haent time t play with it.

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

2 participants