search

Dorthu / openapi3

A Python3 OpenAPI 3 Spec Parser
BSD 3-Clause "New" or "Revised" License
118 stars 47 forks source link

rebased aiopenapi3 #69

Open commonism opened 2 years ago

commonism commented 2 years ago

Rebased aiopenapi3 for merging. Can't re-open https://github.com/Dorthu/openapi3/pull/62

rafalkrupinski commented 2 years ago

Who will review 7kloc across 100 files with a bunch of unrelated changes... 👎

commonism commented 2 years ago

Hi,

actually it ain't that bad, its a bunch of files with pydantic models for OpenAPI 2/3./3.1 description documents, some logic for request/response processing, the dynamic pydantic class generation being the worst of it.

But I agree, I doubt this will be reviewed/merged - I already forked it https://github.com/commonism/aiopenapi3 .

You are involved with OpenAPI client development as well - https://github.com/python-lapidary/lapidary and lapidary even shares some common requirements with aiopenapi3 - pydantic/httpx, basic difference is lapidary generates pydantic classes - aiopenapi3 is dynamic.

From my experience using pydantic will give you a hard time with additionalProperties (defaulting to True even), as it will allow the data to overwrite the Models methods/properties/attributes, requiring a default of Extra.ignore. I consider additionalProperties:True a specification insanity and not worth supporting, sticking with pydantic. Same with any/all/oneOf - using pydantic it can be supported up to to a certain point. That said, the OAI spec keeps giving, I recently noticed parameters are unique by name&type, so you can have PathParameter with name foo and a QueryParameter with the same name.

Good luck with lapidary.

rafalkrupinski commented 2 years ago

Hi,

actually it ain't that bad, its a bunch of files with pydantic models for OpenAPI 2/3./3.1 description documents, some logic for request/response processing, the dynamic pydantic class generation being the worst of it.

But I agree, I doubt this will be reviewed/merged - I already forked it https://github.com/commonism/aiopenapi3 .

I think it's a good idea, your project outgrew this one.

You are involved with OpenAPI client development as well - https://github.com/python-lapidary/lapidary and lapidary even shares some common requirements with aiopenapi3 - pydantic/httpx, basic difference is lapidary generates pydantic classes - aiopenapi3 is dynamic.

I was actually thinking of making it dynamic, but I wanted generated model and at least method stubs for operations, for code completion. The problem is, that the schema that I want it to work with is 1.2MB and it takes too long to load.

From my experience using pydantic will give you a hard time with additionalProperties (defaulting to True even), as it will allow the data to overwrite the Models methods/properties/attributes, requiring a default of Extra.ignore. I consider additionalProperties:True a specification insanity and not worth supporting, sticking with pydantic.

I think the problem is that many programmers interprets openapi/jsonschema as a recipe for types in their languages (that's my case in the beginning, but I see it in other projects). It' not, it's a validation description, and incidentally, so is pydantic BaseModel (that's at least the intention). That's why I think is a good match, tho not 1-to-1. I think with generated validators it should take me a long a way.

I think not supporting additionalProperties=True is a good decision, as it's impossible to decode such data to a concrete class. Lapidary supports errata (kinda like aiopenapi3 plugins but jsonpatch/yaml) so the user can provide their own schemas. But I think eventually a runtime mechanism might be necessary. Dynamic client might be a better basis for this.

Same with any/all/oneOf - using pydantic it can be supported up to to a certain point.

I found anyOf trivial to implement - simply a typing.Union. Works both ways - encoding and decoding.

That said, the OAI spec keeps giving, I recently noticed parameters are unique by name&type, so you can have PathParameter with name foo and a QueryParameter with the same name.

Unique param names are trivial too, I generate names in Hungarian notation - adding (p/q/c/h)_ prefix, and putting the original name in alias. I might change it to suffix to make it work better with code completion (so that developer starts typing the param name and not the prefix).

Feel free to check out the client generated with Lapidary https://github.com/oeklo/gsmtasks-client (the errata got bigger since the last push)

Thank you for your thoughts. Best Regards

rafalkrupinski commented 2 years ago

And don't forget read- and writeOnly properties :)

commonism commented 2 years ago

You are involved with OpenAPI client development as well - https://github.com/python-lapidary/lapidary and lapidary even shares some common requirements with aiopenapi3 - pydantic/httpx, basic difference is lapidary generates pydantic classes - aiopenapi3 is dynamic.

I was actually thinking of making it dynamic, but I wanted generated model and at least method stubs for operations, for code completion. The problem is, that the schema that I want it to work with is 1.2MB and it takes too long to load.

https://api.gsmtasks.com/docs/schema/ - 4s loading with aiopenapi3, pickle it for re-use. https://github.com/APIs-guru/openapi-directory got documents which take minute(s).

From my experience using pydantic will give you a hard time with additionalProperties (defaulting to True even), as it will allow the data to overwrite the Models methods/properties/attributes, requiring a default of Extra.ignore. I consider additionalProperties:True a specification insanity and not worth supporting, sticking with pydantic.

A different use of additionalProperties - dynamic map. https://swagger.io/docs/specification/data-models/dictionaries/

    type: object
    additionalProperties:
      type: string
    {
      "en": "English",
      "fr": "French"
    }

which is rather helpful.

I think not supporting additionalProperties=True is a good decision, as it's impossible to decode such data to a concrete class.

One could hide the unknowns in .__dict__, but … lax input data validation does not improve things …

Lapidary supports errata (kinda like aiopenapi3 plugins but jsonpatch/yaml) so the user can provide their own schemas. But I think eventually a runtime mechanism might be necessary. Dynamic client might be a better basis for this.

I've had my time with SOAP before, the aiopenapi3 plugin interface is inspired by suds.

Same with any/all/oneOf - using pydantic it can be supported up to to a certain point.

I found anyOf trivial to implement - simply a typing.Union. Works both ways - encoding and decoding.

But wrong:

The keywords used to combine schemas are:

Unique param names are trivial too, I generate names in Hungarian notation - adding (p/q/c/h)_ prefix, and putting the original name in alias. I might change it to suffix to make it work better with code completion (so that developer starts typing the param name and not the prefix).

So far I optimized for the case where parameter names do not collide. Thought about .call(…, HeaderParameter(name, value), PathParameter(name, value)) but …

Feel free to check out the client generated with Lapidary https://github.com/oeklo/gsmtasks-client (the errata got bigger since the last push)

Talking about additionalProperties: https://github.com/oeklo/gsmtasks-client/blob/main/gen/gsmtasks/components/schemas/account.py#L24:

import pydantic

class AccountTaskExpiryDurationFromCompleteAfter(pydantic.BaseModel):
    class Config(pydantic.BaseConfig):
        extra = pydantic.Extra.allow

b = AccountTaskExpiryDurationFromCompleteAfter(dict='yes')
b.dict()
#Traceback (most recent call last):
#  File "<stdin>", line 1, in <module>
#TypeError: 'str' object is not callable

Authentication - I don't see authentication besides ApiToken to work properly.

https://github.com/python-lapidary/lapidary/blob/3ffd6acc02006dfac9c9e0a0dc12e7294b66a7cc/src/lapidary/render/templates/client_init.py.jinja2#L30

https://github.com/python-lapidary/lapidary/blob/3ffd6acc02006dfac9c9e0a0dc12e7294b66a7cc/src/lapidary/render/templates/client_op_method.py.jinja2#L34

Aside from having different authentication types, authentication is modular, and you can have multiple SecuritySchemes defined per PathItem. I fly with 

api.authenticate(tokenAuth="BearToken", userAuth=("name","password"), adminAuth=("admin","betterpassword"))

, and lookup which scheme to use per request/on the PathItem.

Talking about IDE integration and completion, I start to see value in code generation, even if it was just stub files for a dynamic client. Stubs are a good idea.

Nice meeting you.

rafalkrupinski commented 2 years ago

From my experience using pydantic will give you a hard time with additionalProperties (defaulting to True even)...

I find this discussion very useful, so I'm moving it to discussions in Lapidary - https://github.com/python-lapidary/lapidary/discussions/19

Same with any/all/oneOf - using pydantic it can be supported up to to a certain point.

https://github.com/python-lapidary/lapidary/discussions/20

So far I optimized for the case where parameter names do not collide. Thought about .call(…, HeaderParameter(name, value), PathParameter(name, value)) but …

Or you could do call(headers=dict(name=value,...),path_params=...) There was a similar proposal for openapi v4.

Talking about additionalProperties: https://github.com/oeklo/gsmtasks-client/blob/main/gen/gsmtasks/components/schemas/account.py#L24:

https://github.com/python-lapidary/lapidary/discussions/22

Authentication - I don't see authentication besides ApiToken to work properly.

https://github.com/python-lapidary/lapidary/discussions/24

Talking about IDE integration and completion, I start to see value in code generation, even if it was just stub files for a dynamic client. Stubs are a good idea.

Funny you should say that, I'm thinking of using aiopenapi3 as the runtime library :)

Nice meeting you.

Likewise