Skip to content
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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dave-tucker
Copy link
Collaborator

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

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 <[email protected]>
@dave-tucker
Copy link
Collaborator Author

Note: I want to add some more tests before we merge this.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 1418141574

  • 47 of 78 (60.26%) changed or added relevant lines in 8 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.2%) to 72.652%

Changes Missing Coverage Covered Lines Changed/Added Lines %
cache/cache.go 0 3 0.0%
mapper/info.go 30 34 88.24%
client/client.go 0 24 0.0%
Totals Coverage Status
Change from base Build 1399758238: -0.2%
Covered Lines: 4192
Relevant Lines: 5770

💛 - Coveralls

Copy link
Collaborator

@amorenoz amorenoz left a 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 the model.DatabaseModel could be useful or at least creating them from model.DatabaseModel. It can combine the information coming from ClientDBModel into the Info.Metadata so that mapper.Info.FieldByColumn and mapper.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
Copy link
Collaborator

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")
Copy link
Collaborator

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)
Copy link
Collaborator

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?

@dave-tucker
Copy link
Collaborator Author

@amorenoz

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?

Having modelgen do this would be the eventual goal I think, but for now I'm recommending documenting this option and asking users to do it by hand.

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

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.

@flavio-fernandes
Copy link
Contributor

Note: I want to add some more tests before we merge this.

Beyond func TestValidateOptional(t *testing.T) { ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

model: Connecting to DBs that don't contain tables that are in the model should not be an error.
4 participants