-
Notifications
You must be signed in to change notification settings - Fork 2k
Feature/orm support sql datanodes #2746
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
base: develop
Are you sure you want to change the base?
Feature/orm support sql datanodes #2746
Conversation
|
hey thank you so much for your commit, we would need someone from the taipy maintainer to review the code, and complete the merger |
|
This PR has been labelled as "🥶Waiting for contributor" because it has been inactive for more than 14 days. If you would like to continue working on this PR, then please add new commit or another comment, otherwise this PR will be closed in 14 days. For more information please refer to the contributing guidelines. |
|
This PR has been labelled as "🥶Waiting for contributor" because it has been inactive for more than 14 days. If you would like to continue working on this PR, then please add new commit or another comment, otherwise this PR will be closed in 14 days. For more information please refer to the contributing guidelines. |
|
hey thank you so much for your commit, we would need someone from the taipy maintainer to review the code, and complete the merger |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds ORM support for SQL Data Nodes using SQLAlchemy 2.0, enabling ORM-style CRUD operations that integrate with Taipy's data layer. The implementation provides a new SQLAlchemyTableDataNode class that wraps SQLAlchemy models with session management and detached instance handling.
- Introduces
ORMSessionManagerfor managing SQLAlchemy engine and session lifecycle with automatic commit/rollback - Implements
SQLAlchemyTableDataNodewith full CRUD operations (create, read, update, delete) returning detached model instances - Adds comprehensive unit tests covering CRUD operations on SQLite with SQLAlchemy ORM
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 16 comments.
| File | Description |
|---|---|
taipy/core/_repository/orm_manager.py |
New session manager for SQLAlchemy providing transactional scopes and engine management |
taipy/core/data/sql_table_data_node_orm.py |
New ORM-enabled DataNode implementation extending the base DataNode class |
tests/core/data/test_sql_table_data_node_orm.py |
Unit tests verifying CRUD functionality with SQLAlchemy models |
.gitignore |
Adds .early.coverage to ignored files |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| from __future__ import annotations | ||
| from contextlib import contextmanager | ||
| from typing import Generator | ||
|
|
||
| try: | ||
| from sqlalchemy import create_engine | ||
| from sqlalchemy.orm import sessionmaker, Session | ||
| except ImportError: | ||
| create_engine = None | ||
| sessionmaker = None | ||
| Session = None |
Copilot
AI
Dec 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing copyright header. All source files in the repository follow the standard Apache 2.0 license header pattern (lines 1-10). Add the copyright header:\npython\n# Copyright 2021-2025 Avaiga Private Limited\n#\n# Licensed under the Apache License, Version 2.0 (the \"License\"); you may not use this file except in compliance with\n# the License. You may obtain a copy of the License at\n#\n# http://www.apache.org/licenses/LICENSE-2.0\n#\n# Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on\n# an \"AS IS\" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the\n# specific language governing permissions and limitations under the License.\n\nfrom __future__ import annotations\n
| from __future__ import annotations | ||
| from typing import Any, Iterable, Optional, Type, List, Dict | ||
|
|
||
| from taipy.core.data.data_node import DataNode | ||
| from taipy.core._repository.orm_manager import ORMSessionManager |
Copilot
AI
Dec 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing copyright header. All source files in the repository follow the standard Apache 2.0 license header pattern (lines 1-10). Add the copyright header as shown in Comment 1.
| from __future__ import annotations | ||
| import pytest |
Copilot
AI
Dec 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing copyright header. All test files in the repository follow the standard Apache 2.0 license header pattern (lines 1-10). Add the copyright header as shown in Comment 1.
| config_id: str, | ||
| orm_model: Type[DeclarativeMeta], # type: ignore[name-defined] | ||
| db_url: str, | ||
| primary_keys: Optional[List[str]] = None, | ||
| **kwargs: Any, | ||
| ): | ||
| super().__init__(config_id=config_id, **kwargs) | ||
| if orm_model is None: | ||
| raise ValueError("`orm_model` must be provided.") | ||
| self._orm_model = orm_model | ||
| self._primary_keys = primary_keys or [] |
Copilot
AI
Dec 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The __init__ signature is inconsistent with other DataNode implementations in the codebase. Other DataNodes (e.g., MongoCollectionDataNode, SQLTableDataNode) follow a standard pattern with parameters like scope, id, owner_id, parent_ids, last_edit_date, edits, version, validity_period, edit_in_progress, editor_id, editor_expiration_date, and properties. This implementation bypasses the standard initialization flow and won't integrate properly with Taipy's configuration system and data node lifecycle management. Consider refactoring to accept a properties dict containing orm_model, db_url, and primary_keys, and pass all standard DataNode parameters to the parent constructor.
| config_id: str, | |
| orm_model: Type[DeclarativeMeta], # type: ignore[name-defined] | |
| db_url: str, | |
| primary_keys: Optional[List[str]] = None, | |
| **kwargs: Any, | |
| ): | |
| super().__init__(config_id=config_id, **kwargs) | |
| if orm_model is None: | |
| raise ValueError("`orm_model` must be provided.") | |
| self._orm_model = orm_model | |
| self._primary_keys = primary_keys or [] | |
| scope: Optional[str] = None, | |
| id: Optional[str] = None, | |
| owner_id: Optional[str] = None, | |
| parent_ids: Optional[List[str]] = None, | |
| last_edit_date: Optional[str] = None, | |
| edits: Optional[List[Any]] = None, | |
| version: Optional[str] = None, | |
| validity_period: Optional[Any] = None, | |
| edit_in_progress: Optional[bool] = None, | |
| editor_id: Optional[str] = None, | |
| editor_expiration_date: Optional[str] = None, | |
| properties: Optional[Dict[str, Any]] = None, | |
| ): | |
| super().__init__( | |
| scope=scope, | |
| id=id, | |
| owner_id=owner_id, | |
| parent_ids=parent_ids, | |
| last_edit_date=last_edit_date, | |
| edits=edits, | |
| version=version, | |
| validity_period=validity_period, | |
| edit_in_progress=edit_in_progress, | |
| editor_id=editor_id, | |
| editor_expiration_date=editor_expiration_date, | |
| properties=properties, | |
| ) | |
| properties = properties or {} | |
| orm_model = properties.get("orm_model") | |
| db_url = properties.get("db_url") | |
| primary_keys = properties.get("primary_keys", []) | |
| if orm_model is None: | |
| raise ValueError("`orm_model` must be provided in properties.") | |
| if db_url is None: | |
| raise ValueError("`db_url` must be provided in properties.") | |
| self._orm_model = orm_model | |
| self._primary_keys = primary_keys |
| Session = Any | ||
|
|
||
|
|
||
| class SQLAlchemyTableDataNode(DataNode): |
Copilot
AI
Dec 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing the abstract method implementations required by the DataNode base class. The DataNode class defines abstract methods storage_type(), _read(), and _write() that must be implemented. Add these required methods:\npython\n@classmethod\ndef storage_type(cls) -> str:\n return \"sqlalchemy_table\"\n\ndef _read(self):\n return self.read_all()\n\ndef _write(self, data):\n return self.write(data)\n
| @pytest.fixture(scope="function") | ||
| def node(tmp_path): | ||
| db_url = f"sqlite:///{tmp_path/'t.db'}" | ||
| sm = ORMSessionManager(db_url) | ||
| Base.metadata.create_all(sm.engine) | ||
| return SQLAlchemyTableDataNode( | ||
| config_id="user_node", | ||
| orm_model=User, | ||
| db_url=db_url, | ||
| primary_keys=["id"], | ||
| ) |
Copilot
AI
Dec 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test fixture creates the database schema but doesn't ensure proper cleanup. The ORMSessionManager holds an open engine connection that should be disposed. Add teardown to prevent resource leaks:\npython\[email protected](scope=\"function\")\ndef node(tmp_path):\n db_url = f\"sqlite:///{tmp_path/'t.db'}\"\n sm = ORMSessionManager(db_url)\n Base.metadata.create_all(sm.engine)\n node = SQLAlchemyTableDataNode(\n config_id=\"user_node\",\n orm_model=User,\n db_url=db_url,\n primary_keys=[\"id\"],\n )\n yield node\n # Cleanup\n sm.engine.dispose()\n
| def test_create_and_read(node): | ||
| node.create({"name": "Alice"}) | ||
| node.create([{"name": "Bob"}, {"name": "Chandra"}]) | ||
| rows = node.read_all() | ||
| assert len(rows) == 3 | ||
| assert {r.name for r in rows} == {"Alice", "Bob", "Chandra"} |
Copilot
AI
Dec 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing test coverage for critical functionality: 1) read_by_pk() method is not tested, 2) Error cases (e.g., invalid columns, missing orm_model, SQLAlchemy not installed) are not covered, 3) The write() and read() DataNode protocol methods are not tested, 4) Edge cases like empty where clauses or updates that match no rows are not covered. Add tests for these scenarios to ensure comprehensive coverage.
| Insert one or many records (dicts). Returns number of rows inserted. | ||
| """ | ||
| Model = self._orm_model | ||
| payloads = [records] if isinstance(records, dict) else list(records) |
Copilot
AI
Dec 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No validation that the keys in payload dictionaries match the model's columns. Passing invalid column names will cause SQLAlchemy to raise an error. Add validation before creating instances:\npython\nvalid_columns = {c.name for c in Model.__table__.columns}\nfor p in payloads:\n if invalid := set(p.keys()) - valid_columns:\n raise ValueError(f\"Invalid column names: {invalid}\")\ns.add_all([Model(**p) for p in payloads])\n
| payloads = [records] if isinstance(records, dict) else list(records) | |
| payloads = [records] if isinstance(records, dict) else list(records) | |
| valid_columns = {c.name for c in Model.__table__.columns} | |
| for p in payloads: | |
| invalid = set(p.keys()) - valid_columns | |
| if invalid: | |
| raise ValueError(f"Invalid column names: {invalid}") |
| self._detach_all(s, [obj]) | ||
| return obj | ||
|
|
||
| def update(self, where: Dict[str, Any], values: Dict[str, Any]) -> int: |
Copilot
AI
Dec 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No validation that the keys in the values dictionary match the model's columns. Invalid column names in values will cause SQLAlchemy to raise unclear errors. Add the same validation as suggested in Comment 14 for the values parameter.
| Session = Any | ||
|
|
||
|
|
||
| class SQLAlchemyTableDataNode(DataNode): |
Copilot
AI
Dec 1, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The class 'SQLAlchemyTableDataNode' does not override 'eq', but adds the new attribute _orm_model.
The class 'SQLAlchemyTableDataNode' does not override 'eq', but adds the new attribute _primary_keys.
The class 'SQLAlchemyTableDataNode' does not override 'eq', but adds the new attribute _session_manager.
✨ Feature: ORM Support for SQL Data Nodes using SQLAlchemy
This PR adds ORM support for SQL Data Nodes using SQLAlchemy — enabling ORM-style CRUD operations and seamless integration with Taipy’s data layer.
🧩 Highlights
🧱 ORMSessionManager
Manages SQLAlchemy engine, sessions, and commits
Uses
expire_on_commit=Falseto preventDetachedInstanceError🧠 SQLAlchemyTableDataNode
Full CRUD support (
create,read,update,delete)Integrates with
DataNodeprotocol (read/write)Returns detached model instances safe for use post-session
🧪 Unit Tests
Verified on SQLite with SQLAlchemy ORM
✅ 100% CRUD coverage, clean teardown
🧩 Dependencies
👤 Contributor
@Naveencodespeaks
✨ Feature: ORM Support for SQL Data Nodes using SQLAlchemyPython Developer | Backend Engineer | Taipy Contributor
This PR adds ORM support for SQL Data Nodes using SQLAlchemy — enabling ORM-style CRUD operations and seamless integration with Taipy’s data layer.
🧩 Highlights
🧱 ORMSessionManager
Manages SQLAlchemy engine, sessions, and commits
Uses expire_on_commit=False to prevent DetachedInstanceError
🧠 SQLAlchemyTableDataNode
Full CRUD support (create, read, update, delete)
Integrates with DataNode protocol (read / write)
Returns detached model instances safe for use post-session
🧪 Unit Tests
Verified on SQLite with SQLAlchemy ORM
✅ 100% CRUD coverage, clean teardown
🧩 Dependencies
Dependency Version
🐍 Python 3.11.7
🧱 SQLAlchemy 2.0.35
🧠 Taipy Core Local dev version
🧪 pytest 8.3.3
🧰 marshmallow 3.26.1
✅ Tests
pytest tests/core/data/test_sql_table_data_node_orm.py -q
... [100%]
3 passed, 2 warnings in 0.39s
🧾 Related Tickets
Related Issue: #2745 - ORM Support for SQL Data Nodes
Closes: #2745
⚙️ Reproduction
initialize_and_test_datanode:
description: "Example usage of SQLAlchemyTableDataNode."
code: |
from taipy.core.data.sql_table_data_node_orm import SQLAlchemyTableDataNode
🔄 Backport
backporting:
targets:
- develop
☑️ Checklist
Status Description
✅ Meets acceptance criteria
🧪 Includes CRUD unit tests
🔄 E2E tests planned post-pipeline integration
📚 Docs update planned next PR
📌 Release notes entry confirmed
👤 Contributor
@Naveencodespeaks
Python Developer | Backend Engineer | Taipy Contributor