Move Lifecycle protection to the end #3072
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
jmsuzuki
left a comment
jmsuzuki
left a comment
There was a problem hiding this comment.
Looks good! thanks for the tests
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| // This handles both: | ||
| // 1. Strategies that convert updates to drop+create (blocks both Removed AND Added for protected tables) | ||
| // 2. Strategies that return Updated (filters out column removals for DeletionProtected tables) | ||
| let filtered_changes: Vec<OlapChange> = if respect_life_cycle { |
There was a problem hiding this comment.
Hum this is a better place - but I think we should pull it out of the diff completely and have it applied in another phase of the infra map compiler.
@LucioFranco Woudl you have a suggestion? The issue here is we produce the plan for changes as the set of diff for the infrastructure. That Plan should be sanitized with lifecycle rules which enfore what kind of operations are allowed on a Table / Object.
Right now it is embeeded in the diff logic and it is prone for developper mistakes and doesn't get enforced no matter what. I am thinking it should wrap the internal planning methods which should become private to the inner module to make sure other modules don't call it without the filtering.
Another idea is triggered in which we could use a visitor pattern to prune the plan? or and build it maybe? But that could be another iteration on the refactor
Move lifecycle protection filtering to the end. Previously, clickhouse strategy could upgrade an Update to a DROP/CREATE after lifecycle filtering
Note
Lifecycle protection is enforced after table diff strategies run, blocking DROP/CREATE for protected tables and adding tests for these scenarios.
strategy.diff_table_update, not before.TableChange::Removedand corresponding orphanAddedwhen target table isDeletionProtectedorExternallyManaged.UpdatedonDeletionProtected, filter outColumnChange::Removedonly.after) lifecycle for decisions; only count/apply non-empty filtered changes.DropCreateStrategymock and comprehensive tests covering: blocking strategy-generated DROP/CREATE forDeletionProtectedandExternallyManaged, allowing forFullyManaged, lifecycle transition to protected, and disabling protection via flag.Written by Cursor Bugbot for commit c5e1f06. This will update automatically on new commits. Configure here.