Make Columns and Tables Optional #257
Conversation
This allows for Columns to be declared optional via the ClientDatabaseModel. Fields may be marked as optional within the struct tag. This allows a connection when a mapped table (and columns that reference it) aren't available at runtime to support upgrades. To allow a user to easily see which Tables and Columns are supported two new APIs were added. IsTableSupported and IsColumnSupported Signed-off-by: Dave Tucker <dave@dtucker.co.uk>
|
Note: I want to add some more tests before we merge this. |
Pull Request Test Coverage Report for Build 1418141574
💛 - Coveralls |
There was a problem hiding this comment.
Generally, I think this is a good approach. Some general comments:
Two mechanisms are introduced here:
-
Static: Add "omitunsupported" to the model's field tag.
How is the user expected to use this mechanism? Are you planning in adding more logic to modelgen so it can add this tag automatically? (e.g: give it two schemas and insert the tag in those fields that are not the same in both)
If not, are we expecting the user to modify it by hand before compiling? -
Preventive: Add a field to the "optional" map in the to the
ClientDBModel.
I don't see this having any effect at all. It's not checked anywhere AFAICS
That's precisely why I thought "caching" the Metadata objects in themodel.DatabaseModelcould be useful or at least creating them frommodel.DatabaseModel. It can combine the information coming fromClientDBModelinto theInfo.Metadataso thatmapper.Info.FieldByColumnandmapper.Info.SetFieldreflect it
Other thoughts:
- If the column type is different we still fail, right? Should we also omit this case?
- Docs need update
| TableSchema *ovsdb.TableSchema // TableSchema associated | ||
| TableName string // Table name | ||
| Fields map[string]string // Map of ColumnName -> FieldName | ||
| OmittedFields map[string]string // Map of ColumnName -> Empty Struct |
There was a problem hiding this comment.
OmittedFields also stores FieldNames, right?
| column, omit := parseStructTag(field) | ||
| if omit { | ||
| return "", ErrOmitted | ||
| } | ||
| if _, ok := i.Metadata.Fields[column]; !ok { | ||
| return "", fmt.Errorf("field does not have orm column information") |
There was a problem hiding this comment.
Not related but now that you're at this file, would you mind s/orm/mapper/ ? ;-)
| @@ -62,7 +80,11 @@ func (i *Info) ColumnByPtr(fieldPtr interface{}) (string, error) { | |||
| objType := reflect.TypeOf(i.Obj).Elem() | |||
| for j := 0; j < objType.NumField(); j++ { | |||
| if objType.Field(j).Offset == offset { | |||
| column := objType.Field(j).Tag.Get("ovsdb") | |||
| field := objType.Field(j) | |||
| column, omit := parseStructTag(field) | |||
There was a problem hiding this comment.
Why rely on the field tag here and not on i.OmittedFields which actually takes into account whether the schema supports the field?
Having
It should be checked when we do schema validation - we skip any fields that are omitted due to not being supported by the runtime schema. The contract with the user is - "if you use this feature, it's on you to guard against sending bad transactions to ovsdb-server" - as such we don't need to check the list of tables omitted tables again, although we could and provide a better error message I suppose 😆 but that could happen in a follow up PR. |
Beyond |
This allows for Columns to be declared optional via the
ClientDatabaseModel.
Fields may be marked as optional within the struct tag.
This allows a connection when a mapped table (and columns that reference it) aren't available at runtime to support upgrades.
To allow a user to easily see which Tables and Columns are supported two
new APIs were added.
IsTableSupported and IsColumnSupported
Fixes: #235