microsoft / kiota

OpenAPI based HTTP Client code generator
https://aka.ms/kiota/docs
MIT License
2.87k stars 195 forks source link

[Python] array anyOf generating invalid Python code #4634

Open pjmagee opened 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?

Linux executable

Client library/SDK language

Python

Describe the bug

Invalid python code causes python runtime exceptions to occur due to the generated python not being valid for anyOf.

Expected behavior

Python anyOf to produce python code that doesnt cause the runtime exceptions to occur when trying to call endpoints from the generated client

How to reproduce

    '200':
      description: The HTTP `200 OK` success status response code indicates that the request has succeeded.
      content:
        application/vnd.api+json:
          schema:
            type: object
            description: A list of LocationSpread entities.
            properties:
              links:
                type: object
                properties:
                  next:
                    type: string
                    nullable: true
              data:
                type: array
                items:
                  $ref: '#/components/schemas/LocationSpreadModel'
              included:
                type: array
                items:
                  anyOf:
                    - $ref: '#/components/schemas/PeriodModel'
                    - $ref: '#/components/schemas/LocationSpreadSpecificationModel'

Open API description file

energyapi.json

Kiota Version

1.14.0

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

No response

Known Workarounds

No response

Configuration

No response

Debug output

PS C:\Projects\icis-openapi-kiota> .\generate-kiota-api-clients.ps1 Generating for https://developer.icis.com/portals/api/sites/icis-live-portal/liveportal/apis/energyapi/download_spec info: Kiota.Builder.KiotaBuilder[0] Cleaning output directory /app/output/Python/energy dbug: Kiota.Builder.KiotaBuilder[0] kiota version 1.14.0 dbug: Kiota.Builder.KiotaBuilder[0] cache file /tmp/kiota/cache/generation/BA16C3B0119BBFE2AA3E7CA599D573E64E2A6E1D154015CF57C24062ED34671C/download_spec not found, downloading from https://developer.icis.com/portals/api/sites/icis-live-portal/liveportal/apis/energyapi/download_spec info: Kiota.Builder.KiotaBuilder[0] loaded description from remote source dbug: Kiota.Builder.KiotaBuilder[0] step 1 - reading the stream - took 00:00:01.0235017 warn: Kiota.Builder.KiotaBuilder[0] OpenAPI warning: #/paths/~1basisPriceAssessments/get/responses/200/content/application~1vnd.api+json/example/data/0/attributes/createdForDate - Data and type mismatch found. warn: Kiota.Builder.KiotaBuilder[0] OpenAPI warning: #/paths/~1basisPriceAssessments/get/responses/200/content/application~1vnd.api+json/example/data/0/attributes/volatilityIndex - Data and type mismatch found. warn: Kiota.Builder.KiotaBuilder[0] OpenAPI warning: #/paths/~1basisPriceAssessments/get/responses/200/content/application~1vnd.api+json/example/data/0/attributes/diffToPrevBasisMid - Data and type mismatch found. warn: Kiota.Builder.KiotaBuilder[0] OpenAPI warning: #/paths/~1basisPriceAssessments/get/responses/200/content/application~1vnd.api+json/example/data/1/attributes/createdForDate - Data and type mismatch found. warn: Kiota.Builder.KiotaBuilder[0] OpenAPI warning: #/paths/~1basisPriceAssessments/get/responses/200/content/application~1vnd.api+json/example/data/1/attributes/volatilityIndex - Data and type mismatch found. warn: Kiota.Builder.KiotaBuilder[0] OpenAPI warning: #/paths/~1basisPriceAssessments~1{id}/get/responses/200/content/application~1vnd.api+json/example/data/attributes/createdForDate - Data and type mismatch found. warn: Kiota.Builder.KiotaBuilder[0] OpenAPI warning: #/paths/~1basisPriceAssessments~1{id}/get/responses/200/content/application~1vnd.api+json/example/data/attributes/volatilityIndex - Data and type mismatch found. warn: Kiota.Builder.KiotaBuilder[0] OpenAPI warning: #/paths/~1basisPriceAssessments~1{id}/get/responses/200/content/application~1vnd.api+json/example/data/attributes/diffToPrevBasisMid - Data and type mismatch found. warn: Kiota.Builder.KiotaBuilder[0] OpenAPI warning: #/paths/~1dataEvents/get/responses/200/content/application~1vnd.api+json/example/data/0/attributes/createdForDate - Data and type mismatch found. warn: Kiota.Builder.KiotaBuilder[0] OpenAPI warning: #/paths/~1dataEvents/get/responses/200/content/application~1vnd.api+json/example/data/1/attributes/createdForDate - Data and type mismatch found. warn: Kiota.Builder.KiotaBuilder[0] OpenAPI warning: #/paths/~1indices/get/responses/200/content/application~1vnd.api+json/example/data/0/attributes/createdForDate - Data and type mismatch found. warn: Kiota.Builder.KiotaBuilder[0] OpenAPI warning: #/paths/~1indices/get/responses/200/content/application~1vnd.api+json/example/data/0/attributes/volume - Data and type mismatch found. warn: Kiota.Builder.KiotaBuilder[0] OpenAPI warning: #/paths/~1indices/get/responses/200/content/application~1vnd.api+json/example/data/0/attributes/numOfTrades - Data and type mismatch found. warn: Kiota.Builder.KiotaBuilder[0] OpenAPI warning: #/paths/~1indices/get/responses/200/content/application~1vnd.api+json/example/data/1/attributes/createdForDate - Data and type mismatch found. warn: Kiota.Builder.KiotaBuilder[0] OpenAPI warning: #/paths/~1indices/get/responses/200/content/application~1vnd.api+json/example/data/1/attributes/volume - Data and type mismatch found. warn: Kiota.Builder.KiotaBuilder[0] OpenAPI warning: #/paths/~1indices/get/responses/200/content/application~1vnd.api+json/example/data/1/attributes/numOfTrades - Data and type mismatch found. warn: Kiota.Builder.KiotaBuilder[0] OpenAPI warning: #/paths/~1indices~1{id}/get/responses/200/content/application~1vnd.api+json/example/data/attributes/createdForDate - Data and type mismatch found. warn: Kiota.Builder.KiotaBuilder[0] OpenAPI warning: #/paths/~1indices~1{id}/get/responses/200/content/application~1vnd.api+json/example/data/attributes/volume - Data and type mismatch found. warn: Kiota.Builder.KiotaBuilder[0] OpenAPI warning: #/paths/~1indices~1{id}/get/responses/200/content/application~1vnd.api+json/example/data/attributes/numOfTrades - Data and type mismatch found. warn: Kiota.Builder.KiotaBuilder[0] OpenAPI warning: #/paths/~1locationSpreads/get/responses/200/content/application~1vnd.api+json/example/data/0/attributes/createdForDate - Data and type mismatch found. warn: Kiota.Builder.KiotaBuilder[0] OpenAPI warning: #/paths/~1locationSpreads/get/responses/200/content/application~1vnd.api+json/example/data/1/attributes/createdForDate - Data and type mismatch found. warn: Kiota.Builder.KiotaBuilder[0] OpenAPI warning: #/paths/~1locationSpreads~1{id}/get/responses/200/content/application~1vnd.api+json/example/data/attributes/createdForDate - Data and type mismatch found. warn: Kiota.Builder.KiotaBuilder[0] OpenAPI warning: #/paths/~1periods/get/responses/200/content/application~1vnd.api+json/example/data/0/attributes/referenceDate - Data and type mismatch found. warn: Kiota.Builder.KiotaBuilder[0] OpenAPI warning: #/paths/~1periods/get/responses/200/content/application~1vnd.api+json/example/data/0/attributes/startDate - Data and type mismatch found. warn: Kiota.Builder.KiotaBuilder[0] OpenAPI warning: #/paths/~1periods/get/responses/200/content/application~1vnd.api+json/example/data/0/attributes/endDate - Data and type mismatch found. warn: Kiota.Builder.KiotaBuilder[0] OpenAPI warning: #/paths/~1periods/get/responses/200/content/application~1vnd.api+json/example/data/1/attributes/referenceDate - Data and type mismatch found. warn: Kiota.Builder.KiotaBuilder[0] OpenAPI warning: #/paths/~1periods/get/responses/200/content/application~1vnd.api+json/example/data/1/attributes/startDate - Data and type mismatch found. warn: Kiota.Builder.KiotaBuilder[0] OpenAPI warning: #/paths/~1periods/get/responses/200/content/application~1vnd.api+json/example/data/1/attributes/endDate - Data and type mismatch found. warn: Kiota.Builder.KiotaBuilder[0] OpenAPI warning: #/paths/~1periods~1{id}/get/responses/200/content/application~1vnd.api+json/example/data/attributes/referenceDate - Data and type mismatch found. warn: Kiota.Builder.KiotaBuilder[0] OpenAPI warning: #/paths/~1periods~1{id}/get/responses/200/content/application~1vnd.api+json/example/data/attributes/startDate - Data and type mismatch found. warn: Kiota.Builder.KiotaBuilder[0] OpenAPI warning: #/paths/~1periods~1{id}/get/responses/200/content/application~1vnd.api+json/example/data/attributes/endDate - Data and type mismatch found. warn: Kiota.Builder.KiotaBuilder[0] OpenAPI warning: #/paths/~1priceAssessments/get/responses/200/content/application~1vnd.api+json/example/data/0/attributes/createdForDate - Data and type mismatch found. warn: Kiota.Builder.KiotaBuilder[0] OpenAPI warning: #/paths/~1priceAssessments/get/responses/200/content/application~1vnd.api+json/example/data/0/attributes/volatilityIndex - Data and type mismatch found. warn: Kiota.Builder.KiotaBuilder[0] OpenAPI warning: #/paths/~1priceAssessments/get/responses/200/content/application~1vnd.api+json/example/data/1/attributes/createdForDate - Data and type mismatch found. warn: Kiota.Builder.KiotaBuilder[0] OpenAPI warning: #/paths/~1priceAssessments/get/responses/200/content/application~1vnd.api+json/example/data/1/attributes/volatilityIndex - Data and type mismatch found. warn: Kiota.Builder.KiotaBuilder[0] OpenAPI warning: #/paths/~1priceAssessments~1{id}/get/responses/200/content/application~1vnd.api+json/example/data/attributes/createdForDate - Data and type mismatch found. warn: Kiota.Builder.KiotaBuilder[0] OpenAPI warning: #/paths/~1priceAssessments~1{id}/get/responses/200/content/application~1vnd.api+json/example/data/attributes/volatilityIndex - Data and type mismatch found. warn: Kiota.Builder.KiotaBuilder[0] OpenAPI warning: #/paths/~1sparkAndDarkSpreads/get/responses/200/content/application~1vnd.api+json/example/data/0/attributes/createdForDate - Data and type mismatch found. warn: Kiota.Builder.KiotaBuilder[0] OpenAPI warning: #/paths/~1sparkAndDarkSpreads/get/responses/200/content/application~1vnd.api+json/example/data/1/attributes/createdForDate - Data and type mismatch found. warn: Kiota.Builder.KiotaBuilder[0] OpenAPI warning: #/paths/~1sparkAndDarkSpreads~1{id}/get/responses/200/content/application~1vnd.api+json/example/data/attributes/createdForDate - Data and type mismatch found. warn: Kiota.Builder.KiotaBuilder[0] OpenAPI warning: #/paths/~1trades/get/responses/200/content/application~1vnd.api+json/example/data/0/attributes/createdForDate - Data and type mismatch found. warn: Kiota.Builder.KiotaBuilder[0] OpenAPI warning: #/paths/~1trades/get/responses/200/content/application~1vnd.api+json/example/data/0/attributes/volume - Data and type mismatch found. warn: Kiota.Builder.KiotaBuilder[0] OpenAPI warning: #/paths/~1trades/get/responses/200/content/application~1vnd.api+json/example/data/0/attributes/totalVolume - Data and type mismatch found. warn: Kiota.Builder.KiotaBuilder[0] OpenAPI warning: #/paths/~1trades/get/responses/200/content/application~1vnd.api+json/example/data/1/attributes/createdForDate - Data and type mismatch found. warn: Kiota.Builder.KiotaBuilder[0] OpenAPI warning: #/paths/~1trades/get/responses/200/content/application~1vnd.api+json/example/data/1/attributes/volume - Data and type mismatch found. warn: Kiota.Builder.KiotaBuilder[0] OpenAPI warning: #/paths/~1trades/get/responses/200/content/application~1vnd.api+json/example/data/1/attributes/totalVolume - Data and type mismatch found. warn: Kiota.Builder.KiotaBuilder[0] OpenAPI warning: #/paths/~1trades~1{id}/get/responses/200/content/application~1vnd.api+json/example/data/attributes/createdForDate - Data and type mismatch found. warn: Kiota.Builder.KiotaBuilder[0] OpenAPI warning: #/paths/~1trades~1{id}/get/responses/200/content/application~1vnd.api+json/example/data/attributes/volume - Data and type mismatch found. warn: Kiota.Builder.KiotaBuilder[0] OpenAPI warning: #/paths/~1trades~1{id}/get/responses/200/content/application~1vnd.api+json/example/data/attributes/totalVolume - Data and type mismatch found. warn: Kiota.Builder.KiotaBuilder[0] OpenAPI warning: #/components/schemas/IndexModel/properties/attributes/properties/numOfTrades - The format integer is not supported by Kiota for the type number and the string type will be used. dbug: Kiota.Builder.KiotaBuilder[0] step 2 - parsing the document - took 00:00:00.1286369 dbug: Kiota.Builder.KiotaBuilder[0] step 3 - updating generation configuration from kiota extension - took 00:00:00.0001125 dbug: Kiota.Builder.KiotaBuilder[0] step 4 - filtering API paths with patterns - took 00:00:00.0026473 dbug: Kiota.Builder.KiotaBuilder[0] step 5 - checking whether the output should be updated - took 00:00:00.0193923 dbug: Kiota.Builder.KiotaBuilder[0] step 6 - create uri space - took 00:00:00.0024655 dbug: Kiota.Builder.KiotaBuilder[0] InitializeInheritanceIndex 00:00:00.0031273 dbug: Kiota.Builder.KiotaBuilder[0] CreateRequestBuilderClass 00:00:00 dbug: Kiota.Builder.KiotaBuilder[0] MapTypeDefinitions 00:00:00.0218516 dbug: Kiota.Builder.KiotaBuilder[0] TrimInheritedModels 00:00:00 dbug: Kiota.Builder.KiotaBuilder[0] CleanUpInternalState 00:00:00 dbug: Kiota.Builder.KiotaBuilder[0] step 7 - create source model - took 00:00:00.0900220 dbug: Kiota.Builder.KiotaBuilder[0] 127ms: Language refinement applied dbug: Kiota.Builder.KiotaBuilder[0] step 8 - refine by language - took 00:00:00.1278204 dbug: Kiota.Builder.KiotaBuilder[0] step 9 - writing files - took 00:00:01.0558486 dbug: Kiota.Builder.KiotaBuilder[0] cache file /tmp/kiota/cache/generation/BA16C3B0119BBFE2AA3E7CA599D573E64E2A6E1D154015CF57C24062ED34671C/download_spec is up to date and clearCache is False, using it info: Kiota.Builder.KiotaBuilder[0] loaded description from remote source dbug: Kiota.Builder.KiotaBuilder[0] step 10 - writing lock file - took 00:00:00.0130857 Generation completed successfully dbug: Kiota.Builder.KiotaBuilder[0] Api manifest path: /app/apimanifest.json

Other information

I'm aware of the other errors coming up the output, but I dont think those impact the include with anyOf not working.

image

It DOES generate the classes, however, Python seems to have an issue in recognising the imports and using the class. See line 53, even PyCharm is not sure what class is being referred to, despite the class being generated below. Is it because the class is nested within the response class?

image

baywet commented 4 months ago

Hi @pjmagee Thanks for using kiota and for reaching out. To be clear, is the import path is correct? i.e. is it constructed properly to point to the type to be imported? (no mismatch in structure/casing that you could edit out).

What happens if you create a schema component for the anyof type and refer to that for the items schema? Do you still run into the same issue?

pjmagee commented 4 months ago

I think because its nested, the import is also broken, so the import path is also kind of incorrect. It also doesnt get picked up correctly.

using Github Copilot, it actually mentions something similar...

image

pjmagee commented 4 months ago

I tried to import from the class, but im struggling to change the import statement to getting it to be detected correctly.

pjmagee commented 4 months ago

Removing the import and doing this seems to be "acceptable" according to Python/PyCharm

"included": lambda n : setattr(self, 'included', n.get_collection_of_object_values(PriceAssessmentsGetResponse.PriceAssessmentsGetResponse_included)),

It has to be fixed up in a few places, but Class.Class works, couldnt figure out the right import way to get it working with the nested class.

pjmagee commented 4 months ago

I think this may be part of a larger issue. Looking at the LocationSpreadsGetResponse_included generated class:

It seems to not generate some of the fields. Unsure if its related to the anyOf or due to the anyOf

image

baywet commented 4 months ago

Thanks for the additional information. @shemogumbe do you think this could be the same thing you were referring to in https://github.com/microsoftgraph/msgraph-beta-sdk-python/issues/391 ??

pjmagee commented 4 months ago

I know our spec isn't perfect, so also confirmed its not a spec specific thing and C# looks to be generating okay.. Just example showing intellisense/properties being generated correctly

image

baywet commented 4 months ago

Thanks for the additional information. @shemogumbe to confirm whether this could be the bug he was mentioning. (it's a holiday for him, we'll probably only hear back next week)

shemogumbe commented 4 months ago

Yes, this could be related, classes are generated and nested, the more the nesting, we lose the ability to import the classes

baywet commented 4 months ago

@shemogumbe I can see the other issue has been closed, does that mean this one is solved as well?

pjmagee commented 4 months ago

If we have a new docker build, light me up and I'll give it another go!

shemogumbe commented 4 months ago

Hello, this is not related to the earlier issue, the other issue was about imports not being available because the user was using a v1 client to execute functionality that is only on Beta. The classes could therefore not be imported because of a client and APVersion mismatch.

I am currently working on this, started investigating from generations involving anyof

Using the file shared at energyapi.json :

  1. The structure is invalid josn
  2. converting this to YAML - the file seemed malformed and I can't hus run generation. To fix the generation, I would wish to generate uisng an openapi file that has anyOf and see the generatd output to help get context.

Any more further context you might share to help me reproduce this @pjmagee

pjmagee commented 4 months ago

That's odd? Our EnergyAPI yaml file is valid that I am able to generate various language outputs, including the Python. The original file is yaml, not json.

https://developer.icis.com/portals/api/sites/icis-live-portal/liveportal/apis/energyapi/download_spec

shemogumbe commented 4 months ago

Hello @pjmagee this works and generates the code https://github.com/shemogumbe/openapi/tree/main/openapi/msgraph-energy/python

Kindly confirm what you mean by, Generates Invalid code

The files generated in that directory are

  1. basis_price_assesments_get_response_links.py with class BasisPriceAssessmentsGetResponse_links
  2. basis_price_assessments_request_builder.py with class class BasisPriceAssessmentsRequestBuilder(BaseRequestBuilder): Notably:
    from .indices_get_response_included import IndicesGetResponse_included
    from .indices_get_response_links import IndicesGetResponse_links

    has an unnecessary import from .indices_get_response_included import IndicesGetResponse_included, the module and the class it contains is not generated when it ought to be generated as it is references here,

Does this capture the issue you are facing?

pjmagee commented 4 months ago

image

I have a screenshot above, but here is another one showing the problem. The code generated is invalid, in that Python cannot detect the generated code, because the imports created and the nested classing is invalid python.

pjmagee commented 4 months ago

I think you guys need some kind of acceptance test that runs Python code to confirm the feature is working. Some IDEs are clever and can know this, others will show you the generated code, but it will not work and will give runtime exceptions. The design of nested classing is what I believe to be an issue with the Python code being generated.

shemogumbe commented 4 months ago

I notice the modules suffixed with _inlcuded are not actually generated, but imported and the classe suffixed _cluded used? right?

shemogumbe commented 4 months ago

Hello @pjmagee could you try generating now and update on the status.

I used https://github.com/shemogumbe/openapi/blob/main/openapi/energy_api_sample.yaml for generation

pjmagee commented 4 months ago

Hello, sorry been really busy. Please give me some time to check this. 🙏

pjmagee commented 4 months ago

the latest change, is better for sure, but somehow i think its not usabile still because something seem broken with the fields not being created?

pjmagee commented 4 months ago

The fields don't seem to be created in the Model

pjmagee commented 4 months ago

This is old code, I can see with new kiota version, the class is moved to its own file now

image

image

But actually, the fields is not existing here?

image image

image

self.period_model and self.basis_price_assessment_speciciation_model both not defined on this class as fields

pjmagee commented 4 months ago

image image

And here with intellisense you can see, it know the function like serialise or get_field_deserialisers on this class exist, but I cannot retrieve the properties of the Model because they are not being defined in class.

You can see line 41 of new class created, ith has errors because IDE knows self.basis_price_assesment_specification_model was not defined and same with self.period_model and when you try use class, the fields of the Model is not presentable to the developer either

pjmagee commented 4 months ago

@shemogumbe @baywet

andrueastman commented 3 months ago

Re-opening for @shemogumbe to take a look

shemogumbe commented 3 months ago

Hello @pjmagee If I get you right, two things are happening:

  1. create_from_discriminator_value, serialize and get_field_deserializer methods do not exist
  2. The two class attributes referenced as self.basis_price_assesment_specificaton_model and self.period_model do not exist
pjmagee commented 3 months ago

Hello @pjmagee If I get you right, two things are happening:

  1. create_from_discriminator_value, serialize and get_field_deserializer methods do not exist
  2. The two class attributes referenced as self.basis_price_assesment_specificaton_model and self.period_model do not exist
  1. the kiota methods in bullet point 1, do exist and you can see them in the screenshot intellisense
  2. Correct on bullet point 2, the code is not generating the model attributes from the spec
shemogumbe commented 3 months ago

Working on this

andrueastman commented 3 months ago

@rkodev to look try looking into this while @shemogumbe is OOF.