Skip to content

fix: make SQLAlchemy import lazy and declare it under [sink] extra - #71

Open
0x070907 wants to merge 8 commits into
0-Shimanshu:mainfrom
0x070907:fix/sqlalchemy-lazy-import
Open

fix: make SQLAlchemy import lazy and declare it under [sink] extra#71
0x070907 wants to merge 8 commits into
0-Shimanshu:mainfrom
0x070907:fix/sqlalchemy-lazy-import

Conversation

@0x070907

@0x070907 0x070907 commented May 16, 2026

Copy link
Copy Markdown
Contributor

Summary

This PR makes the import lazy and declares sqlalchemy under a proper [sink] optional extra in pyproject.toml so dependency declarations match actual runtime expectations.

Why

The Flask integration imports _sink_mode from sqlalchemy.py at module scope, and sqlalchemy.py imports SQLAlchemy at the top level. Since sqlalchemy was only listed under [dev], importing AdiuvareMiddleware crashed on a plain install(pip install .) .

  1. adiuvare/integrations/sqlalchemy.py - Removed top-level from sqlalchemy import event. Moved it inside attach_sink() as a lazy import pointing at pip install adiuvare[sink]
  2. pyproject.toml - Added [sink] optional extra sqlalchemy so dependency declarations match runtime expectations.
  3. tests/test_flask.py - Added test_flask_middleware_import_does_not_require_sqlalchemy which proves flask import works without sqlalchemy.
  4. tests/test_sink.py - Added pytest.importorskip guard at top,so it gracefully skips when sqlalchemy isn't installed instead of crashing

Scope

  • bug fix
  • documentation
  • tests
  • TUI / CLI improvement
  • framework integration
  • signal change
  • configuration or route-policy change

Verification

Commands run:

#1. verify plain install no longer crashes on import for the flask app to run
pip install .
python -m pytest tests/test_flask.py tests/test_sink.py  -v 
# test_sink.py SKIPPED (sqlalchemy not installed)
image
#2.  verify sink extra pulls 
pip install adiuvare[sink]
pytest  tests/test_sink.py  -v
image

Notes:

  • I ran the relevant tests locally
  • I updated docs if behavior changed
  • I updated screenshots if the TUI changed materially

Signal-specific notes

If this PR adds or changes a signal or a meaningful detection pattern, fill this in.

Signal type:

  • hard signal (trackA)
  • soft signal (trackB)
  • not applicable

Source, reproducer, or proof: N/A

Benign case checked: N/A

Performance note: N/A

TUI notes

If this PR changes the TUI, include before/after screenshots or a short capture.

  • before/after screenshots attached
  • not applicable

Local-only file check

  • I did not accidentally stage local-only files such as adiuvare.yaml or local conftest.py changes

Related issue

Closes #67

@github-actions github-actions Bot added the enhancement New feature or request label May 16, 2026
@0-Shimanshu

Copy link
Copy Markdown
Owner

The issue itself is real, but this PR is now a bit out of date against main. The pyproject.toml package-data entry for state/schema.sql is already present there, so the remaining value here is really the MANIFEST.in addition rather than both changes together.

The verification note also needs to be tightened. Importing init_state_db does not actually exercise the schema file read, so it does not fully prove the packaging issue is fixed. A better check would be to call init_state_db(...) against a temporary database path after a normal install and confirm that it no longer raises FileNotFoundError.

@0x070907

Copy link
Copy Markdown
Contributor Author

Hi @0-Shimanshu, I have added the comment noqa : E402 in test_sink.py so that module level import at the top is ignored for the 3 lines after pytest.importorskip, as it should be ignored if the app shouldn't crash when sqlalchemy isn't installed.
Is this test coverage sufficient, or would you like me to add anything else ?

@0-Shimanshu

Copy link
Copy Markdown
Owner

The main fix looks correct now. Making the SQLAlchemy import lazy inside attach_sink() solves the plain Flask import crash, and adding the [flask] extra is the right dependency story.

Before merge, please do one small cleanup pass:

  • Since adiuvare.integrations.sqlalchemy no longer imports SQLAlchemy at module scope, _sink_mode can stay imported at module scope in flask.py. It does not need to be imported inside every request.
  • Please clean up the # noqa: E402 comments in tests/test_sink.py so they use the normal format.

After that, this is good to merge.

@0-Shimanshu

Copy link
Copy Markdown
Owner

The lazy import direction is right, but I want to adjust the dependency boundary a bit before merging.

SQLAlchemy sink is not really a Flask-specific feature. Django, Flask, FastAPI, or even plain Python usage could all use the sink path. So I do not think SQLAlchemy should live under a [flask] extra.

Could you update this PR to use a dedicated sink extra instead?

Something like:

[project.optional-dependencies]
sink = [
  "sqlalchemy",
]

@0x070907

Copy link
Copy Markdown
Contributor Author

@0-Shimanshu , sure I will make the necessary changes and update the branch and let you know

@0x070907
0x070907 force-pushed the fix/sqlalchemy-lazy-import branch from 06dd362 to eeab3a7 Compare May 19, 2026 06:38
@0x070907

0x070907 commented May 19, 2026

Copy link
Copy Markdown
Contributor Author

@0-Shimanshu , I have made the necessary changes and the flask app doesn't crash anymore.

test_sink.py is skipped if sqlachemy doesn't exist.

image

After i run pip install .[sink],all the tests passed

image

Should I update the README,quickstart.md and installation.md with [sink] extra ?
I have resolved all the merge conflicts.Kindly check

@0-Shimanshu

0-Shimanshu commented May 20, 2026

Copy link
Copy Markdown
Owner

The sink dependency should be documented as a dedicated extra, not under Flask.

For local development, use:
pip install ".[sink]"

That works because . means the current Python project, and pip reads the package name from pyproject.toml.
For GitHub install, document it as:
pip install "adiuvare[sink] @ git+https://github.com/0-Shimanshu/ADIUVARE.git"

And later, once we publish to PyPI, users can use:
pip install "adiuvare[sink]"

Please update README / quickstart / installation docs with this wording.
Also do one small formatting cleanup before merge:
remove the extra trailing spaces / blank lines in flask.py, sqlalchemy.py, and test_flask.py.

@0x070907

Copy link
Copy Markdown
Contributor Author

Sure, I'll update it

@0-Shimanshu

Copy link
Copy Markdown
Owner

@0x070907 update
??

@github-actions github-actions Bot added the documentation Improvements or additions to documentation label Jun 1, 2026
@0x070907
0x070907 force-pushed the fix/sqlalchemy-lazy-import branch 2 times, most recently from e8a0749 to c369d62 Compare June 1, 2026 07:14
@0x070907

0x070907 commented Jun 1, 2026

Copy link
Copy Markdown
Contributor Author

Hi @0-Shimanshu, apologies for the delayed response - My exams are going on.

I've now addressed all the feedback:

  • Removed trailing spaces in tests/test_flask.py.
  • Updated README.md, docs/installation.md, docs/quickstart.md, and docs/integrations/sqlalchemy.md to document the [sink] extra.

@0x070907 0x070907 changed the title fix: make SQLAlchemy import lazy and declare it under [flask] extra fix: make SQLAlchemy import lazy and declare it under [sink] extra Jun 1, 2026
@0x070907

0x070907 commented Jun 1, 2026

Copy link
Copy Markdown
Contributor Author

Should I update django.md as well?

@0-Shimanshu

Copy link
Copy Markdown
Owner

Should I update django.md as well?

like what can u first tell me the changes.

@0x070907

0x070907 commented Jun 9, 2026

Copy link
Copy Markdown
Contributor Author

Should I update django.md as well?

like what can u first tell me the changes.

I will add a note about the [sink] extra to docs/integrations/django.md so Django users know how to install SQLAlchemy if they want to use attach_sink() alongside Django

@0-Shimanshu

Copy link
Copy Markdown
Owner

Should I update django.md as well?

like what can u first tell me the changes.

I will add a note about the [sink] extra to docs/integrations/django.md so Django users know how to install SQLAlchemy if they want to use attach_sink() alongside Django

i think that would be good go ahead

@0x070907
0x070907 force-pushed the fix/sqlalchemy-lazy-import branch from 2980986 to 7ed80f0 Compare June 11, 2026 16:38
@0x070907

Copy link
Copy Markdown
Contributor Author

Should I update django.md as well?

like what can u first tell me the changes.

I will add a note about the [sink] extra to docs/integrations/django.md so Django users know how to install SQLAlchemy if they want to use attach_sink() alongside Django

i think that would be good go ahead

Its done,please check

@0-Shimanshu

Copy link
Copy Markdown
Owner

@0x070907 dont force push it break stuff and u pushed i project.toml unecesseary file

@0x070907

Copy link
Copy Markdown
Contributor Author

okay,I wont force push again.But pyproject.toml is necessary because sink extra is added

@0-Shimanshu

Copy link
Copy Markdown
Owner

okay,I wont force push again.But pyproject.toml is necessary because sink extra is added

oo ya right

@0x070907
0x070907 force-pushed the fix/sqlalchemy-lazy-import branch from 51fa4ff to 7ed80f0 Compare June 20, 2026 14:34
@0x070907

Copy link
Copy Markdown
Contributor Author

@0-Shimanshu I accidentally got a merge commit on this branch,and in order to remove the commit without affecting any files or the commit history,I had to force push and I confirm that this hasn't affected any file or the main branch

@Wahid7852
Wahid7852 requested a review from 0-Shimanshu June 24, 2026 20:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Align SQLAlchemy dependency declarations with integration imports

2 participants