Define richer "ColumnsExt" syntax for CRUD base #100
Conversation
Rather than continuing to add per-column flags in a piecemeal fashion, define Columns as a map. Each column may override its select query, add query modifiers (such as joins), and be declared as immutable or read-only. Signed-off-by: Andrew Richardson <[email protected]>
Column definitions may be marked "OmitDefault" at build time, to indicate that they should not be fetched by default. REST APIs may set "ExtraFields" in order to include these columns when desired. Signed-off-by: Andrew Richardson <[email protected]>
awrichar
force-pushed
the
enrichment
branch
from
0018aa5
to
82c7bf1
Compare
awrichar
changed the title
Codecov Report
@@ Coverage Diff @@
## main #100 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 63 63
Lines 5048 5146 +98
=========================================
+ Hits 5048 5146 +98
|
Signed-off-by: Andrew Richardson <[email protected]>
awrichar
force-pushed
the
enrichment
branch
from
89bf23d
to
4c4ab7b
Compare
| Immutable bool // disallow update | ||
| ReadOnly bool // disallow insert and update | ||
| OmitDefault bool // omit from queries unless requested | ||
| QueryModifier QueryModifier |
There was a problem hiding this comment.
I'm not positive if it's correct to have a 1:1 relationship between Column and QueryModifier. I think it makes sense to be able to turn modifiers on and off (since they are usually JOINs). In this design, the use of fields= and extrafields= has the side-effect of turning on the modifiers needed only for those fields. It works very well when you have one optional field to pull in from a related table (such as enriching a linked "id" to a "name").
However, a single JOIN might realistically bring in multiple extra columns... and it's a bit tricky to decide "perform this join if any of these N fields are needed", because we don't have any notion of groups of columns.
There was a problem hiding this comment.
hmmn, so really what we want is to express a join, and the column relationships for the join, and the columns you get from the join.
Then when you ask for fields, you would get them via the join.
| @@ -191,6 +191,15 @@ func (s *Database) FilterUpdate(ctx context.Context, update sq.UpdateBuilder, fi | |||
| return update.Where(fop), nil | |||
| } | |||
|
|
|||
| func (s *Database) aliasColumn(tableName, fieldName string) string { | |||
| switch { | |||
| case tableName == "" || strings.Contains(fieldName, "."): | |||
There was a problem hiding this comment.
I don't particularly like this check for . - but for now I copied it from crud.go. It seems like we may need to propagate a richer notion of tables/fields/joins throughout all these db methods.
| Select string | ||
| Immutable bool // disallow update | ||
| ReadOnly bool // disallow insert and update | ||
| OmitDefault bool // omit from queries unless requested |
There was a problem hiding this comment.
I'm open to naming on this OmitDefault param and its friend ExtraFields. I realize that these are adding some extra complexity in the name of allowing users to tailor the efficiency of the underlying database query - so happy to discuss the trade-offs there too.
There was a problem hiding this comment.
I also have a very slight concern that this flag doesn't get surfaced on OpenAPI. Without extra documentation, I will not know the list of default fields that will be returned, or the list of extra fields I could choose to request.
There was a problem hiding this comment.
Yes - it's a good point, but I'm not sure I have a suggestion for how to do that easily... given the trifecta of ff... struct tags, QueryField definitions, and CRUD object tagging.
| return &namedColumn[T]{Name: colName, Column: *col} | ||
| } | ||
|
|
||
| func (c *CrudBase[T]) getCols() (cols namedColumnList[T]) { |
There was a problem hiding this comment.
This method now builds the column list from both the legacy and new-style definitions. It's called somewhat frequently, and the output should be the same every time (as long as the underlying Columns and ColumnsExt members aren't modified), so I wonder if we should cache it...
There was a problem hiding this comment.
I think if we split the Joins out of the Columns, then we'd make this logic more complicated by needing to bind them by-reference across. Maybe that should work should be joined up with caching.
| @@ -526,18 +659,17 @@ func processGetOpts(ctx context.Context, getOpts []GetOption) (failNotFound bool | |||
| } | |||
|
|
|||
| func (c *CrudBase[T]) GetByID(ctx context.Context, id string, getOpts ...GetOption) (inst T, err error) { | |||
There was a problem hiding this comment.
There's currently no way to pass RequiredFields or ExtraFields when getting by ID or name - so no way to get anything other than the default set of fields. That feels like something that should be added as well.
|
I think this is now stale. We've made a few other enhancements to expose parts of the filter and crud syntax - may revisit an enhanced column definition in the future, but I think it would start from a different place. |
Rather than continuing to add per-column flags in a piecemeal fashion, define Columns as a map. The following concepts have been migrated from top-level CRUD configs to per-column settings (although the original ones will still be honored):
New column features
Each column may override its
Selectquery. This allows selecting complex fields which don't actually map to a single column in this table (such as foreign columns, sub-queries, or data processed by Postgres functions).Each column may specify
OmitDefaultin order to be excluded from queries by default. When querying via REST, such columns can be included by using the existingfields=syntax to choose all fields explicitly, or a newextrafields=syntax to receive a few extra fields along with all the default fields.