The generated Manager.NewConfiguration method is fragile #192
Description
The current version of NewConfiguration takes only a single variadic whose type is an empty interface. This makes the code fragile and it can break at runtime if passed an unrecognized type.
Below is a rewrite that should prevent the problem of passing an unrecognized type. Although, you can still "forget" to pass an implementation a QuorumSpec interface, which would break at runtime. Ideally, this should also be detected at compile time.
// NewConfiguration returns a configuration based on the provided list of nodes and
// an optional quorum specification. The QuorumSpec is necessary for call types that
// process replies. For configurations only used for unicast or multicast call types,
// a QuorumSpec is not needed. The QuorumSpec interface is also a ConfigOption.
// Nodes can be supplied using WithNodeMap or WithNodeList, or WithNodeIDs.
// A new configuration can also be created from an existing configuration,
// using the And, WithNewNodes, Except, and WithoutNodes methods.
func (m *Manager) NewConfiguration(cfg gorums.NodeListOption, qspec ...QuorumSpec) (c *Configuration, err error) {
if len(qspec) > 1 {
return nil, fmt.Errorf("config: wrong number of options: %d", len(qspec))
}
if len(qspec) == 0 {
// return an error if the QuorumSpec interface is not empty and no implementation was provided.
var test interface{} = struct{}{}
if _, empty := test.(QuorumSpec); !empty {
return nil, fmt.Errorf("config: missing required QuorumSpec for quorum calls")
}
}
c = &Configuration{}
if len(qspec) == 1 {
c.qspec = qspec[0]
}
c.RawConfiguration, err = gorums.NewRawConfiguration(m.RawManager, cfg)
if err != nil {
return nil, err
}
// initialize the nodes slice
c.nodes = make([]*Node, c.Size())
for i, n := range c.RawConfiguration {
c.nodes[i] = &Node{n}
}
return c, nil
}Another solution would be to generate different NewConfiguration signatures for the cases where the QuorumSpec is not needed (one-way communications like unicast and multicast). We would still have only one implementation in the generated code, but they would have different signatures. Alternatively, we could keep the same signature in all cases but make the QuorumSpec an empty interface for the one-way cases.
func (m *Manager) NewConfiguration(cfg gorums.NodeListOption, qspec QuorumSpec) (c *Configuration, err error) {
if qspec == nil {
return nil, fmt.Errorf("config: QuorumSpec cannot be nil")
}
c = &Configuration{}
c.qspec = qspec
c.RawConfiguration, err = gorums.NewRawConfiguration(m.RawManager, cfg)
...
}
func (m *Manager) NewConfiguration(cfg gorums.NodeListOption) (c *Configuration, err error) {
c = &Configuration{}
c.RawConfiguration, err = gorums.NewRawConfiguration(m.RawManager, cfg)
...
}All of these solutions would break compatibility with existing code. Most uses of NewConfiguration actually pass the QuorumSpec as the first argument (but some does it the other way around). So either way, we will break someone. Since we haven't made an official release of version 1.0 yet, and I'm not aware of any production uses, I think it should be fine to do one of the above changes. Fixing it is trivial by swapping the order of the arguments.