Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rebased aiopenapi3 #69

Open
wants to merge 125 commits into
base: master
Choose a base branch
from
Open

rebased aiopenapi3 #69

wants to merge 125 commits into from

Conversation

commonism
Copy link

Rebased aiopenapi3 for merging.
Can't re-open #62

describe the base types using dataclasses,
use the data class annotations for parsing
all unit tests pass
 - using Optional
 - default value
 - have a dataclass constructor
 - the classmethod property ObjectBase.required_fields
   requires python 3.9
 - local data loading for split specifications (open5gs)

edit - not including all the api specs …
 - required as __root__ can not have additional fields (extensions)
   in Reference
 - test_model supporting basic discrimination
Reference uses __{g,s}etattr__ to forward to target
@rafalkrupinski
Copy link

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

@commonism
Copy link
Author

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
Copy link

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
Copy link

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

@commonism
Copy link
Author

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:

  • allOf: (AND) Must be valid against all of the subschemas
  • anyOf: (OR) Must be valid against any of the subschemas (one or more)
  • oneOf: (XOR) Must be valid against exactly one of the subschemas

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
Copy link

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 - python-lapidary/lapidary#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:

python-lapidary/lapidary#22

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

python-lapidary/lapidary#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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants