-
Notifications
You must be signed in to change notification settings - Fork 151
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make Columns and Tables Optional #257
base: main
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.DatabaseModel
could be useful or at least creating them frommodel.DatabaseModel
. It can combine the information coming fromClientDBModel
into theInfo.Metadata
so thatmapper.Info.FieldByColumn
andmapper.Info.SetField
reflect 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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