microsoft / kiota-serialization-json-python

JSON serialization implementation for Kiota clients in Python
https://aka.ms/kiota/docs
MIT License
4 stars 12 forks source link

Critical bug: Python Float Parsing #331

Closed pjmagee closed 4 months ago

pjmagee commented 4 months ago

What are you generating using Kiota, clients or plugins?

API Client/SDK

In what context or format are you using Kiota?

Nuget tool

Client library/SDK language

Python

Describe the bug

 def get_float_value(self) -> Optional[float]:
        """Gets the float value of the json node
        Returns:
            float: The integer value of the node
        """
        return self._json_node if isinstance(self._json_node, float) else None

When a response from an API, says in the OpenAPI Spec it can be a non whole number (float, number), the Python is only returning a numeric value, when the value itself is a literal float, if the number seems to be whole number, it is returning None.

Expected behavior

Whole numbers should also be taken into consideration, this seems like quite a big problem! Let's take prices as an example, £3.20 as a float is valid, but £3 is also valid. However, in situations where whole numbers (integers) are returned, the float is not parsed. I would expect float(3) to be returned?

Something like this maybe?

if isinstance(self._json_node, (float, int)):
        return float(self._json_node)

Type "help", "copyright", "credits" or "license" for more information.

isinstance(5, float) False isinstance(5.0, float) True

An OpenAPI Specification can say that an API contains numbers other than whole numbers, but here Python needs to ensure the number is maintained and not lost into a 'None'

How to reproduce

I think it's happening all the time, based on the functionality implemented for get_float_value

Open API description file

Partial OpenAPI description:

attributes:
          type: object
          properties:
            value:
              type: number
              format: double
              example: 27107.99
              description: 'The value of demand for a specified period, ranging from `periodStartDate` to `periodEndDate`.'
            versionDate:
              type: string
              format: date-time
              minLength: 24
              maxLength: 24
              example: '2023-11-14T21:33:38.000Z'
              description: 'For forecasted values, this date indicates when the forecast was generated. For actual values, this date represents the time when the data was collected.'

Kiota Version

1.16.0

Latest Kiota version known to work for scenario above?(Not required)

No response

Known Workarounds

No response

Configuration

No response

Debug output

Click to expand log ``` ```

Other information

No response

pjmagee commented 4 months ago

temporary fix:

"value": lambda n : setattr(self, 'value', float(n._json_node),
baywet commented 4 months ago

Hi @pjmagee Thanks for using kiota and for reporting this. Assuming we apply the fix you've suggested, when it's an int, should we also do float(int_source) so consumers can properly leverage type testing? Also I'll be transferring this to the serialization library where the implementation bug is.

pjmagee commented 4 months ago

Hi @pjmagee Thanks for using kiota and for reporting this. Assuming we apply the fix you've suggested, when it's an int, should we also do float(int_source) so consumers can properly leverage type testing? Also I'll be transferring this to the serialization library where the implementation bug is.

I'm not sure to be honest! I am trying to get some 'dog fooding' of our APIs happening while also using Code generation to speed things up, One of our analysts found this issue and I helped debug and fix it so they could continue to ingest data from the API with Kiota (and not go an alternative route). However your point about the type testing, I'm really not sure!

I don't think we need type testing personally, when we develop an API and it's a JSON API, we would do tests on ther JSON implementation, I dont want to test via Kiota, i want Kiota purely for code generation and to trust it will handle situations properly.

baywet commented 4 months ago

ok, thanks for the additional input. I think it'd make sense that a method with float in the name returns a float. Are you willing to submit a pull request to address the issue?

pjmagee commented 4 months ago

ok, thanks for the additional input. I think it'd make sense that a method with float in the name returns a float. Are you willing to submit a pull request to address the issue?

If it's as simple as what I had suggested, I think I can raise the pull request for this. Unsure on how best to test everything from a local environment. I'm not a Python expert or Python user really.