Conversation
|
Add This PR introduces a new generator capability to mark generated/computed database columns as readonly in generated models via a new A new detection pipeline in Key Changes• Added new config flag Affected Areas• This summary was automatically generated by @propel-code-bot |
There was a problem hiding this comment.
PR is solid overall with minor maintainability improvements needed around error handling and query clarity.
Status: Minor Suggestions | Risk: Low
Issues Identified & Suggestions
- Preserve/log primary MySQL query errors before fallback retries:
internal/generate/readonly_columns.go - Remove invalid PostgreSQL
is_generated='YES'condition to reduce confusion:internal/generate/readonly_columns.go
Review Details
📁 12 files reviewed | 💬 2 comments
👍 / 👎 individual comments to help improve reviews for you
| err := db.Raw( | ||
| `SELECT COLUMN_NAME | ||
| FROM information_schema.COLUMNS | ||
| WHERE TABLE_SCHEMA = ? | ||
| AND TABLE_NAME = ? | ||
| AND ( | ||
| UPPER(COALESCE(GENERATION_TYPE, '')) IN ('VIRTUAL', 'STORED') | ||
| OR UPPER(COALESCE(EXTRA, '')) LIKE '%GENERATED%' | ||
| )`, | ||
| currentDB, tableName, | ||
| ).Scan(&rows).Error | ||
| if err != nil { |
There was a problem hiding this comment.
[Maintainability] The MySQL fallback query silently swallows the original error from the first query attempt. If the primary query fails for a reason other than the GENERATION_TYPE column not existing (e.g., a permissions error, network timeout, or malformed DSN), the code retries with a weaker query and discards the root cause. This makes the failure mode invisible and hard to debug in production.
Consider logging the first error before retrying, or propagating it unless it matches a known "column not found" error condition. At minimum, the original error should not be silently dropped.
Context for Agents
The MySQL fallback query silently swallows the original error from the first query attempt. If the primary query fails for a reason other than the `GENERATION_TYPE` column not existing (e.g., a permissions error, network timeout, or malformed DSN), the code retries with a weaker query and discards the root cause. This makes the failure mode invisible and hard to debug in production.
Consider logging the first error before retrying, or propagating it unless it matches a known "column not found" error condition. At minimum, the original error should not be silently dropped.
File: internal/generate/readonly_columns.go
Line: 70| FROM information_schema.columns | ||
| WHERE table_schema = ? | ||
| AND table_name = ? | ||
| AND UPPER(COALESCE(is_generated, '')) IN ('ALWAYS', 'YES')`, |
There was a problem hiding this comment.
[Maintainability] The PostgreSQL query filters on UPPER(COALESCE(is_generated, '')) IN ('ALWAYS', 'YES'). Per the SQL standard and PostgreSQL documentation, information_schema.columns.is_generated only takes the value 'ALWAYS' for generated columns — 'YES' is not a valid value for this column in any PostgreSQL version. Including 'YES' is either dead code or based on a misreading of the spec. Verify the intended behavior; if 'YES' is not needed, remove it to avoid confusion about what is being matched.
Context for Agents
The PostgreSQL query filters on `UPPER(COALESCE(is_generated, '')) IN ('ALWAYS', 'YES')`. Per the SQL standard and PostgreSQL documentation, `information_schema.columns.is_generated` only takes the value `'ALWAYS'` for generated columns — `'YES'` is not a valid value for this column in any PostgreSQL version. Including `'YES'` is either dead code or based on a misreading of the spec. Verify the intended behavior; if `'YES'` is not needed, remove it to avoid confusion about what is being matched.
File: internal/generate/readonly_columns.go
Line: 110
Detect read-only fields (computed/generated) and mark them with GORM-tag