refactor!: make the API harder to misuse #261
Conversation
MarcoPolo
force-pushed
the
marco/multiaddr-refactor
branch
from
c652d4a to
f43a278
Compare
| // validateComponent MUST be called after creating a non-zero Component. | ||
| // It ensures that we will be able to call all methods on Component without | ||
| // error. | ||
| func validateComponent(c Component) (Component, error) { | ||
| _, err := c.valueAndErr() | ||
| if err != nil { | ||
| return Component{}, err | ||
|
|
||
| } | ||
| if c.protocol.Transcoder != nil { | ||
| err = c.protocol.Transcoder.ValidateBytes([]byte(c.bytes[c.offset:])) | ||
| if err != nil { | ||
| return Component{}, err | ||
| } | ||
| } | ||
| return c, nil |
There was a problem hiding this comment.
NIT
I'd move this logic inside newComponent and ensure that all components are created by newComponent.
Currently there are 4 places in code where we need to do validateComponent(component{...}) and possibly all of them can be changed to newComponent(...)
There was a problem hiding this comment.
I only see two usages of validateComponent:
- In
newComponent. - In
readComponent(it happens twice in that method)
These two functions are a bit different. newComponent is expecting a creating a Component from parts, while readComponent is creating a Component from a byte array. Changing these methods to be the same is a bit awkward as Component.bytes is the full bytes representation of the component, so we would introduce a copy somewhere if we did so.
I think it's fine to leave as is.
| addr, err := ma.NewMultiaddr("/ip4/1.2.3.4/tcp/80") | ||
| // err non-nil when parsing failed. | ||
| */ | ||
| type Multiaddr interface { |
| func (m Multiaddr) EncapsulateC(c Component) Multiaddr { | ||
| if c.Empty() { | ||
| return m | ||
| } | ||
| out := make([]Component, 0, len(m)+1) | ||
| out = append(out, m...) | ||
| out = append(out, c) | ||
| return out | ||
| } |
There was a problem hiding this comment.
As the multiaddr is immutable, can we just appent to the existing slice?
There was a problem hiding this comment.
Is the argument to remove this method or change this to append?
The caller is always free to use append directly. This method does a copy on purpose to avoid sharing the underlying slice. If the caller is okay with that they can append directly.
MarcoPolo
changed the title
This seeks to refactor the codebase to make it much harder to hit nil pointer dereference panics.
This takes a different approach to how we've treated multiaddrs in the past. Instead of attempting to make them a general and performant datastructure, we focus on treating them as just an encoding scheme. Users of multiaddrs are expected to parse the multiaddr into some struct that is suitable for their use case, and use the multiaddr form when interoperating. By treating Multiaddrs as just an encoding scheme we can make a number of simplifications in the codebase. Specifically this PR does the following:
Multiaddrinterface.[]ComponentMultiaddrinterface as there is none.Background
This library has had multiple issues related to
Multiaddrbeing an interface. Many methods use and returnnilas the zero value, which behaves poorly when the user forgets to do a nil check on every returned value and attempts to call a method on the nil pointer. For example, usingSplitto split a Multiaddr and then usingJointo rebuild the original Multiaddr historically would panic in case one side of the split was nil. Using an interface also leads to incorrect usages of==to check if two Multiaddrs were equal (would only work for pointer equality) and incorrectly usingMultiaddras a key for a map.Using an interface is typically done to provide a consistent API surface for multiple implementing types. In practice however, the Multiaddr interface was only implemented for
multiaddrandcomponent(with arguably some awkwardness when using a component as a Multiaddr).The better approach is to use a concrete type for a Multiaddr. This lets pointer receiver methods work even if the pointer is nil, since the compiler already knows which function to call. Most methods now take a value rather than a pointer which avoids the issue of a nil pointer dereference completely.
Migration
Refer to ./v015-MIGRATION.md for breaking changes and migration tips