frontend: OpenAPI first steps #2473

FrostyX · 2023-01-14T06:28:51Z

FrostyX · 2023-01-14T06:39:30Z

After our last discussion about API documentation, I was looking forward to experimenting with all the available packages.

apiflask

In this PR, I wanted to show https://apiflask.com/
It has some quirks but overall it seems quite intuitive.
However, installation requires some work. You need to install rawhide version python-marshmallow (you can do it from my Copr project frostyx/apiflask). Then pip3 install apiflask.

You can access the pretty docs page at http://127.0.0.1:5000/docs and you can download the API spec at http://127.0.0.1:5000/openapi.json

This is just a proof-of-concept, if you prefer any other API package, we can throw this away.

FrostyX · 2023-01-14T06:57:43Z

flask-restx

I also tried https://flask-restx.readthedocs.io/en/latest/index.html which looked the best at first sight, but it seems to really be only for REST APIs. I.e. it expects routes to be classes that implement get, post, put, etc methods. I couldn't find a way to add a route that is a function and specify its endpoint and HTTP method.

flask-openapi3

The https://luolingchun.github.io/flask-openapi3/ looks good but it seems to have less ways how to do things. For example, query parameters need to be defined as classes https://luolingchun.github.io/flask-openapi3/Tutorial/Request/#query . The apiflask package has more options how to do this.

flasgger

I haven't tried https://github.com/flasgger/flasgger yet. It seems to be really straightforward - maybe too much. ~~There is no generation~~, one must manually write the specification either as dict, json or yaml. Marshmallow schemas can be used as well https://github.com/flasgger/flasgger#using-marshmallow-schemas

praiskup · 2023-01-14T19:15:01Z

Seems like flasgger has noticeable more stars on GH. @jpopelka don't you remember why you've chosen flask-restyx for the Packit project?

praiskup · 2023-01-14T19:16:31Z

Just for the record, I'm not a big fan of the marshmallow models (too many fixes needed over the years..).

FrostyX · 2023-01-14T20:33:50Z

Just for the record, I'm not a big fan of the marshmallow models (too many fixes needed over the years..).

If we don't like marshmallow schemas, then I think we have two options

If we don't like writing them, we can probably generate the marshmallow schemas from our SQLAlchemy models - https://flask-marshmallow.readthedocs.io/en/latest/#flask_marshmallow.sqla.SQLAlchemyAutoSchema
If we dislike the dependency, we can do the thing that Flasgger does and manually write the YAML schemas - https://github.com/flasgger/flasgger#using-external-yaml-files

FTR, even the apiflask that I use in this PR AFAIK internally uses marshmallow. I think these are marshmallow

from apiflask import Schema
from apiflask.fields import Integer, String, List, Nested

So we need to decide what exactly we don't like about marshmallow and how much we don't like it :D

praiskup · 2023-01-16T08:25:51Z

Writing them is easy, but I got a feeling it has an unstable API. The
schema validators were chronically updated with newer marshmallow
versions.

Initially I thought we could live with something like python's built-in
typing, or something non-third party. But if that's not possible, writing
the models is not the problem and I think we have to live with that. As I
read your research -> openAPI depends on marshmallow, so the huge userbase
must mean that the marshmallow eventually gets stabilized too.

BTW. the marshmallow in Fedora is maintained by us, and I was so glad we
removed our dep on it with APIv2 drop.

jpopelka · 2023-01-16T10:52:09Z

@jpopelka don't you remember why you've chosen flask-restyx for the Packit project?

I think I just searched for a library to generate the API documentation (Swagger), found an article about (now unmaintained) Flask-RestPlus (from which Flask-RESTX was later forked due to inactivity), tried it, it did what we needed, done.

Seems like flasgger has noticeable more stars on GH.

yup, but the difference is IMHO not so big

flasgger - 3.2k stars
Flask-RestPlus - 2.7k stars (forked to Flask-RESTX - 1.7k stars)

Or the other way around, depends on what we happen to define at the moment.

frontend/coprs_frontend/coprs/views/apiv3_ns/apiv3_general.py

praiskup · 2023-04-26T07:55:06Z

Finally on a review, sorry for the delay. This looks very nice, thank you!

Some general notes to the /api_3/docs route:

can we use our own favicon?
the page claims APIv3 v1.0 -> would it make sense to claim v3.0?

I'd prefer to have our main documentation pointing at the API documentation directly. Perhaps at least in the Do you have an API FAQ entry? (I mean without the misleading hop to the /api/ page).

praiskup · 2023-04-26T07:57:32Z

frontend/coprs_frontend/coprs/views/apiv3_ns/__init__.py

+# > Initializing the Api object always registers the root endpoint / even if
+# > the Swagger UI path is changed. If you wish to use the root endpoint /
+# > for other purposes, you must register it before initializing the Api object.
+# TODO That needs to get fixed and then we can move this route to apiv3_general


There's never a good time to do this. Can we have an issue for this (we should create a new api end point at least, and twist our library to use it).

I linked the issue in the code comment

we should create a new api end point at least, and twist our library to use it

I am not sure if I understand here. Just to clarify, there is no regression - I needed to move the @apiv3_ns.route("/") route to a different file than it was before but it works as expected.

But we want to change/break this long term, right? Then we need some plan somewhere.

praiskup · 2023-04-26T07:59:22Z

frontend/coprs_frontend/coprs/views/apiv3_ns/apiv3_builds.py

+
+ # Workaround - `marshal_with` needs the input `build_id` to be present
+ # in the returned dict. Don't worry, it won't get to the end user, it
+ # will be stripped away.


Don't you have some restx or other reference link?

Honestly, I couldn't find any. This traceback happens when you remove the workaround:

werkzeug.routing.exceptions.BuildError: Could not build url for endpoint 'apiv3_ns.build_get_build' with values ['chroots', 'ended_on', 'id', 'is_background', 'ownername', 'project_dirname', 'projectname', 'repo_url', 'source_package', 'started_on', 'state', 'submitted_on', 'submitter']. Did you forget to specify values ['build_id']?

It doesn't make any sense to me, but adding the build_id key fixed the traceback. Of course, the build_id doesn't appear in the result because it is not defined in the schema, so it gets filtered out.

praiskup · 2023-04-26T08:24:29Z

frontend/coprs_frontend/coprs/views/apiv3_ns/schema.py

+ """
+ Unless this is an error on my side, there is a bug in flask-restx displaying
+ all URL path parameters inside the Swagger documentation as as required.
+ This is a temporary hack until it gets fixed.


Has this been reported?

As it turned out, it was an error on my side. Updated.

praiskup · 2023-04-26T08:34:34Z

frontend/coprs_frontend/coprs/views/apiv3_ns/schema.py

+
+def edit_package_parser():
+ # pylint: disable=missing-function-docstring
+ parser = add_package_parser().copy()


Is this a deep copy?

I would say, yes.

parser_copy.args = deepcopy(self.args)

https://flask-restx.readthedocs.io/en/latest/_modules/flask_restx/reqparse.html#RequestParser.copy

praiskup · 2023-04-26T08:54:14Z

frontend/coprs_frontend/coprs/views/apiv3_ns/apiv3_packages.py

+ def post(self, ownername, projectname, package_name, source_type_text=None):
+ """
+ Edit a package
+ Edit an existing package within a Copr project.


duplicate doc line

This was kind of intentional.

The first docstring line is used for the "card" header and therefore it is visible even when the route is collapsed. The rest of the docstring is displayed once the route is expanded.

So I basically wanted to write the docstring like

""" Edit a package Edit a package within a Copr project. """

but there is a bug in the docstring parsing - python-restx/flask-restx#536

Anyway, we can write the docstrings in any format you want. My preference would be at least a two-line docstring so that we have at least some text when the route is expanded.

praiskup · 2023-04-26T08:57:20Z

frontend/coprs_frontend/coprs/views/apiv3_ns/schema.py

+)
+
+# TODO We are copy-pasting descriptions from web UI to this file. This field
+# is an ideal candidate for figuring out how to share the descriptions


praiskup · 2023-04-26T09:02:56Z

frontend/coprs_frontend/coprs/views/apiv3_ns/schema.py

+
+package_builds_schema = {
+ "latest": Nested(build_model, allow_null=True),
+ "latest_succeeded": Nested(build_model, allow_null=True),


We miss "builds" field here.

I don't think the with_all_builds option is exposed to the API

Your branch is up to date with 'origin/main' $ git grep with_all_builds frontend/ frontend/coprs_frontend/coprs/models.py: def to_dict(self, with_latest_build=False, with_latest_succeeded_build=False, with_all_builds=False): frontend/coprs_frontend/coprs/models.py: if with_all_builds:

Indeed

copr --debug get-package --name python-backoff --with-all-builds @copr/copr-dev # Debug log enabled # [10:48:13] {/usr/lib/python3.11/site-packages/urllib3/connectionpool.py:228} DEBUG - Starting new HTTP connection (1): localhost:55555 [10:48:13] {/usr/lib/python3.11/site-packages/urllib3/connectionpool.py:456} DEBUG - http://localhost:55555 "GET /api_3/package?ownername=%40copr&projectname=copr-dev&packagename=python-backoff&with_latest_build=False&with_latest_succeeded_build=False HTTP/1.1" 200 458 [10:48:13] {/usr/lib/python3.11/site-packages/urllib3/connectionpool.py:228} DEBUG - Starting new HTTP connection (1): localhost:55555 [10:48:13] {/usr/lib/python3.11/site-packages/urllib3/connectionpool.py:456} DEBUG - http://localhost:55555 "GET /api_3/build/list?ownername=%40copr&projectname=copr-dev&packagename=python-backoff HTTP/1.1" 200 809

praiskup · 2023-04-26T09:06:36Z

It is a pity we have poor coverage for these routes; so I'm a bit afraid that we'll miss some of the fields or so. But it is worth the risk I'd say! I like the change.

@jpopelka would you mind taking a 5-minute look? No big review, just check whether the steps we do here are meaningful, or if there isn't an obvious mistake somewhere.

jpopelka

Looks OK to me 👍

FrostyX · 2023-04-27T20:06:02Z

can we use our own favicon?

I don't think so, the favicon path seems to be "hardcoded"
https://github.com/frostyx/flask-restx/blob/docstring-details/doc/conf.py#L182-L185

Packit uses the default favicon as well

the page claims APIv3 v1.0 -> would it make sense to claim v3.0?

Sure

I'd prefer to have our main documentation pointing at the API documentation directly. Perhaps at least in the Do you have an API FAQ entry? (I mean without the misleading hop to the /api/ page).

Let's do this in a separate PR, we want to show the documentation but we also want to show users how to generate their API keys.

FrostyX · 2023-04-27T20:07:45Z

Looks OK to me +1

Thank you very much for taking a look

It is a pity we have poor coverage for these routes; so I'm a bit afraid that we'll miss some of the fields or so.

Same here, but at least in the future we won't have to be afraid of regressions like missing fields.

FrostyX · 2023-04-27T21:34:53Z

I added some changes. Since it is a big PR, I kept them as separate commits for easier review. I will rebase and squash them before merging.

praiskup · 2023-04-28T09:05:41Z

+1, let's rebase and merge - so we can continue on making it even more awesome

See fedora-copr#1865 This is still miles from being finished. I tried probably all OpenAPI libraries that are available and decided to go with flask-restx. It is already in Fedora, Packit team uses it as well, and it is malleable enough to be comfortable used within our codebase. I intentionally defined all data types and descriptions in `schema.py` and in the way that everything is defined separately in its own variable. We should IMHO try to compose things as much as possible to avoid copy-pasting. Remains to be done: - Only a fraction of API endpoints are migrated under flask-restx, we need to finish the rest of them - There is a potential to generate WTForms forms or SQLAlchemy models from the API schema or the other way around. - We should share descriptions between API fields and web UI as much as possible - Packit team uses does something like this, we should do as well @koji_builds_ns.response(HTTPStatus.OK, "OK, koji build group details follow") Error responses should be documented the same way.

praiskup · 2023-04-28T13:06:24Z

@nikromen / @xsuchy do you want to review folks?

FrostyX marked this pull request as draft January 14, 2023 06:30

FrostyX force-pushed the openapi branch from f1036cc to 80de618 Compare February 15, 2023 10:11

FrostyX force-pushed the openapi branch 3 times, most recently from bd3788c to b070135 Compare April 16, 2023 13:13

frontend: automatically create aliases for routes with trailing slash

421d78b

Or the other way around, depends on what we happen to define at the moment.

FrostyX force-pushed the openapi branch from b070135 to e71fe8a Compare April 16, 2023 14:25

github-advanced-security bot found potential problems Apr 16, 2023

View reviewed changes

frontend/coprs_frontend/coprs/views/apiv3_ns/apiv3_general.py Fixed Show fixed Hide fixed

FrostyX force-pushed the openapi branch from e71fe8a to f32ecfd Compare April 16, 2023 14:29

github-advanced-security bot found potential problems Apr 16, 2023

View reviewed changes

frontend/coprs_frontend/coprs/views/apiv3_ns/apiv3_general.py Fixed Show fixed Hide fixed

frontend/coprs_frontend/coprs/views/apiv3_ns/apiv3_general.py Fixed Show fixed Hide fixed

FrostyX force-pushed the openapi branch 2 times, most recently from 56baf3a to a837590 Compare April 16, 2023 15:07

FrostyX marked this pull request as ready for review April 16, 2023 22:56

FrostyX assigned praiskup Apr 24, 2023

praiskup reviewed Apr 26, 2023

View reviewed changes

jpopelka reviewed Apr 26, 2023

View reviewed changes

FrostyX force-pushed the openapi branch from ebeffa3 to df523a1 Compare April 28, 2023 10:49

praiskup merged commit d5a2d9f into fedora-copr:main May 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

frontend: OpenAPI first steps #2473

frontend: OpenAPI first steps #2473

FrostyX commented Jan 14, 2023

FrostyX commented Jan 14, 2023 •

edited

Loading

FrostyX commented Jan 14, 2023 •

edited

Loading

praiskup commented Jan 14, 2023 •

edited

Loading

praiskup commented Jan 14, 2023

FrostyX commented Jan 14, 2023

praiskup commented Jan 16, 2023

jpopelka commented Jan 16, 2023

praiskup commented Apr 26, 2023

praiskup Apr 26, 2023

FrostyX Apr 27, 2023

FrostyX Apr 27, 2023

praiskup Apr 28, 2023

praiskup Apr 26, 2023

FrostyX Apr 27, 2023

praiskup Apr 26, 2023

FrostyX Apr 27, 2023

praiskup Apr 26, 2023

FrostyX Apr 27, 2023

praiskup Apr 26, 2023

FrostyX Apr 27, 2023

praiskup Apr 26, 2023

praiskup Apr 26, 2023

FrostyX Apr 27, 2023

praiskup Apr 28, 2023

praiskup commented Apr 26, 2023

jpopelka left a comment

FrostyX commented Apr 27, 2023

FrostyX commented Apr 27, 2023

FrostyX commented Apr 27, 2023

praiskup commented Apr 28, 2023

praiskup commented Apr 28, 2023

frontend: OpenAPI first steps #2473

frontend: OpenAPI first steps #2473

Conversation

FrostyX commented Jan 14, 2023

FrostyX commented Jan 14, 2023 • edited Loading

apiflask

FrostyX commented Jan 14, 2023 • edited Loading

flask-restx

flask-openapi3

flasgger

praiskup commented Jan 14, 2023 • edited Loading

praiskup commented Jan 14, 2023

FrostyX commented Jan 14, 2023

praiskup commented Jan 16, 2023

jpopelka commented Jan 16, 2023

praiskup commented Apr 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

praiskup commented Apr 26, 2023

jpopelka left a comment

Choose a reason for hiding this comment

FrostyX commented Apr 27, 2023

FrostyX commented Apr 27, 2023

FrostyX commented Apr 27, 2023

praiskup commented Apr 28, 2023

praiskup commented Apr 28, 2023

FrostyX commented Jan 14, 2023 •

edited

Loading

FrostyX commented Jan 14, 2023 •

edited

Loading

praiskup commented Jan 14, 2023 •

edited

Loading