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

⬆️ Upgrade aiohttp-apispec and apispec #2920

Merged
merged 6 commits into from
May 31, 2024

Conversation

ff137
Copy link
Contributor

@ff137 ff137 commented Apr 26, 2024

The aiohttp-apispec project is no longer maintained, and is pinned to some very old versions of apispec and webargs.

It appeared to be the only library that would throw deprecation warnings that had to be added to this pyproject.toml's filterwarnings rules.

I forked the project and manually upgrade some of the dependencies. Not too much effort. All it means is that the version now points to my fork:

aiohttp-apispec = { git = "https://github.com/ff137/aiohttp-apispec.git", tag = "v3.0.1" }

Maybe the maintainer sees my contributions and it can get merged into the official release. Otherwise, anyone can fork my fork, upgrade it, and update the above link.


This permits apispec to go from v3 to v6.6. And webargs to go from v5 to v8.4

This means the custom filterwarning can be removed - the one for ignoring "distutils Version classes are deprecated"

And one more perk is that aiohttp-apispec is now compatible up to python 3.12. So it's one step closer in allowing ACA-Py to finally upgrade from python 3.9 :-)

@ff137
Copy link
Contributor Author

ff137 commented Apr 26, 2024

Everything passed locally. The PR Test failures seem like spurious json-ld context failures. 4 cases of:

FAILED aries_cloudagent/vc/vc_ld/tests/test_manager.py::test_store - pyld.jsonld.JsonLdError: ('Could not expand input before compaction.',)

@ff137
Copy link
Contributor Author

ff137 commented Apr 26, 2024

Sadge. Integration test failures too

@ff137 ff137 marked this pull request as draft April 26, 2024 14:49
@dbluhm
Copy link
Contributor

dbluhm commented May 8, 2024

I haven't glanced at the specifics just yet but I assume it would be accurate to say that replacing this library with an actively maintained alternative is infeasible?

@ff137
Copy link
Contributor Author

ff137 commented May 8, 2024

@dbluhm I think that would be better, yes. But as far as I could tell, it seemed like quite a bit of refactoring to move to another library.

Maybe it's not such a heavy lift -- it's more that it feels redundant to refactor something that's not broken. If there's other motivation for moving to a more up-to-date library, then that's definitely better. But here the only issue I saw is that it's pinned to some deprecated versions

ff137 added 3 commits May 10, 2024 00:10
Signed-off-by: ff137 <ff137@proton.me>
Unmaintained library that I've forked and manually upgraded

Signed-off-by: ff137 <ff137@proton.me>
…ecated"

Signed-off-by: ff137 <ff137@proton.me>
@ff137 ff137 force-pushed the upgrade/aiohttp_apispec branch from 384ed5e to a17a689 Compare May 9, 2024 21:11
@swcurran
Copy link
Contributor

Any recent updates on this? I see the PR to aiohttp-apispec has not been merged, so that doesn't seem to be the path.

@jamshale
Copy link
Contributor

I think we should go ahead with this and look for alternatives in the meantime. It would be better to have a library which is well maintained, but I don't think this is really a downgrade going from what we are using now to @ff137's github.

@ff137
Copy link
Contributor Author

ff137 commented May 31, 2024

@jamshale agreed - it would be better to use a more well-maintained library, but there's probably not much motivation to replace aiohttp-apispec yet, given other priorities.

As it stands I've just taken the existing aiohttp_apispec repo and upgraded dependencies. I've made a PR to merge to their main repo, and emailed the owner, but haven't gotten any feedback.

So, this will just allow some libraries to upgrade a few major version (like apispec and webargs), and it'll allow for python 3.12 when that's desired.

It probably looks weird to have one dependency pointing to my personal fork, but that can be changed any time, and anyone can fork my fork to build on the existing changes if need be

@ff137 ff137 marked this pull request as ready for review May 31, 2024 12:10
@ff137
Copy link
Contributor Author

ff137 commented May 31, 2024

3 scenarios failing in integration tests. Each of them due to a validation error:

marshmallow.exceptions.ValidationError: {'wallet_webhook_urls': ['Not a valid list.']}

Seems to be a change in webargs (edit: webargs5 used marshmallow2, and webargs8 uses marshmallow3), which I guess was previously ignoring a bad value being passed: the demo is providing a str when the model expects a List[str]

Signed-off-by: ff137 <ff137@proton.me>
Copy link

sonarcloud bot commented May 31, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@swcurran swcurran requested review from dbluhm and jamshale May 31, 2024 16:25
@swcurran
Copy link
Contributor

Ready to merge?

Copy link
Contributor

@jamshale jamshale left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved. I think this is better then what we are currently doing. I'll create a ticket for looking into a replacement.

@jamshale jamshale merged commit 667a6fa into openwallet-foundation:main May 31, 2024
8 checks passed
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.

4 participants