Implement API generator and use it on Connection

[uranusjr]

See #15072, cc @ephraimbuddy

It turns out Connexion performs argument introspection to decide what arguments to pass to an endpoint (a bit like pytest fixtures), so we can't actually make the functions too generic. For example, we must have a parameter named connection_id in various Connection endpoints, but that's obvious specific to Connection and cannot be refactored out. So this two-layer approach is the best I can come up with for now.

I'm not actually convinced myself if this is a good idea (it feels like a ton of architecture that makes code more difficult to understand), but at least now we have something to think about.

[ashb]

Connexion seems to be an abandonded project, so we might want to migrate off to some thing else instead of putting more time in to using it?

[potiuk]

Connexion seems to be an abandonded project, so we might want to migrate off to some thing else instead of putting more time in to using it?

I'd quite agree.

[kaxil]

https://github.com/tiangolo/fastapi ? 😄

[ashb]

Sadly not -- (at least last time I looked) FastAPI is the inverse of what we want when it comes to the schema -- FastAPI creates the OpenAPI schema from the views/models etc, where as we want to specify the schema up front and the implement it

[ashb]

https://github.com/p1c2u/openapi-core is on my list to look at, but it's still a fairly small project so I wouldn't feel happy with Airflow using it unless we also got one of us to have co-maintainer on the project.

[kaxil]

Sadly not -- (at least last time I looked) FastAPI is the inverse of what we want when it comes to the schema -- FastAPI creates the OpenAPI schema from the views/models etc, where as we want to specify the schema up front and the implement it

Oh yeah, forgot about that !! :(

[uranusjr]

Connexion seems to have found some new life recently: spec-first/connexion#1429

But this PR (and the idea of turning the API implementation into structured classes) isn't really that related to Connexion or whatever OpenAPI tool we use; it was mentioned because the implementation hit some restrictions in Connexion and had to avoid some encapsulation to make things work. But this actually has a "nice" side effect that makes our logic encapsulation decoupled to the OpenAPI part, and we wouldn't need to change the extracted classes when we migrate to another OpenAPI implementation. (What will require the class to change is if we switch away from Marshmallow, e.g. use FaskAPI.)

[RobbeSneyders]

@Ruwann and I have recently joined as maintainers on Connexion, specifically because it is the only mature API-first Python framework. Let us know if we can help out.

[uranusjr]

Any thoughts on the general approach taken here? Again, the abstraction implemented here is not really affected by what OpenAPI implementation we use.

[ephraimbuddy]

Any thoughts on the general approach taken here? Again, the abstraction implemented here is not really affected by what OpenAPI implementation we use.

👍 I like the approach.

[uranusjr]

Should be ready.

[ephraimbuddy]

Suggested change

	instance_index: List[str] # List keys to uniquely select an instance.
	instance_indices: List[str] # List keys to uniquely select an instance.

Wondering if it should be plural?

[uranusjr]

This is not a list of indices, but describing an index (in the database sense) to select a unique instance out for comparison (another name I briefly considered was primary_key). But the name is indeed difficult to understand, maybe I'll change it to something like unique_instance_fields. Suggestions are definitely much welcomed.

[ephraimbuddy]

Cool. I prefer the unique_instance_fields

[ephraimbuddy]

Dictionary, right?

[uranusjr]

Right, I changed the implementation but forgot to update the comment.

[github-actions]

The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease.

[ephraimbuddy]

Let's hear from others before merge.

[raphaelauv]

New release with flask2 support -> https://github.com/zalando/connexion/releases/tag/2.10.0

[github-actions]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.