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

Fix incorrect model schema parsing for enum type columns #190

Conversation

alexanderConstantinescu
Copy link
Contributor

Enum type columns, such as: LoadBalancer.Protocol was currently parsed
to:

// LoadBalancer defines an object in Load_Balancer table
type LoadBalancer struct {
	Protocol        []LoadBalancerProtocol        `ovsdb:"protocol"`
}

Which is incorrect since the protocol is an enumerated field, not a set
of enumerated fields. This happened because the table schema for
load_balancer specified a max / min field in OVN. The table schema
definition in OVN is a bit dubious since it defines:

protocol":{"type":{"key":{"enum":["set",["sctp","tcp","udp"]],"type":"string"},"min":0}}

but we can work-around such particularities by switching the order at
which we parse the fields.

/assign @dave-tucker

Enum type columns, such as: LoadBalancer.Protocol was currently parsed
to:

```
// LoadBalancer defines an object in Load_Balancer table
type LoadBalancer struct {
	Protocol        []LoadBalancerProtocol        `ovsdb:"protocol"`
}
```
Which is incorrect since the protocol is an enumerated field, not a set
of enumerated fields. This happened because the table schema for
load_balancer specified a max / min field in OVN. The table schema
definition in OVN is a bit dubious since it defines:

```
protocol":{"type":{"key":{"enum":["set",["sctp","tcp","udp"]],"type":"string"},"min":0}}
```

but we can work-around such particularities by switching the order at
which we parse the fields.

Signed-off-by: Alexander Constantinescu <[email protected]>
@amorenoz
Copy link
Collaborator

amorenoz commented Jul 12, 2021

The RFC states:

  If "min" is not 1 or "max" is not 1, or both, and "value" is not
  specified, the type is a set of scalar type "key".

So I think the encoding is correct. It's not the only case btw, enums are very commonly typed as sets so that an empty value is allowed.

@alexanderConstantinescu
Copy link
Contributor Author

The RFC states:

  If "min" is not 1 or "max" is not 1, or both, and "value" is not
  specified, the type is a set of scalar type "key".

So I think the encoding is correct. It's not the only case btw, enums are very commonly typed as sets so that an empty value is allowed.

Thinking a little bit more about this I might have jumped the gun on this PR. I am almost leaning towards saying that the OVN NB DB schema seems wrong: https://github.com/ovn-org/ovn/blob/master/ovn-nb.ovsschema#L172-L175. Thanks for the RFE definition: should probably be:

               "protocol": {
                    "type": {"key": {"type": "string",
                             "enum": ["set", ["tcp", "udp", "sctp"]]},
                             "min": 1, "max": 1}},

based on that.

@dave-tucker
Copy link
Collaborator

Closing as the encoding is correct, but there is no client-side validation (tracked in #160) and the Go type generated for and Enum Set doesn't reflect min/max constraints tracked in #154
@alexanderConstantinescu I'll work on a PR to get the generator to give you a more type that reflects the min = 0, max = 1 constraint.

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.

3 participants