Byte length not calculated correctly in python

[ugnom]

Copying from original issue in swagger-codegen issue 6582
swagger-api/swagger-codegen#6582

Description

In our swagger declaration, we defined a property of type string and format bytes. We also specified that it should be between 15 and 16 bytes in length, like "AAAAAAAAAAAAAAAAAAAAAA==". When using our service as a REST API and sending it through our swagger page, strings like the example before work correctly. However, when trying to use the python client, and send in the string "AAAAAAAAAAAAAAAAAAAAAA==", it checks the string character length (24), instead of the byte length (16). If we try to send in something other than a string, it fails the regular expression check. Basically, the python generated code is incorrectly checking the length of bytes by looking at string length.

Swagger-codegen version

2.2.3

Swagger declaration file content or url

lte_subscription:
    type: object
    required:
      - auth_key
    properties:
      auth_key:
        type: string
        format: byte
        x-nullable: true
        minLength: 15
        maxLength: 16
        example: "AAAAAAAAAAAAAAAAAAAAAA=="

Command line used for generation

Generated into python

Steps to reproduce

if you call

import swagger_client
lte = swagger_client.LteSubscription(auth_key="AAAAAAAAAAAAAAAAAAAAAA==")

Related issues/PRs

Checked, and couldn't find anything

Suggest a fix/enhancement

in the generated code, it currently is:

@auth_key.setter
    def auth_key(self, auth_key):
        """
        Sets the auth_key of this LteSubscription.

        :param auth_key: The auth_key of this LteSubscription.
        :type: str
        """
        if auth_key is None:
            raise ValueError("Invalid value for `auth_key`, must not be `None`")
        if auth_key is not None and len(auth_key) > 16:
            raise ValueError("Invalid value for `auth_key`, length must be less than or equal to `16`")
        if auth_key is not None and len(auth_key) < 15:
            raise ValueError("Invalid value for `auth_key`, length must be greater than or equal to `15`")
        if auth_key is not None and not re.search('^(?:[A-Za-z0-9+\/]{4})*(?:[A-Za-z0-9+\/]{2}==|[A-Za-z0-9+\/]{3}=)?$', auth_key):
            raise ValueError("Invalid value for `auth_key`, must be a follow pattern or equal to `/^(?:[A-Za-z0-9+\/]{4})*(?:[A-Za-z0-9+\/]{2}==|[A-Za-z0-9+\/]{3}=)?$/`")

        self._auth_key = auth_key

but it should be this. Considering it requires a base64 encoded string, it should check the length of the base 64 decoding.

import base64

@auth_key.setter
    def auth_key(self, auth_key):
        """
        Sets the auth_key of this LteSubscription.

        :param auth_key: The auth_key of this LteSubscription.
        :type: str
        """
        param_length = len(base64.b64decode(auth_key))
        if auth_key is None:
            raise ValueError("Invalid value for `auth_key`, must not be `None`")
        if auth_key is not None and len(param_length) > 16:
            raise ValueError("Invalid value for `auth_key`, byte length must be less than or equal to `16`")
        if auth_key is not None and len(param_length) < 15:
            raise ValueError("Invalid value for `auth_key`, byte length must be greater than or equal to `15`")
        if auth_key is not None and not re.search('^(?:[A-Za-z0-9+\/]{4})*(?:[A-Za-z0-9+\/]{2}==|[A-Za-z0-9+\/]{3}=)?$', auth_key):
            raise ValueError("Invalid value for `auth_key`, must be a follow pattern or equal to `/^(?:[A-Za-z0-9+\/]{4})*(?:[A-Za-z0-9+\/]{2}==|[A-Za-z0-9+\/]{3}=)?$/`")

        self._auth_key = auth_key

[ugnom]

@wing328
I took a look at the code.
What I am planning to change is to add a new boolean variable as isBase64 and put true when type == string and format == byte.
Then in the mustache template adding isBase64 condition to add "import base64" and using base64 decoded length.

If it is valid enough I will start the change.

[wing328]

@ugnom I'll look into your suggestion and get back to you by tonight.

[ugnom]

@wing328
Looks like there is already a condition for this as "isByteArray" which gets true when type is string and format is byte. I guess I can reuse this condition.

It is a bit harder for me to figure out putting "import base64" part dynamically at the top of the file, as "isByteArray" is in each model so that I can't be really sure how I can see if any one of model in the file needs base64. I'll take a bit closer look for that. I appreciate if you have comment on this part.

[wing328]

You can define the import mapping at https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/PythonClientCodegen.java#L73, e.g.

        importMapping.put("ByteArray", "base64");

Then you can use it in the mustache templates, e.g.

openapi-generator/modules/openapi-generator/src/main/resources/kotlin-server/model.mustache

Lines 4 to 5 in b594262

	{{#imports}}import {{import}}
	{{/imports}}

Please give it a try and let me know if you have any question or encounter any issue with the method above.

[ugnom]

Thank you, I could figure out the import part too.
but got another concern here that :

in Python, base64 is usually only take bytes type. So I'm thinking ByteArray type should be converted to bytes type in Python (it's currently done as a str type)
But if I do so, it will be a spec change for the user of the generated code too. that they have to put bytes type for the parameter of the model, while currently it is str type they are to put.

Can you give me an advice for how I should proceed?

1. change the ByteArray as bytes type for python
2. keep ByteArray as str and make the sufficient validation steps in the generated model code (I think this one is what is expected)

If we do number 2, I have to put the base64 imported against str type, so every other model which has str type (there are some other swagger types converted to str in python). That make the generated code a bit less nicer. Is it acceptable?

I appreciate if you can give some suggestions for which way to go.

[wing328]

I prefer the following:

1. change the ByteArray as bytes type for python

The user can use --type-mapping to restore the mapping of ByteArray back to str if they need to:

       --type-mappings <type mappings>
            sets mappings between OpenAPI spec types and generated code types in
            the format of OpenaAPIType=generatedType,OpenAPIType=generatedType.
            For example: array=List,map=Map,string=String. You can also have
            multiple occurrences of this option.

Also mapping ByteArray to str is just a workaround so it needs to fix to map to a correct type in Python.

[ugnom]

OK, I'll take that direction and change the ByteArray to bytes type.

I also taking a further look for the --type-mapping option. I guess I have to change a bit to take care of the situation when the ByteArray taken as str but still needs to count length as base64.

[wing328]

We'll mark it as a breaking change so don't worry too much about backward compatibility.

If the users insist on using "str" for byte array, they can stick with the latest stable version before this enhancement.

[ugnom]

I tried with changing byteArray type as bytes type in Python, but it does not seem to sound well.
Anyway it requires to change the bytes type into string when I try to encode it to json format to put it in a request.

Maybe it should end up with str type after all, from what I see for the python specification around base64, bytes type and json encoding.

I would like to have a comment on this about this. This will be relating to the specification of generator itself so that it is kind of hard for me to decide.

My guts telling me it probably should be str, so that users can take the things less confusing.

[yucer]

Just want to note that in OpenAPI spec all strings are supposed to be unicode, and that is not "bytes".

At least python2 maxLen should fail for strings if unicode characters are returned from the server. They need more characters in the raw (byte) representation.