-
Notifications
You must be signed in to change notification settings - Fork 73
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
fix: Use a SQLAlchemy to generate an insert statement #2843
fix: Use a SQLAlchemy to generate an insert statement #2843
Conversation
Reviewer's Guide by SourceryThis pull request refactors the SQL insert statement generation to use SQLAlchemy, which prevents SQL injection vulnerabilities and improves code maintainability. The changes include modifying the Sequence diagram for SQLAlchemy insert statement generationsequenceDiagram
participant Client
participant SQLSink
participant SQLAlchemy
participant Database
Client->>SQLSink: bulk_insert_records()
activate SQLSink
SQLSink->>SQLSink: generate_insert_statement()
SQLSink->>SQLAlchemy: Create Table object
SQLAlchemy-->>SQLSink: Return Table
SQLSink->>SQLAlchemy: Generate Insert statement
SQLAlchemy-->>SQLSink: Return Insert object
SQLSink->>Database: Execute Insert
Database-->>SQLSink: Confirm insertion
SQLSink-->>Client: Complete
deactivate SQLSink
Class diagram showing SQL sink changesclassDiagram
class SQLSink {
+generate_insert_statement(full_table_name, schema)
+bulk_insert_records(records, schema)
-conform_schema(schema)
-conform_record(record)
}
class SQLConnector {
+create_engine()
+quote(name) deprecated
+parse_full_table_name(full_table_name)
}
class SQLAlchemy {
+Table
+Column
+MetaData
+Insert
}
SQLSink --> SQLConnector
SQLSink --> SQLAlchemy
note for SQLSink "Now uses SQLAlchemy for insert statements"
note for SQLConnector "quote() method deprecated"
Flow diagram of insert statement generationgraph TD
A[Start] --> B[Get Schema Properties]
B --> C[Parse Table Name]
C --> D[Create SQLAlchemy Table]
D --> E[Create Columns]
E --> F[Generate Insert Statement]
F --> G[Return SQLAlchemy Insert]
style D fill:#f9f,stroke:#333,stroke-width:2px
style F fill:#f9f,stroke:#333,stroke-width:2px
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2843 +/- ##
==========================================
+ Coverage 91.31% 91.33% +0.01%
==========================================
Files 62 62
Lines 5206 5205 -1
Branches 672 671 -1
==========================================
Hits 4754 4754
Misses 319 319
+ Partials 133 132 -1 ☔ View full report in Codecov by Sentry. |
CodSpeed Performance ReportMerging #2843 will not alter performanceComparing Summary
|
ac6d156
to
db06328
Compare
@sourcery-ai review |
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.
Hey @edgarrmondragon - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider mapping JSON schema types to appropriate SQLAlchemy column types instead of assuming all columns are strings. This would provide better type safety and data integrity.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟡 Testing: 1 issue found
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
db06328
to
06a2f92
Compare
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.
Hey @edgarrmondragon - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider using appropriate SQLAlchemy column types instead of assuming String for all columns. This would provide better type safety and potentially better performance.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
sa.Column( | ||
name, sa.String | ||
) # Assuming all columns are of type String for simplicity # noqa: E501 |
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.
issue (performance): Using String type for all columns could cause data type mismatches and performance issues.
Consider mapping the schema types to appropriate SQLAlchemy types based on the conformed_schema['properties'] type definitions.
conformed_schema = self.conform_schema(schema) | ||
property_names = list(conformed_schema["properties"]) | ||
|
||
_, schema_name, table_name = self.connector.parse_full_table_name( |
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.
issue: Need to handle case where schema_name is None.
The Table constructor will fail if schema_name is None. Consider adding a conditional to handle this case.
Summary by Sourcery
Update insert statements to use SQLAlchemy executables.
Bug Fixes:
Enhancements:
Tests:
📚 Documentation preview 📚: https://meltano-sdk--2843.org.readthedocs.build/en/2843/
Summary by Sourcery
Update insert statements to use SQLAlchemy executables.
Bug Fixes:
Enhancements:
Tests: