From 562f0e5851f7a1613a099405cb43418fbd62387e Mon Sep 17 00:00:00 2001 From: Hector Sanjuan Date: Tue, 25 Mar 2025 23:41:16 +0100 Subject: [PATCH 1/4] ipld/unixfs/io: better initialization and options for Directories WithMaxWidth() -------------- The new option sets effectively a maximum width to the directories. Currently, on a dynamic directory, the switching to-from Basic to HAMT is controlled by HAMTShardingSize, and the width of the HAMT shards by DefaultShardWidth. When WithMaxLinks() is specified, the switching is additionally controlled by the number of links exceeding the limit. In that case, MaxLinks is used as ShardWidth. The directory can only be converted back to BasicDirectory when the total number of links is below the limit. Backwards compatibility is kept and tests have been added. Note that when setting MaxLinks to a high number, it is possible that we still suffer automatic conversion to HAMT before hitting MaxLinks, if the estimated directory size is above 256KiB (as before). WithStat() ---------- Allows to set Stat on a new directory. WithCidBuilder() --------------- Allows to set the CidBuilder on a new directory. --- CHANGELOG.md | 6 + ipld/unixfs/io/directory.go | 310 ++++++++++++++++++++++++++----- ipld/unixfs/io/directory_test.go | 129 ++++++++++--- 3 files changed, 374 insertions(+), 71 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4c9019053..c8a45416f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,12 @@ The following emojis are used to highlight certain changes: ### Added +- `ipld/unixfs/io/directory`: We have made some changes to unixfs directory tooling: + - We have exposed creator methods for new `BasicDirectory` and `HAMTDirectory`, that complement the existing `NewDirectory()` which creates dynamic directories. + - We have added `WithCidBuilder(...)` and `WithMaxLinks(...)` as options to these new methods so that empty directories can be initilized as wished from the get-go. + - `WithMaxLinks(...)` is a new option that allows to set a limit to the number of children that a directory DAG node can have. For details on what exactly it does for each of the directory types, please check the documentation. + - `WithStats(...)` is a new option that allows to set the Stat information for the new directory node. + ### Changed - upgrade to `go-libp2p` [v0.41.1](https://github.com/libp2p/go-libp2p/releases/tag/v0.41.1) diff --git a/ipld/unixfs/io/directory.go b/ipld/unixfs/io/directory.go index edac04b22..e3ec3b093 100644 --- a/ipld/unixfs/io/directory.go +++ b/ipld/unixfs/io/directory.go @@ -4,6 +4,7 @@ import ( "context" "errors" "os" + "time" "github.com/alecthomas/units" mdag "github.com/ipfs/boxo/ipld/merkledag" @@ -69,6 +70,57 @@ type Directory interface { // GetCidBuilder returns the CID Builder used. GetCidBuilder() cid.Builder + + // setMaxLinks sets the max width of the Directory. + setMaxLinks(n int) +} + +type directoryWithOptions interface { + Directory + // setMaxLinks sets the max width of the Directory. + setMaxLinks(n int) + setStats(os.FileMode, time.Time) +} + +type DirectoryOption func(directoryWithOptions) + +// WithMaxLinks stablishes the max number of links allowed for a directory: +// +// - On Dynamic directories using a BasicDirectory, it can trigger conversion +// to HAMT when set and exceeded. The subsequent HAMT nodes will use MaxLinks +// as ShardWidth. Conversion can happen too based on HAMTShardingSize. +// +// - On Dynamic directories using a HAMTDirectory, it can trigger conversion +// to BasicDirectory when the number of links is below MaxLinks (and +// HAMTShardingSize allows). +// +// - On Basic directories, it causes an error when adding more than MaxLinks +// children. +// +// - On HAMT directories, it sets the ShardWidth, otherwise DefaultShardWidth +// is used. +// +// HAMT directories require this value to be a power of 2 and a multiple of 8. +// If it is not the case, it will not be used and DefaultHAMTWidth be used +// instead. +func WithMaxLinks(n int) DirectoryOption { + return func(d directoryWithOptions) { + d.setMaxLinks(n) + } +} + +// WithCidBuilder sets the CidBuilder for new Directories. +func WithCidBuilder(cb cid.Builder) DirectoryOption { + return func(d directoryWithOptions) { + d.SetCidBuilder(cb) + } +} + +// WithStat can be used to set the empty directory node permissions. +func WithStat(mode os.FileMode, mtime time.Time) DirectoryOption { + return func(d directoryWithOptions) { + d.setStats(mode, mtime) + } } // TODO: Evaluate removing `dserv` from this layer and providing it in MFS. @@ -84,6 +136,12 @@ func init() { linksize.LinkSizeFunction = productionLinkSize } +var ( + _ directoryWithOptions = (*DynamicDirectory)(nil) + _ directoryWithOptions = (*BasicDirectory)(nil) + _ directoryWithOptions = (*HAMTDirectory)(nil) +) + // BasicDirectory is the basic implementation of `Directory`. All the entries // are stored in a single node. type BasicDirectory struct { @@ -96,6 +154,14 @@ type BasicDirectory struct { // (We maintain this value up to date even if the HAMTShardingSize is off // since potentially the option could be activated on the fly.) estimatedSize int + totalLinks int + + // opts + // maxNumberOfLinks. If set, can trigger conversion to HAMT directory. + maxLinks int + cidBuilder cid.Builder + mode os.FileMode + mtime time.Time } // HAMTDirectory is the HAMT implementation of `Directory`. @@ -104,30 +170,107 @@ type HAMTDirectory struct { shard *hamt.Shard dserv ipld.DAGService + // opts + maxLinks int + cidBuilder cid.Builder + mode os.FileMode + mtime time.Time + // Track the changes in size by the AddChild and RemoveChild calls // for the HAMTShardingSize option. sizeChange int + totalLinks int } -func newEmptyBasicDirectory(dserv ipld.DAGService) *BasicDirectory { - return newBasicDirectoryFromNode(dserv, format.EmptyDirNode()) +// NewBasicDirectory creates an empty basic directory with the given options. +func NewBasicDirectory(dserv ipld.DAGService, opts ...DirectoryOption) *BasicDirectory { + basicDir := new(BasicDirectory) + basicDir.dserv = dserv + + for _, o := range opts { + o(basicDir) + } + + var node *mdag.ProtoNode + if basicDir.mode > 0 { + node = format.EmptyDirNodeWithStat(basicDir.mode, basicDir.mtime) + } else { + node = format.EmptyDirNode() + } + basicDir.node = node + basicDir.node.SetCidBuilder(basicDir.cidBuilder) + + // Scan node links (if any) to restore estimated size. + basicDir.computeEstimatedSizeAndTotalLinks() + + return basicDir } -func newBasicDirectoryFromNode(dserv ipld.DAGService, node *mdag.ProtoNode) *BasicDirectory { +// NewBasicDirectoryFromNode creates a basic directory wrapping the given +// ProtoNode. +func NewBasicDirectoryFromNode(dserv ipld.DAGService, node *mdag.ProtoNode) *BasicDirectory { basicDir := new(BasicDirectory) basicDir.node = node basicDir.dserv = dserv // Scan node links (if any) to restore estimated size. - basicDir.computeEstimatedSize() + basicDir.computeEstimatedSizeAndTotalLinks() return basicDir } -// NewDirectory returns a Directory implemented by DynamicDirectory -// containing a BasicDirectory that can be converted to a HAMTDirectory. -func NewDirectory(dserv ipld.DAGService) Directory { - return &DynamicDirectory{newEmptyBasicDirectory(dserv)} +// NewHAMTDirectory creates an empty HAMT directory with the given options. +func NewHAMTDirectory(dserv ipld.DAGService, sizeChange int, opts ...DirectoryOption) (*HAMTDirectory, error) { + dir := new(HAMTDirectory) + dir.dserv = dserv + dir.sizeChange = sizeChange + dir.maxLinks = 0 + + for _, opt := range opts { + opt(dir) + } + + shardWidth := DefaultShardWidth + if dir.maxLinks > 0 { + if !validShardWidth(dir.maxLinks) { + return nil, errors.New("MaxLinks must be a power of 2 and multiple of 8") + } + shardWidth = dir.maxLinks + } + + shard, err := hamt.NewShard(dir.dserv, shardWidth) + if err != nil { + return nil, err + } + + shard.SetCidBuilder(dir.cidBuilder) + dir.shard = shard + + return dir, nil +} + +// NewHAMTDirectoryFromNode creates a HAMT directory from the given node, +// which must correspond to an existing HAMT. +func NewHAMTDirectoryFromNode(dserv ipld.DAGService, node ipld.Node) (*HAMTDirectory, error) { + dir := new(HAMTDirectory) + dir.dserv = dserv + dir.sizeChange = 0 + + shard, err := hamt.NewHamtFromDag(dserv, node) + if err != nil { + return nil, err + } + dir.shard = shard + dir.totalLinks = len(node.Links()) + + return dir, nil +} + +// NewDirectory returns a Directory implemented by DynamicDirectory containing +// a BasicDirectory that automatically converts to a from a HAMTDirectory +// based on HAMTShardingSize and MaxLinks (when set). +func NewDirectory(dserv ipld.DAGService, opts ...DirectoryOption) Directory { + return &DynamicDirectory{NewBasicDirectory(dserv, opts...)} } // ErrNotADir implies that the given node was not a unixfs directory @@ -148,23 +291,33 @@ func NewDirectoryFromNode(dserv ipld.DAGService, node ipld.Node) (Directory, err switch fsNode.Type() { case format.TDirectory: - return &DynamicDirectory{newBasicDirectoryFromNode(dserv, protoBufNode.Copy().(*mdag.ProtoNode))}, nil + return &DynamicDirectory{NewBasicDirectoryFromNode(dserv, protoBufNode.Copy().(*mdag.ProtoNode))}, nil case format.THAMTShard: - shard, err := hamt.NewHamtFromDag(dserv, node) + hamtDir, err := NewHAMTDirectoryFromNode(dserv, node) if err != nil { return nil, err } - return &DynamicDirectory{&HAMTDirectory{shard, dserv, 0}}, nil + return &DynamicDirectory{hamtDir}, nil } return nil, ErrNotADir } -func (d *BasicDirectory) computeEstimatedSize() { +func (d *BasicDirectory) setMaxLinks(n int) { + d.maxLinks = n +} + +func (d *BasicDirectory) setStats(mode os.FileMode, mtime time.Time) { + d.mode = mode + d.mtime = mtime +} + +func (d *BasicDirectory) computeEstimatedSizeAndTotalLinks() { d.estimatedSize = 0 // err is just breaking the iteration and we always return nil _ = d.ForEachLink(context.TODO(), func(l *ipld.Link) error { d.addToEstimatedSize(l.Name, l.Cid) + d.totalLinks++ return nil }) // ForEachLink will never fail traversing the BasicDirectory @@ -181,13 +334,16 @@ func (d *BasicDirectory) removeFromEstimatedSize(name string, linkCid cid.Cid) { // Something has gone very wrong. Log an error and recompute the // size from scratch. log.Error("BasicDirectory's estimatedSize went below 0") - d.computeEstimatedSize() + d.computeEstimatedSizeAndTotalLinks() } } // SetCidBuilder implements the `Directory` interface. func (d *BasicDirectory) SetCidBuilder(builder cid.Builder) { - d.node.SetCidBuilder(builder) + d.cidBuilder = builder + if d.node != nil { + d.node.SetCidBuilder(builder) + } } // AddChild implements the `Directory` interface. It adds (or replaces) @@ -219,7 +375,15 @@ func (d *BasicDirectory) needsToSwitchToHAMTDir(name string, nodeToAdd ipld.Node operationSizeChange += linksize.LinkSizeFunction(name, nodeToAdd.Cid()) } - return d.estimatedSize+operationSizeChange >= HAMTShardingSize, nil + switchShardingSize := d.estimatedSize+operationSizeChange >= HAMTShardingSize + switchMaxLinks := false + // We should switch if we have reached maxLinks and a new link is being + // added and maxLinks is valid for shardWidth. + if nodeToAdd != nil && entryToRemove == nil && validShardWidth(d.maxLinks) && + (d.totalLinks+1) > d.maxLinks { + switchMaxLinks = true + } + return switchShardingSize || switchMaxLinks, nil } // addLinkChild adds the link as an entry to this directory under the given @@ -231,12 +395,20 @@ func (d *BasicDirectory) addLinkChild(ctx context.Context, name string, link *ip if err != nil && err != os.ErrNotExist { return err } + if err == nil { // existed + d.totalLinks-- + } + + if d.maxLinks > 0 && d.totalLinks+1 > d.maxLinks { + return errors.New("BasicDirectory: cannot add child: maxLinks reached") + } err = d.node.AddRawLink(name, link) if err != nil { return err } d.addToEstimatedSize(name, link.Cid) + d.totalLinks++ return nil } @@ -304,6 +476,7 @@ func (d *BasicDirectory) RemoveChild(ctx context.Context, name string) error { // The name actually existed so we should update the estimated size. d.removeFromEstimatedSize(link.Name, link.Cid) + d.totalLinks-- return d.node.RemoveNodeLink(name) // GetNodeLink didn't return ErrLinkNotFound so this won't fail with that @@ -321,22 +494,18 @@ func (d *BasicDirectory) GetCidBuilder() cid.Builder { } // switchToSharding returns a HAMT implementation of this directory. -func (d *BasicDirectory) switchToSharding(ctx context.Context) (*HAMTDirectory, error) { - hamtDir := new(HAMTDirectory) - hamtDir.dserv = d.dserv - - shard, err := hamt.NewShard(d.dserv, DefaultShardWidth) +func (d *BasicDirectory) switchToSharding(ctx context.Context, opts ...DirectoryOption) (*HAMTDirectory, error) { + hamtDir, err := NewHAMTDirectory(d.dserv, 0, opts...) if err != nil { return nil, err } - shard.SetCidBuilder(d.node.CidBuilder()) - hamtDir.shard = shard for _, lnk := range d.node.Links() { err = hamtDir.shard.SetLink(ctx, lnk.Name, lnk) if err != nil { return nil, err } + hamtDir.totalLinks++ } return hamtDir, nil @@ -344,7 +513,19 @@ func (d *BasicDirectory) switchToSharding(ctx context.Context) (*HAMTDirectory, // SetCidBuilder implements the `Directory` interface. func (d *HAMTDirectory) SetCidBuilder(builder cid.Builder) { - d.shard.SetCidBuilder(builder) + d.cidBuilder = builder + if d.shard != nil { + d.shard.SetCidBuilder(builder) + } +} + +func (d *HAMTDirectory) setMaxLinks(maxLinks int) { + d.maxLinks = maxLinks +} + +func (d *HAMTDirectory) setStats(mode os.FileMode, mtime time.Time) { + d.mode = mode + d.mtime = mtime } // AddChild implements the `Directory` interface. @@ -358,6 +539,9 @@ func (d *HAMTDirectory) AddChild(ctx context.Context, name string, nd ipld.Node) d.removeFromSizeChange(oldChild.Name, oldChild.Cid) } d.addToSizeChange(name, nd.Cid()) + if oldChild == nil { + d.totalLinks++ + } return nil } @@ -396,6 +580,7 @@ func (d *HAMTDirectory) RemoveChild(ctx context.Context, name string) error { if oldChild != nil { d.removeFromSizeChange(oldChild.Name, oldChild.Cid) + d.totalLinks-- } return nil @@ -412,9 +597,10 @@ func (d *HAMTDirectory) GetCidBuilder() cid.Builder { } // switchToBasic returns a BasicDirectory implementation of this directory. -func (d *HAMTDirectory) switchToBasic(ctx context.Context) (*BasicDirectory, error) { - basicDir := newEmptyBasicDirectory(d.dserv) - basicDir.SetCidBuilder(d.GetCidBuilder()) +func (d *HAMTDirectory) switchToBasic(ctx context.Context, opts ...DirectoryOption) (*BasicDirectory, error) { + // needsToSwichToBasicDir checks d.maxLinks is appropiate. No check is + // performed here. + basicDir := NewBasicDirectory(d.dserv, opts...) err := d.ForEachLink(ctx, func(lnk *ipld.Link) error { err := basicDir.addLinkChild(ctx, lnk.Name, lnk) @@ -472,14 +658,32 @@ func (d *HAMTDirectory) needsToSwitchToBasicDir(ctx context.Context, name string operationSizeChange += linksize.LinkSizeFunction(name, nodeToAdd.Cid()) } - if d.sizeChange+operationSizeChange >= 0 { - // We won't have reduced the HAMT net size. - return false, nil + // We must switch if size and maxlinks are below threshold + canSwitchSize := false + // Directory size reduced, perhaps below limit. + if d.sizeChange+operationSizeChange < 0 { + canSwitchSize, err = d.sizeBelowThreshold(ctx, operationSizeChange) + if err != nil { + return false, err + } } - // We have reduced the directory size, check if went below the - // HAMTShardingSize threshold to trigger a switch. - return d.sizeBelowThreshold(ctx, operationSizeChange) + canSwitchMaxLinks := true + if d.maxLinks > 0 { + total := d.totalLinks + if nodeToAdd != nil && entryToRemove == nil { + total++ + } else if nodeToAdd == nil && entryToRemove != nil { + total-- + } + if total > d.maxLinks { + // prevent switching as we would end up with too many links + canSwitchMaxLinks = false + } + } + + return canSwitchSize && canSwitchMaxLinks, nil + } // Evaluate directory size and a future sizeChange and check if it will be below @@ -535,7 +739,7 @@ func (d *HAMTDirectory) sizeBelowThreshold(ctx context.Context, sizeChange int) // to switch from BasicDirectory to HAMTDirectory and backwards based on // size. type DynamicDirectory struct { - Directory + directoryWithOptions } var _ Directory = (*DynamicDirectory)(nil) @@ -543,7 +747,7 @@ var _ Directory = (*DynamicDirectory)(nil) // AddChild implements the `Directory` interface. We check when adding new entries // if we should switch to HAMTDirectory according to global option(s). func (d *DynamicDirectory) AddChild(ctx context.Context, name string, nd ipld.Node) error { - hamtDir, ok := d.Directory.(*HAMTDirectory) + hamtDir, ok := d.directoryWithOptions.(*HAMTDirectory) if ok { // We evaluate a switch in the HAMTDirectory case even for an AddChild // as it may overwrite an existing entry and end up actually reducing @@ -554,7 +758,7 @@ func (d *DynamicDirectory) AddChild(ctx context.Context, name string, nd ipld.No } if switchToBasic { - basicDir, err := hamtDir.switchToBasic(ctx) + basicDir, err := hamtDir.switchToBasic(ctx, WithMaxLinks(hamtDir.maxLinks), WithCidBuilder(hamtDir.GetCidBuilder())) if err != nil { return err } @@ -562,15 +766,15 @@ func (d *DynamicDirectory) AddChild(ctx context.Context, name string, nd ipld.No if err != nil { return err } - d.Directory = basicDir + d.directoryWithOptions = basicDir return nil } - return d.Directory.AddChild(ctx, name, nd) + return d.directoryWithOptions.AddChild(ctx, name, nd) } // BasicDirectory - basicDir := d.Directory.(*BasicDirectory) + basicDir := d.directoryWithOptions.(*BasicDirectory) switchToHAMT, err := basicDir.needsToSwitchToHAMTDir(name, nd) if err != nil { return err @@ -578,7 +782,16 @@ func (d *DynamicDirectory) AddChild(ctx context.Context, name string, nd ipld.No if !switchToHAMT { return basicDir.AddChild(ctx, name, nd) } - hamtDir, err = basicDir.switchToSharding(ctx) + + maxLinks := DefaultShardWidth + // Verify that our maxLinks is usuable for ShardWidth (power of 2, multiple of 8) + if validShardWidth(basicDir.maxLinks) { + maxLinks = basicDir.maxLinks + } else if basicDir.maxLinks > 0 { + log.Warning("using DefaultShardWidth for HAMT directories. MaxLinks cannot be used as it is not a power of 2 and multiple of 8.") + } + + hamtDir, err = basicDir.switchToSharding(ctx, WithMaxLinks(maxLinks), WithCidBuilder(basicDir.GetCidBuilder())) if err != nil { return err } @@ -586,7 +799,7 @@ func (d *DynamicDirectory) AddChild(ctx context.Context, name string, nd ipld.No if err != nil { return err } - d.Directory = hamtDir + d.directoryWithOptions = hamtDir return nil } @@ -594,9 +807,9 @@ func (d *DynamicDirectory) AddChild(ctx context.Context, name string, nd ipld.No // a HAMTDirectory that might need to be downgraded to a BasicDirectory. The // upgrade path is in AddChild. func (d *DynamicDirectory) RemoveChild(ctx context.Context, name string) error { - hamtDir, ok := d.Directory.(*HAMTDirectory) + hamtDir, ok := d.directoryWithOptions.(*HAMTDirectory) if !ok { - return d.Directory.RemoveChild(ctx, name) + return d.directoryWithOptions.RemoveChild(ctx, name) } switchToBasic, err := hamtDir.needsToSwitchToBasicDir(ctx, name, nil) @@ -608,7 +821,8 @@ func (d *DynamicDirectory) RemoveChild(ctx context.Context, name string) error { return hamtDir.RemoveChild(ctx, name) } - basicDir, err := hamtDir.switchToBasic(ctx) + // We have not removed the element that violates MaxLinks, so we have to +1 the limit. We -1 below. + basicDir, err := hamtDir.switchToBasic(ctx, WithMaxLinks(hamtDir.maxLinks+1), WithCidBuilder(hamtDir.GetCidBuilder())) if err != nil { return err } @@ -616,6 +830,14 @@ func (d *DynamicDirectory) RemoveChild(ctx context.Context, name string) error { if err != nil { return err } - d.Directory = basicDir + + basicDir.setMaxLinks(hamtDir.maxLinks - 1) + d.directoryWithOptions = basicDir return nil } + +// validShardWidth verifies that the given number is positive, a power of 2 +// and a multiple of 8. +func validShardWidth(n int) bool { + return n > 0 && (n&(n-1)) == 0 && n%8 == 0 +} diff --git a/ipld/unixfs/io/directory_test.go b/ipld/unixfs/io/directory_test.go index f69ad0b27..a89ce4865 100644 --- a/ipld/unixfs/io/directory_test.go +++ b/ipld/unixfs/io/directory_test.go @@ -17,7 +17,6 @@ import ( mdag "github.com/ipfs/boxo/ipld/merkledag" mdtest "github.com/ipfs/boxo/ipld/merkledag/test" ft "github.com/ipfs/boxo/ipld/unixfs" - "github.com/ipfs/boxo/ipld/unixfs/hamt" "github.com/ipfs/boxo/ipld/unixfs/internal" "github.com/ipfs/boxo/ipld/unixfs/private/linksize" blocks "github.com/ipfs/go-block-format" @@ -26,6 +25,7 @@ import ( dssync "github.com/ipfs/go-datastore/sync" ipld "github.com/ipfs/go-ipld-format" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestEmptyNode(t *testing.T) { @@ -117,7 +117,7 @@ func TestDuplicateAddDir(t *testing.T) { func TestBasicDirectory_estimatedSize(t *testing.T) { ds := mdtest.Mock() - basicDir := newEmptyBasicDirectory(ds) + basicDir := NewBasicDirectory(ds) testDirectorySizeEstimation(t, basicDir, ds, func(dir Directory) int { return dir.(*BasicDirectory).estimatedSize @@ -126,7 +126,7 @@ func TestBasicDirectory_estimatedSize(t *testing.T) { func TestHAMTDirectory_sizeChange(t *testing.T) { ds := mdtest.Mock() - hamtDir, err := newEmptyHAMTDirectory(ds, DefaultShardWidth) + hamtDir, err := NewHAMTDirectory(ds, 0, WithMaxLinks(DefaultShardWidth)) assert.NoError(t, err) testDirectorySizeEstimation(t, hamtDir, ds, func(dir Directory) int { @@ -205,13 +205,13 @@ func mockLinkSizeFunc(fixedSize int) func(linkName string, linkCid cid.Cid) int } func checkBasicDirectory(t *testing.T, dir Directory, errorMessage string) { - if _, ok := dir.(*DynamicDirectory).Directory.(*BasicDirectory); !ok { + if _, ok := dir.(*DynamicDirectory).directoryWithOptions.(*BasicDirectory); !ok { t.Fatal(errorMessage) } } func checkHAMTDirectory(t *testing.T, dir Directory, errorMessage string) { - if _, ok := dir.(*DynamicDirectory).Directory.(*HAMTDirectory); !ok { + if _, ok := dir.(*DynamicDirectory).directoryWithOptions.(*HAMTDirectory); !ok { t.Fatal(errorMessage) } } @@ -228,7 +228,7 @@ func TestProductionLinkSize(t *testing.T) { assert.Equal(t, 48, productionLinkSize(link.Name, link.Cid)) ds := mdtest.Mock() - basicDir := newEmptyBasicDirectory(ds) + basicDir := NewBasicDirectory(ds) assert.NoError(t, err) for i := 0; i < 10; i++ { basicDir.AddChild(context.Background(), strconv.FormatUint(uint64(i), 10), ft.EmptyFileNode()) @@ -297,8 +297,8 @@ func TestIntegrityOfDirectorySwitch(t *testing.T) { err := ds.Add(ctx, child) assert.NoError(t, err) - basicDir := newEmptyBasicDirectory(ds) - hamtDir, err := newEmptyHAMTDirectory(ds, DefaultShardWidth) + basicDir := NewBasicDirectory(ds) + hamtDir, err := NewHAMTDirectory(ds, 0, WithMaxLinks(DefaultShardWidth)) assert.NoError(t, err) for i := 0; i < 1000; i++ { basicDir.AddChild(ctx, strconv.FormatUint(uint64(i), 10), child) @@ -307,9 +307,9 @@ func TestIntegrityOfDirectorySwitch(t *testing.T) { compareDirectoryEntries(t, basicDir, hamtDir) hamtDirFromSwitch, err := basicDir.switchToSharding(ctx) - assert.NoError(t, err) + require.NoError(t, err) basicDirFromSwitch, err := hamtDir.switchToBasic(ctx) - assert.NoError(t, err) + require.NoError(t, err) compareDirectoryEntries(t, basicDir, basicDirFromSwitch) compareDirectoryEntries(t, hamtDir, hamtDirFromSwitch) } @@ -391,7 +391,7 @@ func TestHAMTEnumerationWhenComputingSize(t *testing.T) { // the HAMTShardingSize threshold. nodesToFetch += thresholdToWidthRatio - hamtDir, err := newHAMTDirectoryFromNode(dsrv, completeHAMTRoot) + hamtDir, err := NewHAMTDirectoryFromNode(dsrv, completeHAMTRoot) assert.NoError(t, err) countGetsDS.resetCounter() @@ -527,27 +527,102 @@ func TestDirBuilder(t *testing.T) { } } -func newHAMTDirectoryFromNode(dserv ipld.DAGService, node ipld.Node) (*HAMTDirectory, error) { - shard, err := hamt.NewHamtFromDag(dserv, node) - if err != nil { - return nil, err +func TestBasicDirectoryWithMaxLinks(t *testing.T) { + ds := mdtest.Mock() + ctx := context.Background() + + dir := NewBasicDirectory(ds, WithMaxLinks(2)) + + child1 := ft.EmptyDirNode() + require.NoError(t, ds.Add(ctx, child1)) + + err := dir.AddChild(ctx, "entry1", child1) + require.NoError(t, err) + + child2 := ft.EmptyDirNode() + require.NoError(t, ds.Add(ctx, child2)) + + err = dir.AddChild(ctx, "entry2", child2) + require.NoError(t, err) + + child3 := ft.EmptyDirNode() + require.NoError(t, ds.Add(ctx, child3)) + + err = dir.AddChild(ctx, "entry3", child3) + require.Error(t, err) +} + +// TestHAMTDirectoryWithMaxLinks tests that no HAMT shard as more than MaxLinks. +func TestHAMTDirectoryWithMaxLinks(t *testing.T) { + ds := mdtest.Mock() + ctx := context.Background() + + dir, err := NewHAMTDirectory(ds, 0, WithMaxLinks(8)) + require.NoError(t, err) + + // Ensure we have at least 2 levels of HAMT by adding many nodes + for i := 0; i < 300; i++ { + child := ft.EmptyDirNode() + require.NoError(t, ds.Add(ctx, child)) + err := dir.AddChild(ctx, fmt.Sprintf("entry%d", i), child) + require.NoError(t, err) + } + + dirnd, err := dir.GetNode() + require.NoError(t, err) + + require.Equal(t, 8, len(dirnd.Links()), "Node should not have more than 8 links") + + for _, l := range dirnd.Links() { + childNd, err := ds.Get(ctx, l.Cid) + if err != nil { + t.Errorf("Failed to get node: %s", err) + continue + } + assert.Equal(t, 8, len(childNd.Links()), "Node does not have 8 links") } - return &HAMTDirectory{ - dserv: dserv, - shard: shard, - }, nil } -func newEmptyHAMTDirectory(dserv ipld.DAGService, shardWidth int) (*HAMTDirectory, error) { - shard, err := hamt.NewShard(dserv, shardWidth) - if err != nil { - return nil, err +func TestDynamicDirectoryWithMaxLinks(t *testing.T) { + ds := mdtest.Mock() + ctx := context.Background() + + dir := NewDirectory(ds, WithMaxLinks(8)) + + for i := 0; i < 8; i++ { + child := ft.EmptyDirNode() + require.NoError(t, ds.Add(ctx, child)) + err := dir.AddChild(ctx, fmt.Sprintf("entry%d", i), child) + require.NoError(t, err) + checkBasicDirectory(t, dir, "directory should be basic because it has less children than MaxLinks") } - return &HAMTDirectory{ - dserv: dserv, - shard: shard, - }, nil + for i := 8; i < 58; i++ { + child := ft.EmptyDirNode() + require.NoError(t, ds.Add(ctx, child)) + err := dir.AddChild(ctx, fmt.Sprintf("entry%d", i), child) + require.NoError(t, err) + checkHAMTDirectory(t, dir, "directory should be sharded because more children than MaxLinks") + } + + // Check that the directory root node has 8 links + dirnd, err := dir.GetNode() + require.NoError(t, err) + assert.Equal(t, 8, len(dirnd.Links()), "HAMT Directory root node should have 8 links") + + // Continue the code by removing 50 elements while checking that the underlying directory is a HAMT directory. + for i := 57; i >= 9; i-- { + err := dir.RemoveChild(ctx, fmt.Sprintf("entry%d", i)) + require.NoError(t, err) + checkHAMTDirectory(t, dir, "directory should still be sharded while more than 8 children") + } + + // Continue removing elements until the directory is a basic directory again + for i := 8; i >= 0; i-- { + err := dir.RemoveChild(ctx, fmt.Sprintf("entry%d", i)) + require.NoError(t, err) + checkBasicDirectory(t, dir, "directory should be basic when less than 8 children") + } } // countGetsDS is a DAG service that keeps track of the number of From 79be477ce86fc5068a22ef8bb263b056f3e3f1a4 Mon Sep 17 00:00:00 2001 From: Hector Sanjuan Date: Thu, 3 Apr 2025 13:14:40 +0200 Subject: [PATCH 2/4] ipld/unixfs/directory: address review from @gammazero --- ipld/unixfs/io/directory.go | 38 +++++++++++++++++--------------- ipld/unixfs/io/directory_test.go | 28 +++++++++++++++++++++++ 2 files changed, 48 insertions(+), 18 deletions(-) diff --git a/ipld/unixfs/io/directory.go b/ipld/unixfs/io/directory.go index e3ec3b093..66e20e940 100644 --- a/ipld/unixfs/io/directory.go +++ b/ipld/unixfs/io/directory.go @@ -221,13 +221,13 @@ func NewBasicDirectoryFromNode(dserv ipld.DAGService, node *mdag.ProtoNode) *Bas // NewHAMTDirectory creates an empty HAMT directory with the given options. func NewHAMTDirectory(dserv ipld.DAGService, sizeChange int, opts ...DirectoryOption) (*HAMTDirectory, error) { - dir := new(HAMTDirectory) - dir.dserv = dserv - dir.sizeChange = sizeChange - dir.maxLinks = 0 + dir := HAMTDirectory{ + dserv: dserv, + sizeChange: sizeChange, + } for _, opt := range opts { - opt(dir) + opt(&dir) } shardWidth := DefaultShardWidth @@ -246,15 +246,15 @@ func NewHAMTDirectory(dserv ipld.DAGService, sizeChange int, opts ...DirectoryOp shard.SetCidBuilder(dir.cidBuilder) dir.shard = shard - return dir, nil + return &dir, nil } // NewHAMTDirectoryFromNode creates a HAMT directory from the given node, // which must correspond to an existing HAMT. func NewHAMTDirectoryFromNode(dserv ipld.DAGService, node ipld.Node) (*HAMTDirectory, error) { - dir := new(HAMTDirectory) - dir.dserv = dserv - dir.sizeChange = 0 + dir := HAMTDirectory{ + dserv: dserv, + } shard, err := hamt.NewHamtFromDag(dserv, node) if err != nil { @@ -263,7 +263,7 @@ func NewHAMTDirectoryFromNode(dserv ipld.DAGService, node ipld.Node) (*HAMTDirec dir.shard = shard dir.totalLinks = len(node.Links()) - return dir, nil + return &dir, nil } // NewDirectory returns a Directory implemented by DynamicDirectory containing @@ -392,10 +392,11 @@ func (d *BasicDirectory) addLinkChild(ctx context.Context, name string, link *ip // Remove old link and account for size change (if it existed; ignore // `ErrNotExist` otherwise). err := d.RemoveChild(ctx, name) - if err != nil && err != os.ErrNotExist { - return err - } - if err == nil { // existed + if err != nil { + if !errors.Is(err, os.ErrNotExist) { + return err + } + } else { // existed d.totalLinks-- } @@ -671,9 +672,10 @@ func (d *HAMTDirectory) needsToSwitchToBasicDir(ctx context.Context, name string canSwitchMaxLinks := true if d.maxLinks > 0 { total := d.totalLinks - if nodeToAdd != nil && entryToRemove == nil { + if nodeToAdd != nil { total++ - } else if nodeToAdd == nil && entryToRemove != nil { + } + if entryToRemove != nil { total-- } if total > d.maxLinks { @@ -788,7 +790,7 @@ func (d *DynamicDirectory) AddChild(ctx context.Context, name string, nd ipld.No if validShardWidth(basicDir.maxLinks) { maxLinks = basicDir.maxLinks } else if basicDir.maxLinks > 0 { - log.Warning("using DefaultShardWidth for HAMT directories. MaxLinks cannot be used as it is not a power of 2 and multiple of 8.") + log.Warn("using DefaultShardWidth for HAMT directories. MaxLinks cannot be used as it is not a power of 2 and multiple of 8.") } hamtDir, err = basicDir.switchToSharding(ctx, WithMaxLinks(maxLinks), WithCidBuilder(basicDir.GetCidBuilder())) @@ -839,5 +841,5 @@ func (d *DynamicDirectory) RemoveChild(ctx context.Context, name string) error { // validShardWidth verifies that the given number is positive, a power of 2 // and a multiple of 8. func validShardWidth(n int) bool { - return n > 0 && (n&(n-1)) == 0 && n%8 == 0 + return n > 0 && (n&(n-1)) == 0 && n&7 == 0 } diff --git a/ipld/unixfs/io/directory_test.go b/ipld/unixfs/io/directory_test.go index a89ce4865..866de794f 100644 --- a/ipld/unixfs/io/directory_test.go +++ b/ipld/unixfs/io/directory_test.go @@ -625,6 +625,34 @@ func TestDynamicDirectoryWithMaxLinks(t *testing.T) { } } +// add a test that tests that validShardWidth(n) only returns true with positive numbers that are powers of 8 and multiples of 8. +func TestValidShardWidth(t *testing.T) { + testCases := []struct { + width int + expect bool + }{ + {0, false}, + {-1, false}, + {1, false}, + {2, false}, + {4, false}, + {8, true}, + {16, true}, + {32, true}, + {64, true}, + {512, true}, + {1024, true}, + {4096, true}, + } + + for _, tc := range testCases { + t.Run(fmt.Sprintf("Width%d", tc.width), func(t *testing.T) { + result := validShardWidth(tc.width) + assert.Equal(t, tc.expect, result, "Expected %v for width %d", tc.expect, tc.width) + }) + } +} + // countGetsDS is a DAG service that keeps track of the number of // unique CIDs fetched. type countGetsDS struct { From 0c402e4167b48e4a17f138c35bf278fcda74e58a Mon Sep 17 00:00:00 2001 From: Hector Sanjuan Date: Fri, 4 Apr 2025 17:45:01 +0200 Subject: [PATCH 3/4] unixfs/directory: Add specific MaxHAMTFanout() option, separate from MaxLinks --- ipld/unixfs/io/directory.go | 138 +++++++++++++++++++------------ ipld/unixfs/io/directory_test.go | 8 +- 2 files changed, 89 insertions(+), 57 deletions(-) diff --git a/ipld/unixfs/io/directory.go b/ipld/unixfs/io/directory.go index 66e20e940..baff71763 100644 --- a/ipld/unixfs/io/directory.go +++ b/ipld/unixfs/io/directory.go @@ -79,33 +79,52 @@ type directoryWithOptions interface { Directory // setMaxLinks sets the max width of the Directory. setMaxLinks(n int) + // used for hamt directories only + setHAMTMaxFanout(n int) setStats(os.FileMode, time.Time) } type DirectoryOption func(directoryWithOptions) -// WithMaxLinks stablishes the max number of links allowed for a directory: +// WithMaxLinks stablishes the max number of links allowed for a Basic +// directory or a Dynamic directory with an underlying Basic directory: // // - On Dynamic directories using a BasicDirectory, it can trigger conversion -// to HAMT when set and exceeded. The subsequent HAMT nodes will use MaxLinks -// as ShardWidth. Conversion can happen too based on HAMTShardingSize. +// to HAMT when set and exceeded. The subsequent HAMT nodes will use +// MaxHAMTFanout as ShardWidth when set, or DefaultShardWidth +// otherwise. Conversion can be triggered too based on HAMTShardingSize. // // - On Dynamic directories using a HAMTDirectory, it can trigger conversion -// to BasicDirectory when the number of links is below MaxLinks (and +// to BasicDirectory when the number of directory entries is below MaxLinks (and // HAMTShardingSize allows). // -// - On Basic directories, it causes an error when adding more than MaxLinks -// children. +// - On pure Basic directories, it causes an error when adding more than +// MaxLinks children. +func WithMaxLinks(n int) DirectoryOption { + return func(d directoryWithOptions) { + d.setMaxLinks(n) + } +} + +// WithMaxHAMTFanout stablishes the maximum fanout factor (number of links) for +// a HAMT directory or a Dynamic directory with an underlying HAMT directory: +// +// - On Dynamic directories, it specifies the HAMT fanout when a HAMT +// used. When unset, DefaultShardWidth applies. // -// - On HAMT directories, it sets the ShardWidth, otherwise DefaultShardWidth +// - On pure HAMT directories, it sets the ShardWidth, otherwise DefaultShardWidth // is used. // -// HAMT directories require this value to be a power of 2 and a multiple of 8. -// If it is not the case, it will not be used and DefaultHAMTWidth be used -// instead. -func WithMaxLinks(n int) DirectoryOption { +// HAMT directories require this value to be a power of 2 and a multiple of +// 8. If it is not the case, it will not be used and DefaultHAMTWidth will be +// used instead. +func WithMaxHAMTFanout(n int) DirectoryOption { + if !validShardWidth(n) { + log.Warnf("Invalid HAMTMaxFanout: %d. Using default (%d)", n, DefaultShardWidth) + n = DefaultShardWidth + } return func(d directoryWithOptions) { - d.setMaxLinks(n) + d.setHAMTMaxFanout(n) } } @@ -158,10 +177,11 @@ type BasicDirectory struct { // opts // maxNumberOfLinks. If set, can trigger conversion to HAMT directory. - maxLinks int - cidBuilder cid.Builder - mode os.FileMode - mtime time.Time + maxLinks int + maxHAMTFanout int + cidBuilder cid.Builder + mode os.FileMode + mtime time.Time } // HAMTDirectory is the HAMT implementation of `Directory`. @@ -171,10 +191,11 @@ type HAMTDirectory struct { dserv ipld.DAGService // opts - maxLinks int - cidBuilder cid.Builder - mode os.FileMode - mtime time.Time + maxLinks int + maxHAMTFanout int + cidBuilder cid.Builder + mode os.FileMode + mtime time.Time // Track the changes in size by the AddChild and RemoveChild calls // for the HAMTShardingSize option. @@ -184,8 +205,10 @@ type HAMTDirectory struct { // NewBasicDirectory creates an empty basic directory with the given options. func NewBasicDirectory(dserv ipld.DAGService, opts ...DirectoryOption) *BasicDirectory { - basicDir := new(BasicDirectory) - basicDir.dserv = dserv + basicDir := &BasicDirectory{ + dserv: dserv, + maxHAMTFanout: DefaultShardWidth, + } for _, o := range opts { o(basicDir) @@ -221,24 +244,17 @@ func NewBasicDirectoryFromNode(dserv ipld.DAGService, node *mdag.ProtoNode) *Bas // NewHAMTDirectory creates an empty HAMT directory with the given options. func NewHAMTDirectory(dserv ipld.DAGService, sizeChange int, opts ...DirectoryOption) (*HAMTDirectory, error) { - dir := HAMTDirectory{ - dserv: dserv, - sizeChange: sizeChange, + dir := &HAMTDirectory{ + dserv: dserv, + sizeChange: sizeChange, + maxHAMTFanout: DefaultShardWidth, } for _, opt := range opts { - opt(&dir) + opt(dir) } - shardWidth := DefaultShardWidth - if dir.maxLinks > 0 { - if !validShardWidth(dir.maxLinks) { - return nil, errors.New("MaxLinks must be a power of 2 and multiple of 8") - } - shardWidth = dir.maxLinks - } - - shard, err := hamt.NewShard(dir.dserv, shardWidth) + shard, err := hamt.NewShard(dir.dserv, dir.maxHAMTFanout) if err != nil { return nil, err } @@ -246,13 +262,13 @@ func NewHAMTDirectory(dserv ipld.DAGService, sizeChange int, opts ...DirectoryOp shard.SetCidBuilder(dir.cidBuilder) dir.shard = shard - return &dir, nil + return dir, nil } // NewHAMTDirectoryFromNode creates a HAMT directory from the given node, // which must correspond to an existing HAMT. func NewHAMTDirectoryFromNode(dserv ipld.DAGService, node ipld.Node) (*HAMTDirectory, error) { - dir := HAMTDirectory{ + dir := &HAMTDirectory{ dserv: dserv, } @@ -263,12 +279,12 @@ func NewHAMTDirectoryFromNode(dserv ipld.DAGService, node ipld.Node) (*HAMTDirec dir.shard = shard dir.totalLinks = len(node.Links()) - return &dir, nil + return dir, nil } // NewDirectory returns a Directory implemented by DynamicDirectory containing // a BasicDirectory that automatically converts to a from a HAMTDirectory -// based on HAMTShardingSize and MaxLinks (when set). +// based on HAMTShardingSize, MaxLinks and MaxHAMTFanout (when set). func NewDirectory(dserv ipld.DAGService, opts ...DirectoryOption) Directory { return &DynamicDirectory{NewBasicDirectory(dserv, opts...)} } @@ -307,6 +323,10 @@ func (d *BasicDirectory) setMaxLinks(n int) { d.maxLinks = n } +func (d *BasicDirectory) setHAMTMaxFanout(n int) { + d.maxHAMTFanout = n +} + func (d *BasicDirectory) setStats(mode os.FileMode, mtime time.Time) { d.mode = mode d.mtime = mtime @@ -377,9 +397,9 @@ func (d *BasicDirectory) needsToSwitchToHAMTDir(name string, nodeToAdd ipld.Node switchShardingSize := d.estimatedSize+operationSizeChange >= HAMTShardingSize switchMaxLinks := false - // We should switch if we have reached maxLinks and a new link is being - // added and maxLinks is valid for shardWidth. - if nodeToAdd != nil && entryToRemove == nil && validShardWidth(d.maxLinks) && + // We should switch if maxLinks is set, we have reached it and a new link is being + // added. + if nodeToAdd != nil && entryToRemove == nil && d.maxLinks > 0 && (d.totalLinks+1) > d.maxLinks { switchMaxLinks = true } @@ -520,8 +540,12 @@ func (d *HAMTDirectory) SetCidBuilder(builder cid.Builder) { } } -func (d *HAMTDirectory) setMaxLinks(maxLinks int) { - d.maxLinks = maxLinks +func (d *HAMTDirectory) setMaxLinks(n int) { + d.maxLinks = n +} + +func (d *HAMTDirectory) setHAMTMaxFanout(n int) { + d.maxHAMTFanout = n } func (d *HAMTDirectory) setStats(mode os.FileMode, mtime time.Time) { @@ -599,7 +623,7 @@ func (d *HAMTDirectory) GetCidBuilder() cid.Builder { // switchToBasic returns a BasicDirectory implementation of this directory. func (d *HAMTDirectory) switchToBasic(ctx context.Context, opts ...DirectoryOption) (*BasicDirectory, error) { - // needsToSwichToBasicDir checks d.maxLinks is appropiate. No check is + // needsoSwichToBasicDir checks d.maxLinks is appropiate. No check is // performed here. basicDir := NewBasicDirectory(d.dserv, opts...) @@ -760,7 +784,7 @@ func (d *DynamicDirectory) AddChild(ctx context.Context, name string, nd ipld.No } if switchToBasic { - basicDir, err := hamtDir.switchToBasic(ctx, WithMaxLinks(hamtDir.maxLinks), WithCidBuilder(hamtDir.GetCidBuilder())) + basicDir, err := hamtDir.switchToBasic(ctx, WithMaxLinks(hamtDir.maxLinks), WithMaxHAMTFanout(hamtDir.maxHAMTFanout), WithCidBuilder(hamtDir.GetCidBuilder())) if err != nil { return err } @@ -785,15 +809,13 @@ func (d *DynamicDirectory) AddChild(ctx context.Context, name string, nd ipld.No return basicDir.AddChild(ctx, name, nd) } - maxLinks := DefaultShardWidth + hamtFanout := DefaultShardWidth // Verify that our maxLinks is usuable for ShardWidth (power of 2, multiple of 8) - if validShardWidth(basicDir.maxLinks) { - maxLinks = basicDir.maxLinks - } else if basicDir.maxLinks > 0 { - log.Warn("using DefaultShardWidth for HAMT directories. MaxLinks cannot be used as it is not a power of 2 and multiple of 8.") + if validShardWidth(basicDir.maxHAMTFanout) { + hamtFanout = basicDir.maxHAMTFanout } - hamtDir, err = basicDir.switchToSharding(ctx, WithMaxLinks(maxLinks), WithCidBuilder(basicDir.GetCidBuilder())) + hamtDir, err = basicDir.switchToSharding(ctx, WithMaxHAMTFanout(hamtFanout), WithMaxLinks(basicDir.maxLinks), WithCidBuilder(basicDir.GetCidBuilder())) if err != nil { return err } @@ -823,17 +845,25 @@ func (d *DynamicDirectory) RemoveChild(ctx context.Context, name string) error { return hamtDir.RemoveChild(ctx, name) } + maxLinks := hamtDir.maxLinks // We have not removed the element that violates MaxLinks, so we have to +1 the limit. We -1 below. - basicDir, err := hamtDir.switchToBasic(ctx, WithMaxLinks(hamtDir.maxLinks+1), WithCidBuilder(hamtDir.GetCidBuilder())) + if maxLinks > 0 { + maxLinks++ + } + + basicDir, err := hamtDir.switchToBasic(ctx, WithMaxLinks(maxLinks), WithCidBuilder(hamtDir.GetCidBuilder())) if err != nil { return err } + err = basicDir.RemoveChild(ctx, name) if err != nil { return err } - basicDir.setMaxLinks(hamtDir.maxLinks - 1) + if maxLinks > 0 { + basicDir.maxLinks-- + } d.directoryWithOptions = basicDir return nil } diff --git a/ipld/unixfs/io/directory_test.go b/ipld/unixfs/io/directory_test.go index 866de794f..f5301b8c8 100644 --- a/ipld/unixfs/io/directory_test.go +++ b/ipld/unixfs/io/directory_test.go @@ -205,12 +205,14 @@ func mockLinkSizeFunc(fixedSize int) func(linkName string, linkCid cid.Cid) int } func checkBasicDirectory(t *testing.T, dir Directory, errorMessage string) { + t.Helper() if _, ok := dir.(*DynamicDirectory).directoryWithOptions.(*BasicDirectory); !ok { t.Fatal(errorMessage) } } func checkHAMTDirectory(t *testing.T, dir Directory, errorMessage string) { + t.Helper() if _, ok := dir.(*DynamicDirectory).directoryWithOptions.(*HAMTDirectory); !ok { t.Fatal(errorMessage) } @@ -557,7 +559,7 @@ func TestHAMTDirectoryWithMaxLinks(t *testing.T) { ds := mdtest.Mock() ctx := context.Background() - dir, err := NewHAMTDirectory(ds, 0, WithMaxLinks(8)) + dir, err := NewHAMTDirectory(ds, 0, WithMaxHAMTFanout(8)) require.NoError(t, err) // Ensure we have at least 2 levels of HAMT by adding many nodes @@ -587,7 +589,7 @@ func TestDynamicDirectoryWithMaxLinks(t *testing.T) { ds := mdtest.Mock() ctx := context.Background() - dir := NewDirectory(ds, WithMaxLinks(8)) + dir := NewDirectory(ds, WithMaxLinks(8), WithMaxHAMTFanout(16)) for i := 0; i < 8; i++ { child := ft.EmptyDirNode() @@ -608,7 +610,7 @@ func TestDynamicDirectoryWithMaxLinks(t *testing.T) { // Check that the directory root node has 8 links dirnd, err := dir.GetNode() require.NoError(t, err) - assert.Equal(t, 8, len(dirnd.Links()), "HAMT Directory root node should have 8 links") + assert.Equal(t, 16, len(dirnd.Links()), "HAMT Directory root node should have 16 links") // Continue the code by removing 50 elements while checking that the underlying directory is a HAMT directory. for i := 57; i >= 9; i-- { From bf08968aae3898b7d5f196b19f5a4c2e2209a01a Mon Sep 17 00:00:00 2001 From: Hector Sanjuan Date: Fri, 4 Apr 2025 18:49:06 +0200 Subject: [PATCH 4/4] unixfs/directory: Let 0 be default for HAMTFanout without warning. --- ipld/unixfs/io/directory.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/ipld/unixfs/io/directory.go b/ipld/unixfs/io/directory.go index baff71763..7858bd9ef 100644 --- a/ipld/unixfs/io/directory.go +++ b/ipld/unixfs/io/directory.go @@ -119,10 +119,13 @@ func WithMaxLinks(n int) DirectoryOption { // 8. If it is not the case, it will not be used and DefaultHAMTWidth will be // used instead. func WithMaxHAMTFanout(n int) DirectoryOption { - if !validShardWidth(n) { + if n > 0 && !validShardWidth(n) { log.Warnf("Invalid HAMTMaxFanout: %d. Using default (%d)", n, DefaultShardWidth) n = DefaultShardWidth } + if n == 0 { + n = DefaultShardWidth + } return func(d directoryWithOptions) { d.setHAMTMaxFanout(n) }