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

The generated Manager.NewConfiguration method is fragile #192

Open
meling opened this issue Dec 11, 2024 · 0 comments
Open

The generated Manager.NewConfiguration method is fragile #192

meling opened this issue Dec 11, 2024 · 0 comments

Comments

@meling
Copy link
Member

meling commented Dec 11, 2024

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.

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

No branches or pull requests

1 participant