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

Look for a well maintained replacement of aiohttp-apispec #3003

Closed
jamshale opened this issue May 31, 2024 · 5 comments
Closed

Look for a well maintained replacement of aiohttp-apispec #3003

jamshale opened this issue May 31, 2024 · 5 comments
Labels
1.0.0 To be addressed for the ACA-Py 1.0.0 release

Comments

@jamshale
Copy link
Contributor

aiohttp-apispec isn't being maintained and we are now using a forked repo. It's from @ff137's github so it's not a big concern currently. But this isn't what we want long term. If the repo gets deleted then the version of acapy that uses it will not work anymore.

We need to find a replacement for our swagger documentation that is well maintained and will continue to be, and determine a migration plan.

@PatStLouis
Copy link
Contributor

There's a list of alternatives available.

This one seems to be the better option in terms of activity.

@jamshale
Copy link
Contributor Author

jamshale commented Jun 3, 2024

I wondering if the best way forward is just to keep a fork of aiohttp-apispec like we are doing now. Maybe owned by hyperledger, instead of @ff137, and publish releases ourselves.

I don't think there's really much maintenance to do. It just can't prevent us from upgrading our dependencies in the stale state it's been in. From what I can tell any change is going to be massive and doesn't really improve our situation very much.

@ff137
Copy link
Contributor

ff137 commented Jun 4, 2024

I'm happy with the changes being forked by hyperledger. That'll make sense if you want to stick with using aiohttp-apispec long term.

There's probably some benefits to migrating, but it's certainly not a game changer and can probably turn into a lot of work. It's basically all the routes.py files that will need to be refactored. For example, these decorators:

@docs(tags=["multitenancy"], summary="Query subwallets")
@querystring_schema(WalletListQueryStringSchema())
@response_schema(WalletListSchema(), 200, description="")

and then

async def register(app: web.Application):
    """Register routes."""

    app.add_routes(
        [
            web.get("/multitenancy/wallets", wallets_list, allow_head=False), 
            ...

will have to follow a different design pattern in aiohttp-pydantic

Then, another big thing is that the endpoint functions will receive a Pydantic model, instead of a web.requests object. Which is pretty great if you ask me. Things like:

    body = await request.json()
    rev_reg_def_id = body.get("rev_reg_def_id")
    options = body.get("options", {})

that can be improved significantly, because you can avoid unnecessary json deserialisation and working with Any types. I think that'll be a pretty great quality of life improvement.

Ultimately, it does represent a major refactoring job. It's not an easy "replace all" kind of problem. And worth mentioning, it'll probably impact the acapy-plugins repo as well, to adapt to the new pattern.

But, personally, I don't shy away from big refactoring jobs that only amount to minor improvements 😄 I sometimes enjoy it, so I can take a look at how much is really required with migrating to aiohttp-pydantic, and whether it's really a worthwhile improvement. Will be fairly low priority, so will take some time to get around to it

@jamshale
Copy link
Contributor Author

jamshale commented Jun 4, 2024

@ff137 Sounds good. Thanks for the insight with aiohttp-pydantic. I was wondering if we could make that work with the marshmallow models we currently have. I'm mostly just concerned with the timeline for getting a v1 release completed. I'm not really a fan of releasing it the way it is right now.

I might try again migrating a single routes file. If we have a good example of the process I'm not opposed to doing a large migration for v1.

If it seems unfeasible short term, I think we should just get a pypi release for what we are using now so it's not pointing to a forked repo and we can ensure it will work permanently.

@jamshale jamshale added the 1.0.0 To be addressed for the ACA-Py 1.0.0 release label Jun 4, 2024
@jamshale
Copy link
Contributor Author

jamshale commented Jun 6, 2024

I'm going to close this for now via #3019. We should still consider migrating to another tool but we can open another ticket when we think we're ready. I think this is good enough to move forward for 1.0.0.

@jamshale jamshale closed this as completed Jun 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.0.0 To be addressed for the ACA-Py 1.0.0 release
Projects
None yet
Development

No branches or pull requests

3 participants