Handle columns who use functions as defaults #192
Conversation
bplunkett-stripe
left a comment
bplunkett-stripe
left a comment
There was a problem hiding this comment.
Thanks for taking a stab at this!
Some changes to make:
- Update the dependency generation
- Update the
schema_testto ensure the functions are correctly fetched - Create acceptance tests for this and validate the existing acceptance tests all pass. These will be your best friend. The code is fragile; the tests are robust.
|
|
||
| var deps []dependency = []dependency{ | ||
| mustRun(csg.GetSQLVertexId(col, diffTypeDelete)).before(csg.GetSQLVertexId(col, diffTypeAddAlter)), | ||
| }, nil | ||
| } | ||
|
|
||
| for _, depFunction := range col.DependsOnFunctions { | ||
| deps = append(deps, mustRun(csg.GetSQLVertexId(col, diffTypeDelete)).after(buildFunctionVertexId(depFunction, diffTypeDelete))) | ||
| } | ||
|
|
||
| return deps, nil | ||
|
|
||
| } | ||
|
|
||
| func (csg *columnSQLVertexGenerator) GetDeleteDependencies(_ schema.Column) ([]dependency, error) { | ||
| return nil, nil | ||
| func (csg *columnSQLVertexGenerator) GetDeleteDependencies(col schema.Column) ([]dependency, error) { | ||
|
|
||
| var deps []dependency | ||
| for _, depFunction := range col.DependsOnFunctions { | ||
| deps = append(deps, mustRun(csg.GetSQLVertexId(col, diffTypeDelete)).before(buildFunctionVertexId(depFunction, diffTypeDelete))) | ||
| } | ||
| return deps, nil |
There was a problem hiding this comment.
I believe this does nothing right now, because the functions exist in the root graph. Ideally, we merge the graphs, but that's too much effort right now.
There was a problem hiding this comment.
So they should be removed ?
| ) | ||
| } | ||
|
|
||
| for _, col := range table.Columns { |
There was a problem hiding this comment.
This is the currently awkward part about column sql generation: add/delete of columns is pushed under table add/alter.
As a result, what I believe you need to do:
- If the column is new/altered, then you need to build a dependency between the table and the function such that function add/alter comes BEFORE the table add/alter
- If the column is being deleted, then you need to build a dependency between the table and the function such that the function deletes comes BEFORE the table add/alter
^
The same is true if the table is being totally created or totally deleted.
There was a problem hiding this comment.
Yes, but that what I'm doing no ?
Here we're in table add/alter, and we add a dep for each column's function that have a dependency, and just bellow the delete (in opposite mode)
diffTypeDelete may be wrong there however ^^'
the-glu
left a comment
the-glu
left a comment
There was a problem hiding this comment.
Sorry I took me so long to be able to fix that one, I had to focus on something else ^^'
I added some acceptance tests (in the function_cases_test) that do pass.
I'm not sure I understood what I would update in schema_test. Should I add the base test case to be sure it's handled?
I left my question about dependency generation on individual comments as well.
| ) | ||
| } | ||
|
|
||
| for _, col := range table.Columns { |
There was a problem hiding this comment.
Yes, but that what I'm doing no ?
Here we're in table add/alter, and we add a dep for each column's function that have a dependency, and just bellow the delete (in opposite mode)
diffTypeDelete may be wrong there however ^^'
|
|
||
| var deps []dependency = []dependency{ | ||
| mustRun(csg.GetSQLVertexId(col, diffTypeDelete)).before(csg.GetSQLVertexId(col, diffTypeAddAlter)), | ||
| }, nil | ||
| } | ||
|
|
||
| for _, depFunction := range col.DependsOnFunctions { | ||
| deps = append(deps, mustRun(csg.GetSQLVertexId(col, diffTypeDelete)).after(buildFunctionVertexId(depFunction, diffTypeDelete))) | ||
| } | ||
|
|
||
| return deps, nil | ||
|
|
||
| } | ||
|
|
||
| func (csg *columnSQLVertexGenerator) GetDeleteDependencies(_ schema.Column) ([]dependency, error) { | ||
| return nil, nil | ||
| func (csg *columnSQLVertexGenerator) GetDeleteDependencies(col schema.Column) ([]dependency, error) { | ||
|
|
||
| var deps []dependency | ||
| for _, depFunction := range col.DependsOnFunctions { | ||
| deps = append(deps, mustRun(csg.GetSQLVertexId(col, diffTypeDelete)).before(buildFunctionVertexId(depFunction, diffTypeDelete))) | ||
| } | ||
| return deps, nil |
There was a problem hiding this comment.
So they should be removed ?
|
Just a gentle reminder about this PR, when you have a moment, could you please take a quick look? Thanks a lot! |
|
@bplunkett-stripe is there anything preventing this from ending up in the tool? Not being able to modify columns that use functions as defaults is a fairly big limitations (at least in my use cases). |
I'll prioritize getting this merged in. |
3 participants
Description
Detect and add as dependency functions that can be used as default.
I'm really not sure I'm not missing some locations where I should process the new
DependsOnFunctionsfield, but it's working for my case ;)Motivation
See #191 for the base case issue, where default value use functions ;)
Testing
Tested locally, tests yet to be added :)