chore(cloud-native): upgrade to SQLAlchemy v2.x #12937
Conversation
Signed-off-by: iromli <isman.firmansyah@gmail.com>
📝 WalkthroughWalkthroughUpdated many Dockerfiles to point JANS_SOURCE_VERSION at a new commit; migrated jans-pycloudlib and several scripts to SQLAlchemy 2.x APIs (transactions, text(), URL.create, RowMapping, metadata.reflect) and bumped the sqlalchemy requirement to >=2.0. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)jans-pycloudlib/jans/pycloudlib/lock/sql_lock.py (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
🔇 Additional comments (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
mo-auto
added
comp-jans-pycloudlib
kind-dependencies
Signed-off-by: iromli <isman.firmansyah@gmail.com>
Signed-off-by: iromli <isman.firmansyah@gmail.com>
Signed-off-by: iromli <isman.firmansyah@gmail.com>
Signed-off-by: iromli <isman.firmansyah@gmail.com>
mo-auto
added
comp-docker-jans-all-in-one
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docker-jans-cloudtools/scripts/cleaner.py (1)
16-22: Add type validation to prevent SQL injection via the--limitparameter.The
limitClick option lacks type validation, meaning it accepts arbitrary strings. Sincelimitis directly interpolated into SQL queries (lines 38, 40), a malicious value like--limit "1000; DROP TABLE users"could lead to SQL injection.🔎 Proposed fix
@click.option( "--limit", help="How many expired entries need to be removed per table", + type=int, default=1000, show_default=True, )jans-pycloudlib/jans/pycloudlib/lock/sql_lock.py (1)
42-45: Bug:self._dialectshould beself.client.dialect.The
SqlLockclass doesn't have a_dialectattribute. The dialect is accessed viaself.client.dialect. This will raiseAttributeErrorwhen aDatabaseErroris caught.🔎 Proposed fix
- if self._dialect in ("pgsql", "postgresql") and exc.orig.pgcode != "42P07": + if self.client.dialect in ("pgsql", "postgresql") and exc.orig.pgcode != "42P07": raise_on_error = True - elif self._dialect == "mysql" and exc.orig.args[0] != 1050: + elif self.client.dialect == "mysql" and exc.orig.args[0] != 1050:
♻️ Duplicate comments (3)
docker-jans-casa/Dockerfile (1)
64-64: Same JANS_SOURCE_VERSION update as other Dockerfiles.This change is consistent with the version update in docker-jans-cloudtools/Dockerfile and other files in this PR.
docker-jans-configurator/Dockerfile (1)
30-30: Same JANS_SOURCE_VERSION update as other Dockerfiles.This change is consistent with the version update across all Dockerfiles in this PR.
docker-jans-fido2/Dockerfile (1)
70-70: Same JANS_SOURCE_VERSION update as other Dockerfiles.This change is consistent with the version update across all Dockerfiles in this PR.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (17)
docker-jans-all-in-one/Dockerfiledocker-jans-auth-server/Dockerfiledocker-jans-casa/Dockerfiledocker-jans-cloudtools/Dockerfiledocker-jans-cloudtools/scripts/cleaner.pydocker-jans-config-api/Dockerfiledocker-jans-configurator/Dockerfiledocker-jans-fido2/Dockerfiledocker-jans-link/Dockerfiledocker-jans-persistence-loader/Dockerfiledocker-jans-persistence-loader/scripts/sql_setup.pydocker-jans-saml/Dockerfiledocker-jans-saml/scripts/configure_kc.pydocker-jans-scim/Dockerfilejans-pycloudlib/jans/pycloudlib/lock/sql_lock.pyjans-pycloudlib/jans/pycloudlib/persistence/sql.pyjans-pycloudlib/pyproject.toml
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-18T07:43:55.761Z
Learnt from: pujavs
Repo: JanssenProject/jans PR: 12704
File: jans-config-api/docs/jans-config-api-swagger.yaml:17540-17546
Timestamp: 2025-11-18T07:43:55.761Z
Learning: The file `jans-config-api/docs/jans-config-api-swagger.yaml` is auto-generated with dependent modules changes and metadata. The config API does not override it.
Applied to files:
docker-jans-configurator/Dockerfiledocker-jans-config-api/Dockerfile
📚 Learning: 2025-11-10T17:38:39.610Z
Learnt from: iromli
Repo: JanssenProject/jans PR: 12679
File: docker-jans-config-api/Dockerfile:120-120
Timestamp: 2025-11-10T17:38:39.610Z
Learning: In docker-jans-config-api/Dockerfile, the sed command that replaces 'your-openid-provider.server' with '%(hostname)s' in the adminui-policy-store.json file is intentional. The %(hostname)s variable uses Python string interpolation format and is consumed by scripts executed during container runtime.
Applied to files:
docker-jans-fido2/Dockerfiledocker-jans-scim/Dockerfiledocker-jans-cloudtools/Dockerfiledocker-jans-config-api/Dockerfiledocker-jans-persistence-loader/Dockerfiledocker-jans-casa/Dockerfiledocker-jans-saml/Dockerfile
🧬 Code graph analysis (4)
docker-jans-saml/scripts/configure_kc.py (1)
jans-pycloudlib/jans/pycloudlib/persistence/sql.py (2)
metadata(660-677)engine(533-576)
jans-pycloudlib/jans/pycloudlib/lock/sql_lock.py (1)
jans-pycloudlib/jans/pycloudlib/persistence/sql.py (1)
engine(533-576)
docker-jans-cloudtools/scripts/cleaner.py (1)
jans-pycloudlib/jans/pycloudlib/persistence/sql.py (1)
quoted_id(746-748)
jans-pycloudlib/jans/pycloudlib/persistence/sql.py (1)
jans-pycloudlib/jans/pycloudlib/lock/sql_lock.py (2)
get(63-81)table(51-61)
🪛 Ruff (0.14.10)
docker-jans-cloudtools/scripts/cleaner.py
38-38: Possible SQL injection vector through string-based query construction
(S608)
40-40: Possible SQL injection vector through string-based query construction
(S608)
jans-pycloudlib/jans/pycloudlib/persistence/sql.py
705-705: Unused noqa directive (unknown: B902)
Remove unused noqa directive
(RUF100)
743-743: Unused noqa directive (unknown: B902)
Remove unused noqa directive
(RUF100)
773-773: Unused noqa directive (unknown: B902)
Remove unused noqa directive
(RUF100)
776-776: Use X | Y for type annotations
Convert to X | Y
(UP007)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: docker (scim)
- GitHub Check: docker (fido2)
- GitHub Check: docker (cloudtools)
- GitHub Check: docker (loadtesting-jmeter)
- GitHub Check: docker (configurator)
- GitHub Check: docker (config-api)
- GitHub Check: docker (auth-server)
- GitHub Check: docker (persistence-loader)
- GitHub Check: sonar scan (jans-scim)
- GitHub Check: sonar scan (jans-fido2)
- GitHub Check: sonar scan (jans-auth-server)
🔇 Additional comments (27)
docker-jans-all-in-one/Dockerfile (1)
62-62: The all-in-one image aggregates pre-built component images; SQLAlchemy compatibility must be verified in individual components, not in docker-jans-all-in-one/app/requirements.txt.The app/requirements.txt does not contain SQLAlchemy. Verify SQLAlchemy v2.x compatibility in the individual component images (configurator, persistence-loader, jans-auth, jans-config-api, jans-fido2, jans-scim, jans-casa, jans-saml) that are composed into this all-in-one image. The commit
5b98e9ff2eff00fa421529fcc5b84e054f3c79b7aggregates artifacts from these components; any database ORM compatibility updates must be validated in those upstream builds.Likely an incorrect or invalid review comment.
docker-jans-link/Dockerfile (1)
66-66: The commit correctly aligns with the SQLAlchemy v2.x upgrade.SQLAlchemy v2.x is properly specified in
jans-pycloudlib/pyproject.toml(sqlalchemy>=2.0), and the pinned commit contains the necessary refactoring: updatingmetadata.reflect(self.engine)tometadata.reflect(conn)injans-pycloudlib/jans/pycloudlib/persistence/sql.py. This is a required SQLAlchemy 2.0 compatibility change. Runtime testing remains essential to validate the full upgrade path.docker-jans-auth-server/Dockerfile (1)
106-106: SQLAlchemy v2.x is already specified in jans-pycloudlib, not auth-server—no verification needed.The commit (5b98e9f) is a table reflection bug fix (
metadata.reflect(conn)instead ofmetadata.reflect(engine)), not a SQLAlchemy v2.x migration. SQLAlchemy v2.x is already a dependency of jans-pycloudlib (viapyproject.toml:sqlalchemy>=2.0), which auth-server depends on. The auth-server's own requirements.txt does not list SQLAlchemy directly—it inherits it transitively.Likely an incorrect or invalid review comment.
docker-jans-scim/Dockerfile (1)
65-65: Confirmed: jans-pycloudlib specifies SQLAlchemy v2.x compatibility.The SQLAlchemy v2.x upgrade is properly addressed. The
jans-pycloudlibdependency indocker-jans-scim/requirements.txtinstalls from a local path that includespyproject.tomlspecifyingsqlalchemy>=2.0. This ensures SQLAlchemy v2.x will be installed at runtime. Verify that commit5b98e9ff2eff00fa421529fcc5b84e054f3c79b7is a valid build commit before deployment, as the specific commit hash could not be independently confirmed.docker-jans-persistence-loader/Dockerfile (1)
21-21: SQLAlchemy v2.x compatibility verified — implementation already meets all requirements.Verification confirms the persistence-loader component properly implements SQLAlchemy v2.x compatibility:
- ✅ All DDL/DML operations in
sql_setup.pyuse thetext()wrapper for raw SQL (lines 268, 269, 292, 308, 321, 324, 345, 358, 362, 363) — the required v2.0 execution style- ✅ SQLAlchemy v2.x is specified:
jans-pycloudlibdeclaressqlalchemy>=2.0in its dependencies, which is pulled transitively- ✅ No v1-style execute patterns detected; all raw SQL properly wrapped with
text()The implementation correctly addresses the sonatype-2023-4321 vulnerability mitigation through SQLAlchemy v2.x.
docker-jans-cloudtools/Dockerfile (1)
47-47: Commit 5b98e9f contains SQLAlchemy v2-compatible changes.The commit exists and specifically fixes table reflection in jans-pycloudlib (a behavior changed in SQLAlchemy v2). The pyproject.toml in this commit confirms
sqlalchemy>=2.0as an explicit dependency, verifying SQLAlchemy v2 compatibility.docker-jans-cloudtools/scripts/cleaner.py (1)
38-41: Implementation correctly uses dialect-specific quote characters.The
quoted_idmethod inSqlClientproperly handles identifier quoting for both MySQL (backtick:`) and PostgreSQL (double quote:"). The adapter is correctly initialized based on theCN_SQL_DB_DIALECTenvironment variable, and the quoting approach is appropriate for table names sourced from database metadata viaget_table_mapping().docker-jans-saml/scripts/configure_kc.py (2)
360-361: LGTM: Correct SQLAlchemy v2.x metadata reflection pattern.The change from
MetaData(bind=self.engine)toMetaData()followed bymetadata.reflect(self.engine)correctly adapts to SQLAlchemy 2.x, where thebindparameter was removed from the MetaData constructor.
372-372: LGTM: Correct SQLAlchemy v2.x parameter passing.The dictionary-based parameter passing (
{"username": self.user}) aligns with SQLAlchemy 2.x requirements for text() queries.docker-jans-persistence-loader/scripts/sql_setup.py (2)
267-269: LGTM: Proper transactional DDL execution.Wrapping ALTER TABLE operations in an explicit transaction block (
with conn.begin()) and usingtext()for SQL strings aligns with SQLAlchemy 2.x best practices for DDL operations.
291-292: LGTM: Proper transactional column addition.The transaction block ensures atomicity for the ADD COLUMN operation, consistent with SQLAlchemy 2.x patterns.
docker-jans-config-api/Dockerfile (1)
77-77: Commit 5b98e9f contains SQLAlchemy v2.x compatible assets.Verified: The commit's jans-pycloudlib/pyproject.toml specifies
sqlalchemy>=2.0, confirming the SQLAlchemy v2.x migration is present. The commit message "fix: reflect newly created table" aligns with SQLAlchemy v2 compatibility work.docker-jans-saml/Dockerfile (1)
42-42: Commit verified to contain SQLAlchemy v2.x compatible assets.The referenced commit (5b98e9f) includes the necessary SQLAlchemy v2 API changes:
text()wrapper for SQL queries,metadata.reflect(engine/conn)calls for table reflection, and proper v2 imports. Thepyproject.tomlcorrectly specifiessqlalchemy>=2.0, confirming v2.x compatibility.jans-pycloudlib/jans/pycloudlib/lock/sql_lock.py (4)
72-81: LGTM! Correct SQLAlchemy v2.x patterns for read operations.The
select(self.table)syntax andentry._mappingaccess are the correct SQLAlchemy 2.x idioms. Read operations don't require explicit transaction blocks.
100-107: LGTM! Proper transactional write with IntegrityError handling.The explicit transaction block via
conn.begin()is the correct SQLAlchemy 2.x pattern for write operations. TheIntegrityErrorcatch for duplicate key scenarios is appropriate.
125-128: LGTM! Consistent transaction handling for update operation.
141-144: LGTM! Consistent transaction handling for delete operation.jans-pycloudlib/jans/pycloudlib/persistence/sql.py (10)
27-27: LGTM! Required import for SQLAlchemy v2.x raw SQL execution.
649-657: LGTM! Correct use ofURL.create()for SQLAlchemy v2.x.Using
URL.create()is the recommended approach in SQLAlchemy 2.x for building connection URLs programmatically.
674-677: LGTM! Correct SQLAlchemy v2.x metadata reflection pattern.The unbound
MetaData()with explicitreflect(self.engine)is the correct v2 approach, as thebindparameter was removed.
681-683: LGTM! Proper use oftext()for raw SQL in SQLAlchemy v2.x.
756-761: LGTM! Correct SQLAlchemy v2.x select syntax for count query.
800-803: LGTM! Correct transactional update operation.
810-818: LGTM! Correct SQLAlchemy v2.x patterns for search/iteration.
821-825: LGTM! Proper use oftext()andscalar()for version query.
954-957: LGTM! Consistent transactional delete operation.
987-990: LGTM! Proper transactional upsert with dialect-specific query handling.
Signed-off-by: iromli <isman.firmansyah@gmail.com>
Signed-off-by: iromli <isman.firmansyah@gmail.com>
Signed-off-by: iromli <isman.firmansyah@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
docker-jans-persistence-loader/scripts/sql_setup.pydocker-jans-saml/scripts/configure_kc.pyjans-pycloudlib/jans/pycloudlib/persistence/sql.py
🧰 Additional context used
🧬 Code graph analysis (3)
docker-jans-saml/scripts/configure_kc.py (1)
jans-pycloudlib/jans/pycloudlib/persistence/sql.py (2)
metadata(660-677)engine(533-576)
docker-jans-persistence-loader/scripts/sql_setup.py (1)
jans-pycloudlib/jans/pycloudlib/persistence/sql.py (1)
quoted_id(746-748)
jans-pycloudlib/jans/pycloudlib/persistence/sql.py (1)
jans-pycloudlib/jans/pycloudlib/lock/sql_lock.py (2)
get(63-81)table(51-61)
🪛 Ruff (0.14.10)
jans-pycloudlib/jans/pycloudlib/persistence/sql.py
776-776: Use X | Y for type annotations
Convert to X | Y
(UP007)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: sonar scan (jans-scim)
- GitHub Check: sonar scan (jans-orm)
- GitHub Check: sonar scan (jans-link)
- GitHub Check: sonar scan (jans-core)
- GitHub Check: sonar scan (jans-pycloudlib)
- GitHub Check: sonar scan (jans-linux-setup)
- GitHub Check: sonar scan (jans-keycloak-link)
- GitHub Check: sonar scan (jans-config-api)
- GitHub Check: docker (scim)
- GitHub Check: docker (persistence-loader)
- GitHub Check: docker (cloudtools)
- GitHub Check: docker (monolith)
- GitHub Check: docker (fido2)
- GitHub Check: docker (config-api)
- GitHub Check: docker (configurator)
- GitHub Check: docker (auth-server)
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (go)
🔇 Additional comments (11)
docker-jans-saml/scripts/configure_kc.py (2)
360-361: LGTM! Correct SQLAlchemy v2.x metadata initialization.The metadata is now initialized without binding to an engine, and reflection is performed explicitly via
reflect(self.engine). This aligns with SQLAlchemy 2.x best practices.
367-395: LGTM! Correct parameter binding and SQL identifier handling.The dictionary-style parameter binding (line 372, 392) is correct for SQLAlchemy v2.x. The direct interpolation of the privilege name on line 389 is appropriate, as SQL identifiers (privilege names, table names) cannot be bound parameters. The inline comment clearly explains this constraint.
jans-pycloudlib/jans/pycloudlib/persistence/sql.py (7)
649-657: LGTM! Correct SQLAlchemy v2.x URL construction.Using
URL.create()with explicitly specified parameters (including the emptyquery={}dict) is the recommended pattern for SQLAlchemy 2.x.
660-677: LGTM! Correct metadata initialization for SQLAlchemy v2.x.The metadata is initialized without engine binding (line 675) and reflection is performed explicitly via
reflect(self.engine)(line 676), which is the correct pattern for SQLAlchemy 2.x.
679-744: LGTM! Correct transaction patterns and text() usage.The use of
text()for raw SQL (lines 682, 702, 742) and explicit transaction blocks withconn.begin()(lines 700, 740) are correct SQLAlchemy v2.x patterns. The metadata refresh within the transaction (line 704) ensures schema changes are immediately visible.
756-825: LGTM! Correct SQLAlchemy v2.x query and result handling patterns.The code correctly uses:
select()with explicit column lists or tables (lines 756, 782-786, 811-813)entry._mappingfor converting rows to dictionaries (lines 793, 818)- Connection-scoped
scalar()withtext()for server version query (lines 823-825)All patterns align with SQLAlchemy 2.x best practices.
Note: The
Uniontype hint on line 776 is intentionally kept per previous review discussion.
795-803: LGTM! Correct transactional update pattern.The explicit transaction block with
conn.begin()ensures the update operation is atomic.
949-957: LGTM! Correct transactional delete pattern.The delete operation properly uses an explicit transaction and returns a boolean indicating success based on
rowcount.
971-990: LGTM! Correct transactional upsert pattern.The upsert operation properly applies JSON defaults, constructs the dialect-specific upsert query via the adapter, and executes it within a transaction.
docker-jans-persistence-loader/scripts/sql_setup.py (2)
266-269: LGTM! Correct transactional DDL with proper identifier quoting.The transaction block (line 267) ensures atomicity of the DROP/ADD COLUMN operations, and both statements properly use
self.client.quoted_id()for SQL identifiers (table name and column name).
290-292: LGTM! Correct transactional ADD COLUMN with proper quoting.The explicit transaction and proper identifier quoting are correct.
Signed-off-by: iromli <isman.firmansyah@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
docker-jans-persistence-loader/scripts/sql_setup.py
🧰 Additional context used
🧬 Code graph analysis (1)
docker-jans-persistence-loader/scripts/sql_setup.py (1)
jans-pycloudlib/jans/pycloudlib/persistence/sql.py (1)
quoted_id(746-748)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
- GitHub Check: docker (monolith)
- GitHub Check: sonar scan (jans-scim)
- GitHub Check: Analyze (go)
- GitHub Check: docker (configurator)
- GitHub Check: sonar scan (jans-cli-tui)
- GitHub Check: sonar scan (jans-auth-server)
- GitHub Check: docker (auth-server)
- GitHub Check: docker (scim)
- GitHub Check: docker (cloudtools)
- GitHub Check: Analyze (python)
- GitHub Check: sonar scan (agama)
- GitHub Check: docker (fido2)
- GitHub Check: sonar scan (jans-linux-setup)
- GitHub Check: docker (persistence-loader)
- GitHub Check: docker (config-api)
- GitHub Check: lint (jans-cli-tui)
🔇 Additional comments (4)
docker-jans-persistence-loader/scripts/sql_setup.py (4)
266-269: LGTM! Proper SQLAlchemy v2.x transaction handling.The transaction block ensures atomic execution of the DROP and ADD COLUMN operations, and identifier quoting is consistently applied to both table and column names using
self.client.quoted_id().
290-292: LGTM! Consistent transaction and quoting pattern.Single DDL statement properly wrapped in a transaction block with consistent identifier quoting.
302-324: LGTM! Previous review feedback addressed.The identifier quoting inconsistency flagged in past reviews has been fixed - both
table_nameandidx[0]are now properly quoted usingself.client.quoted_id()on line 321. The parameterized information_schema query correctly uses named placeholders with a parameter dictionary.
360-363: LGTM! Consistent identifier quoting in DROP/ADD COLUMN.Both the DROP COLUMN and ADD COLUMN statements correctly quote all identifiers using
self.client.quoted_id(), addressing the consistency concerns from past reviews.
Signed-off-by: iromli <isman.firmansyah@gmail.com>
Signed-off-by: iromli <isman.firmansyah@gmail.com>
|
|
|
|
Signed-off-by: iromli <isman.firmansyah@gmail.com>
|
Prepare
Description
Target issue
closes #12936
closes #12939
Implementation Details
Test and Document the changes
Please check the below before submitting your PR. The PR will not be merged if there are no commits that start with
docs:to indicate documentation changes or if the below checklist is not selected.Summary by CodeRabbit
Chores
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.