thingsboard-python-rest-client icon indicating copy to clipboard operation
thingsboard-python-rest-client copied to clipboard

constructors of classes derived from VersionCreateRequest and VersionLoadRequest broken

Open timo opened this issue 1 year ago • 4 comments

The commit 80b249b introduced this change to the end of the constructor of SingleEntityVersionCreateRequest and ComplexVersionCreateRequest:

-        VersionCreateRequest.__init__(self, branch, type, version_name)
+        VersionCreateRequest.__init__(self, *args, **kwargs)

This is at the end of the constructors of these classes. The first thing the superclass constructor does here is to set _branch, _type, and _version_name to None, and since these are not in args or in kwargs, they don't get set to the values they are meant to be (this is what the subclass constructor did just a few lines earlier).

The same problem is also in the VersionLoadRequest superclass and its derived classes.

timo avatar Mar 04 '24 17:03 timo

This is quite possibly a problem in the swagger definition that spring exports for thingsboard. I have not seen anything that explicitly speaks against it, but this doesn't seem correct:

      "ComplexVersionCreateRequest": {
        "title": "ComplexVersionCreateRequest",
        "properties": {
          "branch": {...},
          "entityTypes": {...},
          "syncStrategy": {...},
          "type": {...},
          "versionName": {...}
        }, 
        "allOf": [
          {
            "$ref": "#/components/schemas/VersionCreateRequest"
          },...],
... }, ...
      "VersionCreateRequest": {
        "title": "VersionCreateRequest",
        "type": "object",
        "properties": {
          "branch": {...},
          "type": {...},
          "versionName": {...}
        }},

Is it okay to have the branch, type, and versionName properties duplicated in the parent and child classes? At least the swagger api spec doesn't have it like that in their examples.

timo avatar Mar 04 '24 17:03 timo

a random find, SmsTwoFaAccountConfig also has this problem with the use_by_default parameter, which is probably much harder to actually notice is not working

timo avatar Mar 04 '24 18:03 timo

AlarmNotificationRuleTriggerConfig as well. It's probably a good idea to just go through every mention of "allOf" in the swagger definition json and check the constructors for this pattern

timo avatar Mar 04 '24 18:03 timo

Perhaps you are using an older version of https://github.com/swagger-api/swagger-codegen/pull/11968 which at least now passes all the arguments to the superclass constructor and that should fix this issue

timo avatar Mar 04 '24 18:03 timo