UI4T architecture rehaul #1022
UI4T architecture rehaul #1022
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request introduces a new API module for DBT automation, adding endpoints to create and manage DBT nodes and operations. It also includes a migration to add new models for DBT entities, along with corresponding Django model definitions. Additionally, new schema definitions for endpoint payloads have been created and a dedicated router with proper tagging and mounting was added to the main API. An import statement was updated to include the new DBT automation module. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant A as DBT Automation API
participant DB as Database
%% Create DBT Model Node
C->>A: POST /dbtmodel/{model_uuid}/node/ (payload)
A->>A: Validate org warehouse & DBT workspace
A->>DB: Start transaction & create DbtNode
DB-->>A: Node created
A-->>C: Node creation response
%% Chain Operation Node
C->>A: POST /node/{node_uuid}/chain/operation/ (payload)
A->>A: Validate warehouse, workspace & node existence
A->>DB: Create DbtOperation, new DbtNode and chain edge
DB-->>A: Operation chained
A-->>C: Chained operation response
%% Retrieve Project DAG
C->>A: GET /graph/
A->>A: Validate configurations
A->>DB: Fetch nodes and edges
DB-->>A: Return DAG data
A-->>C: Project DAG response
%% Delete Node
C->>A: DELETE /node/{node_uuid}/
A->>A: Validate node & dependencies
A->>DB: Delete node and edges within a transaction
DB-->>A: Deletion succeeded
A-->>C: Deletion response
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1022 +/- ##
==========================================
- Coverage 70.48% 69.26% -1.23%
==========================================
Files 84 86 +2
Lines 8082 8366 +284
==========================================
+ Hits 5697 5795 +98
- Misses 2385 2571 +186 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
ddpui/routes.py (1)
92-92: Router mounted correctly but consider grouping with related routers.The DBT automation router is correctly mounted, but for better code organization, consider grouping it with the other DBT-related router (line 85).
src_api.add_router("/api/airbyte/", airbyte_router) src_api.add_router("/api/dashboard/", dashboard_router) src_api.add_router("/api/data/", data_router) src_api.add_router("/api/dbt/", dbt_router) +src_api.add_router("/api/dbt_automation/", dbtautomation_router) src_api.add_router("/api/notifications/", notification_router) src_api.add_router("/api/prefect/tasks/", orgtask_router) src_api.add_router("/api/prefect/", pipeline_router) src_api.add_router("/api/superset/", superset_router) src_api.add_router("/api/tasks/", task_router) src_api.add_router("/api/transform/", transform_router) -src_api.add_router("/api/dbt_automation/", dbtautomation_router) src_api.add_router("/api/userpreferences/", userpreference_router)ddpui/schemas/dbt_automation_schema.py (3)
1-1: Remove unused import.The
Fieldimport from ninja is not used in this file. Consider removing it to keep the imports clean.-from ninja import Field, Schema +from ninja import Schema🧰 Tools
🪛 Ruff (0.8.2)
1-1:
ninja.Fieldimported but unusedRemove unused import:
ninja.Field(F401)
11-17: Consider adding type annotation for canvas_lock_id.While setting a default value of
Noneis good, explicitly defining the type asOptional[str]would improve type safety and clarity.from ninja import Schema +from typing import Optional class CreateDbtModelNodePayload(Schema): """ schema to define the payload required to create a model node on the canvas """ - canvas_lock_id: str = None + canvas_lock_id: Optional[str] = None
19-26: Apply the same type annotation improvement to ChainOperationPayload.Similar to the previous comment, explicitly defining the type for
canvas_lock_idwould improve clarity.class ChainOperationPayload(Schema): """ schema to chain operation on an existing node """ op_type: str config: dict - canvas_lock_id: str = None + canvas_lock_id: Optional[str] = Noneddpui/models/dbt_automation.py (2)
5-7: Remove unused imports.The static analysis indicates that
uuid,Schema, andEnumare never referenced within this file. Please remove them to reduce clutter and avoid confusion.Apply this diff to remove the unused imports:
- import uuid - from ninja import Schema - from enum import Enum🧰 Tools
🪛 Ruff (0.8.2)
5-5:
uuidimported but unusedRemove unused import:
uuid(F401)
6-6:
ninja.Schemaimported but unusedRemove unused import:
ninja.Schema(F401)
7-7:
enum.Enumimported but unusedRemove unused import:
enum.Enum(F401)
18-18: Avoid overshadowing the importeduuid.Each model redefines the name
uuidas a field, overshadowing the importeduuidmodule, which is unused anyway. After removing the import, there's no overlap, but it's clearer to retainuuidas a field name if the import is removed entirely. Alternatively, if you intend to useuuid.uuid4()in the future, consider renaming the field to something likeuuid_field.Also applies to: 36-36, 51-51, 70-70
🧰 Tools
🪛 Ruff (0.8.2)
18-18: Redefinition of unused
uuidfrom line 5(F811)
ddpui/api/dbt_automation_api.py (1)
2-2: Remove unused imports.These imports are not used within the file and cause unnecessary overhead. Please remove them to keep the codebase clean.
Also applies to: 3-3, 4-4, 12-12, 15-15, 24-24, 26-26, 31-31, 33-33, 34-34, 35-35, 36-36, 37-37, 39-39, 40-40, 45-45
🧰 Tools
🪛 Ruff (0.8.2)
2-2:
shutilimported but unusedRemove unused import:
shutil(F401)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
ddpui/api/dbt_automation_api.py(1 hunks)ddpui/migrations/0119_orgdbtmodelv1_orgdbtoperationv1_dbtnode_dbtedgev1.py(1 hunks)ddpui/models/__init__.py(1 hunks)ddpui/models/dbt_automation.py(1 hunks)ddpui/routes.py(3 hunks)ddpui/schemas/dbt_automation_schema.py(1 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
ddpui/models/dbt_automation.py (3)
ddpui/models/org.py (1)
OrgDbt(40-58)ddpui/models/dbt_workflow.py (1)
OrgDbtModelType(21-30)ddpui/tests/services/test_dbtautomation_service.py (1)
orgdbt(34-43)
ddpui/api/dbt_automation_api.py (3)
ddpui/models/org.py (2)
OrgDbt(40-58)OrgWarehouse(116-137)ddpui/models/dbt_automation.py (4)
OrgDbtModelv1(15-30)OrgDbtOperationv1(33-44)DbtEdgev1(67-87)DbtNode(47-64)ddpui/schemas/dbt_automation_schema.py (3)
CreateDbtModelNodePayload(11-16)ChainOperationPayload(19-26)EdgeConfig(4-8)
🪛 Ruff (0.8.2)
ddpui/models/__init__.py
7-7: ddpui.models.dbt_automation imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
ddpui/schemas/dbt_automation_schema.py
1-1: ninja.Field imported but unused
Remove unused import: ninja.Field
(F401)
ddpui/models/dbt_automation.py
5-5: uuid imported but unused
Remove unused import: uuid
(F401)
6-6: ninja.Schema imported but unused
Remove unused import: ninja.Schema
(F401)
7-7: enum.Enum imported but unused
Remove unused import: enum.Enum
(F401)
18-18: Redefinition of unused uuid from line 5
(F811)
36-36: Redefinition of unused uuid from line 5
(F811)
51-51: Redefinition of unused uuid from line 5
(F811)
70-70: Redefinition of unused uuid from line 5
(F811)
ddpui/api/dbt_automation_api.py
2-2: shutil imported but unused
Remove unused import: shutil
(F401)
3-3: pathlib.Path imported but unused
Remove unused import: pathlib.Path
(F401)
4-4: datetime.datetime imported but unused
Remove unused import: datetime.datetime
(F401)
12-12: django.utils.text.slugify imported but unused
Remove unused import: django.utils.text.slugify
(F401)
15-15: ddpui.ddpdbt.dbt_service.setup_local_dbt_workspace imported but unused
Remove unused import: ddpui.ddpdbt.dbt_service.setup_local_dbt_workspace
(F401)
24-24: ddpui.models.canvaslock.CanvasLock imported but unused
Remove unused import: ddpui.models.canvaslock.CanvasLock
(F401)
26-26: ddpui.schemas.org_task_schema.DbtProjectSchema imported but unused
Remove unused import: ddpui.schemas.org_task_schema.DbtProjectSchema
(F401)
31-31: ddpui.schemas.dbt_automation_schema.EdgeConfig imported but unused
Remove unused import: ddpui.schemas.dbt_automation_schema.EdgeConfig
(F401)
33-33: ddpui.core.orgdbt_manager.DbtProjectManager imported but unused
Remove unused import: ddpui.core.orgdbt_manager.DbtProjectManager
(F401)
34-34: ddpui.utils.taskprogress.TaskProgress imported but unused
Remove unused import: ddpui.utils.taskprogress.TaskProgress
(F401)
35-35: ddpui.core.transformfunctions.validate_operation_config imported but unused
Remove unused import: ddpui.core.transformfunctions.validate_operation_config
(F401)
36-36: ddpui.api.warehouse_api.get_warehouse_data imported but unused
Remove unused import: ddpui.api.warehouse_api.get_warehouse_data
(F401)
37-37: ddpui.models.tasks.TaskProgressHashPrefix imported but unused
Remove unused import: ddpui.models.tasks.TaskProgressHashPrefix
(F401)
39-39: ddpui.core.dbtautomation_service imported but unused
Remove unused import: ddpui.core.dbtautomation_service
(F401)
40-40: ddpui.core.dbtautomation_service.sync_sources_for_warehouse imported but unused
Remove unused import: ddpui.core.dbtautomation_service.sync_sources_for_warehouse
(F401)
45-45: ddpui.utils.transform_workflow_helpers.from_orgdbtoperation imported but unused
Remove unused import: ddpui.utils.transform_workflow_helpers.from_orgdbtoperation
(F401)
206-206: Redefinition of unused post_chain_operation_node from line 87
(F811)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: checks (3.10, 6)
🔇 Additional comments (7)
ddpui/routes.py (2)
21-21: Import and API router configuration for DBT automation added correctly.Good addition of the DBT automation router import, following the established pattern for other routers.
79-79: Tags properly set for DBT automation router.The tag "Dbt Automation (UI4T)" is clearly defined, which will properly categorize this section in the API documentation.
ddpui/schemas/dbt_automation_schema.py (1)
4-8: Well-defined schema for edge configuration.The
EdgeConfigschema is well-structured with appropriate type annotations for source columns and sequence.ddpui/migrations/0119_orgdbtmodelv1_orgdbtoperationv1_dbtnode_dbtedgev1.py (4)
14-42: Well-structured OrgDbtModelv1 model creation.The model definition is correctly implemented with appropriate fields and constraints. The type field with predefined choices is a good practice.
43-62: OrgDbtOperationv1 model properly defined.The operation model includes all necessary fields with correct types. The JSONField for config is appropriate for storing operation configurations.
63-98: DbtNode model with proper relationships.The DbtNode model correctly establishes relationships with OrgDbtModelv1, OrgDbtOperationv1, and orgdbt through ForeignKey fields.
99-132: DbtEdgev1 model correctly defines directed edges between nodes.The edge model properly uses ForeignKey relationships to connect nodes in a directed manner with from_node and to_node fields. The config JSONField allows for flexible edge configuration.
| from ddpui.models.org_preferences import OrgPreferences | ||
| from ddpui.models.org_supersets import OrgSupersets | ||
| from ddpui.models.org_wren import OrgWren | ||
| import ddpui.models.dbt_automation |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Import statement lacks proper package exposure mechanism.
The import statement is flagged by static analysis as unused. If the intention is to make the dbt_automation module accessible when importing from ddpui.models, you should add it to an __all__ list in the __init__.py file.
from ddpui.models.tasks import Task
from ddpui.models.llm import AssistantPrompt
from ddpui.models.syncstats import SyncStats
from ddpui.models.org_preferences import OrgPreferences
from ddpui.models.org_supersets import OrgSupersets
from ddpui.models.org_wren import OrgWren
import ddpui.models.dbt_automation
+ __all__ = [
+ "Task", "AssistantPrompt", "SyncStats", "OrgPreferences",
+ "OrgSupersets", "OrgWren", "dbt_automation"
+ ]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import ddpui.models.dbt_automation | |
| from ddpui.models.tasks import Task | |
| from ddpui.models.llm import AssistantPrompt | |
| from ddpui.models.syncstats import SyncStats | |
| from ddpui.models.org_preferences import OrgPreferences | |
| from ddpui.models.org_supersets import OrgSupersets | |
| from ddpui.models.org_wren import OrgWren | |
| import ddpui.models.dbt_automation | |
| __all__ = [ | |
| "Task", "AssistantPrompt", "SyncStats", "OrgPreferences", | |
| "OrgSupersets", "OrgWren", "dbt_automation" | |
| ] |
🧰 Tools
🪛 Ruff (0.8.2)
7-7: ddpui.models.dbt_automation imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
ddpui/models/dbt_automation.py
Outdated
| def __str__(self) -> str: | ||
| return f"DbtEdge[{self.from_node.operation.op_type or self.from_node.model.name} -> {self.to_node.operation.op_type or self.to_node.model.name}]" |
There was a problem hiding this comment.
Fix incorrect attribute references in the DbtEdgev1.__str__ method.
The string representation references self.from_node.operation and self.from_node.model, but the foreign key fields are named dbtoperation and dbtmodel. Accessing the non-existent .operation or .model attributes will cause an error.
Apply this diff to ensure the correct foreign key fields are used:
-def __str__(self) -> str:
- return f"DbtEdge[{self.from_node.operation.op_type or self.from_node.model.name} -> {self.to_node.operation.op_type or self.to_node.model.name}]"
+def __str__(self) -> str:
+ from_op = self.from_node.dbtoperation.op_type if self.from_node.dbtoperation else self.from_node.dbtmodel.name
+ to_op = self.to_node.dbtoperation.op_type if self.to_node.dbtoperation else self.to_node.dbtmodel.name
+ return f"DbtEdge[{from_op} -> {to_op}]"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def __str__(self) -> str: | |
| return f"DbtEdge[{self.from_node.operation.op_type or self.from_node.model.name} -> {self.to_node.operation.op_type or self.to_node.model.name}]" | |
| def __str__(self) -> str: | |
| from_op = self.from_node.dbtoperation.op_type if self.from_node.dbtoperation else self.from_node.dbtmodel.name | |
| to_op = self.to_node.dbtoperation.op_type if self.to_node.dbtoperation else self.to_node.dbtmodel.name | |
| return f"DbtEdge[{from_op} -> {to_op}]" |
|
|
||
| @dbtautomation_router.delete("/node/{node_uuid}/", auth=auth.CustomAuthMiddleware()) | ||
| @has_permission(["can_create_dbt_model"]) | ||
| def post_chain_operation_node(request, node_uuid: str, canvas_lock_id: str = None): |
There was a problem hiding this comment.
Rename the duplicate function.
The function post_chain_operation_node is redefined here, overshadowing the version at line 87, causing confusion. Please rename this deletion endpoint to something like delete_dbt_node or remove_dbt_node to differentiate from the operation-chaining endpoint.
Apply this diff to rename the function:
-@dbtautomation_router.delete("/node/{node_uuid}/", auth=auth.CustomAuthMiddleware())
-@has_permission(["can_create_dbt_model"])
-def post_chain_operation_node(request, node_uuid: str, canvas_lock_id: str = None):
+@dbtautomation_router.delete("/node/{node_uuid}/", auth=auth.CustomAuthMiddleware())
+@has_permission(["can_create_dbt_model"])
+def delete_dbt_node(request, node_uuid: str, canvas_lock_id: str = None):📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def post_chain_operation_node(request, node_uuid: str, canvas_lock_id: str = None): | |
| @dbtautomation_router.delete("/node/{node_uuid}/", auth=auth.CustomAuthMiddleware()) | |
| @has_permission(["can_create_dbt_model"]) | |
| def delete_dbt_node(request, node_uuid: str, canvas_lock_id: str = None): |
🧰 Tools
🪛 Ruff (0.8.2)
206-206: Redefinition of unused post_chain_operation_node from line 87
(F811)
1 participant
Summary by CodeRabbit