Skip to content
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions drivers/pg/driver.go
Original file line number Diff line number Diff line change
Expand Up @@ -176,3 +176,13 @@ func (s *Driver) RefreshKinds(ctx context.Context) error {
s.SchemaManager.kindIDsByKind = map[int16]graph.Kind{}
return s.SchemaManager.Fetch(ctx)
}

func (s *Driver) Optimize(ctx context.Context, opts ...graph.OptimizeOption) error {
config := &graph.OptimizeConfig{}
for _, opt := range opts {
opt(config)
}

// PostgreSQL VACUUM/ANALYZE implementation
return nil
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

No-op Optimize silently reports success.

The method builds config from the options but discards it and returns nil without performing any work. This is fine as an intentional placeholder, but as written it advertises the StorageMaintainer capability while doing nothing, so callers cannot distinguish "optimized" from "not yet implemented." Consider marking the placeholder as a TODO so it surfaces in tracking and isn't mistaken for a completed implementation.

📝 Suggested placeholder marker
-	// PostgreSQL VACUUM/ANALYZE implementation
-	return nil
+	// TODO(BED-8353): implement PostgreSQL VACUUM/ANALYZE using config.Targets.
+	// A nil/empty config.Targets means "optimize every region" per graph.OptimizeConfig.
+	return nil

Want me to open a follow-up issue to track the VACUUM/ANALYZE implementation?

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@drivers/pg/driver.go` around lines 180 - 188, The Optimize method currently
builds graph.OptimizeConfig but returns nil (no-op) which falsely signals
success; update Driver.Optimize to mark it as an explicit placeholder by adding
a TODO comment and returning a clear not-implemented error (e.g.,
errors.New("Driver.Optimize: not implemented") or a package-specific
ErrNotImplemented) instead of nil, and ensure any capability advertising
(StorageMaintainer) is removed or gated until the VACUUM/ANALYZE implementation
using graph.OptimizeConfig is completed so callers can distinguish implemented
vs placeholder behavior.

57 changes: 57 additions & 0 deletions graph/optimize.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
package graph

import (
"context"
)

// TargetStorage identifies a region of graph storage that an optimization
// pass may target.
type TargetStorage int

const (
Nodes TargetStorage = iota
Relationships
)

// OptimizeConfig is the resolved configuration for a single Optimize call.
// Drivers apply every OptimizeOption to this struct before reading it.
type OptimizeConfig struct {
// Targets is the set of storage regions to optimize. A nil or empty
// slice instructs the driver to optimize every region it knows about.
Targets []TargetStorage
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opinion: While convenient, an empty slice representing "all" supported storage targets may be misleading

}

// OptimizeOption mutates an OptimizeConfig and is applied in the order
// passed to Optimize.
type OptimizeOption func(*OptimizeConfig)

// OptimizeTargets restricts the optimization pass to the given storage
// regions. Repeated calls append targets rather than replacing them.
// Calling OptimizeTargets with no arguments does not change the resolved
// target set; if no targets are configured by the time Optimize runs, the
// driver treats that as "all regions it knows about".
func OptimizeTargets(targets ...TargetStorage) OptimizeOption {
return func(c *OptimizeConfig) {
c.Targets = append(c.Targets, targets...)
}
}

// StorageOptimizer is an optional capability implemented by drivers that
// can perform storage optimization. Drivers that cannot perform meaningful optimization
// must not implement this interface; a failed type assertion is the
// signal for "this driver does not support storage optimization".
// A non-nil error returned from Optimize signals a driver-specific failure
// during a supported call.
//
// Consumers detect support with a type assertion against a graph.Database:
//
// if m, ok := db.(graph.StorageOptimizer); ok {
// err := m.Optimize(ctx, graph.OptimizeTargets(graph.Nodes, graph.Relationships))
// ...
// }
type StorageOptimizer interface {
// OptimizeStorage runs a storage optimization pass against the regions
// identified by opts. With no options, every region the driver
// recognizes is optimized.
OptimizeStorage(ctx context.Context, opts ...OptimizeOption) error
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Incomplete rename: doc/example still reference the old Optimize method.

The interface method is now OptimizeStorage, but the doc comments and the consumer example were not fully updated:

  • Line 49 example calls m.Optimize(...), which no longer exists on StorageOptimizer — anyone copying this snippet will fail to compile.
  • Prose references to "Optimize" remain at lines 16, 25, 31, and 43.
📝 Proposed fix for the example call
 //	if m, ok := db.(graph.StorageOptimizer); ok {
-//	    err := m.Optimize(ctx, graph.OptimizeTargets(graph.Nodes, graph.Relationships))
+//	    err := m.OptimizeStorage(ctx, graph.OptimizeTargets(graph.Nodes, graph.Relationships))
 //	    ...
 //	}

Also update the prose references at lines 16, 25, 31, and 43 from "Optimize" to "OptimizeStorage" for consistency.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// A non-nil error returned from Optimize signals a driver-specific failure
// during a supported call.
//
// Consumers detect support with a type assertion against a graph.Database:
//
// if m, ok := db.(graph.StorageOptimizer); ok {
// err := m.Optimize(ctx, graph.OptimizeTargets(graph.Nodes, graph.Relationships))
// ...
// }
type StorageOptimizer interface {
// OptimizeStorage runs a storage optimization pass against the regions
// identified by opts. With no options, every region the driver
// recognizes is optimized.
OptimizeStorage(ctx context.Context, opts ...OptimizeOption) error
// A non-nil error returned from Optimize signals a driver-specific failure
// during a supported call.
//
// Consumers detect support with a type assertion against a graph.Database:
//
// if m, ok := db.(graph.StorageOptimizer); ok {
// err := m.OptimizeStorage(ctx, graph.OptimizeTargets(graph.Nodes, graph.Relationships))
// ...
// }
type StorageOptimizer interface {
// OptimizeStorage runs a storage optimization pass against the regions
// identified by opts. With no options, every region the driver
// recognizes is optimized.
OptimizeStorage(ctx context.Context, opts ...OptimizeOption) error
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@graph/optimize.go` around lines 43 - 56, Update the documentation and example
to use the new method name OptimizeStorage everywhere instead of the old
Optimize: change the consumer example to call m.OptimizeStorage(ctx,
graph.OptimizeTargets(...)) and update all prose references to "Optimize"
(including lines mentioning the interface and behavior) to "OptimizeStorage";
ensure the interface name StorageOptimizer and the parameter type OptimizeOption
remain unchanged and the example still shows the type assertion against
graph.Database and the correct ctx and options usage.

}
54 changes: 54 additions & 0 deletions graph/optimize_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
package graph_test

import (
"testing"

"github.com/specterops/dawgs/graph"
"github.com/stretchr/testify/assert"
)

func TestOptimizeOptions(t *testing.T) {
t.Parallel()

tests := []struct {
name string
opts []graph.OptimizeOption
want []graph.TargetStorage
}{
{
name: "no options yields nil Targets",
opts: nil,
want: nil,
},
{
name: "OptimizeTargets with no args leaves Targets nil",
opts: []graph.OptimizeOption{graph.OptimizeTargets()},
want: nil,
},
{
name: "single call preserves order",
opts: []graph.OptimizeOption{graph.OptimizeTargets(graph.Nodes, graph.Relationships)},
want: []graph.TargetStorage{graph.Nodes, graph.Relationships},
},
{
name: "repeated calls accumulate",
opts: []graph.OptimizeOption{
graph.OptimizeTargets(graph.Nodes),
graph.OptimizeTargets(graph.Relationships),
},
want: []graph.TargetStorage{graph.Nodes, graph.Relationships},
},
}

for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
t.Parallel()

var cfg graph.OptimizeConfig
for _, o := range tc.opts {
o(&cfg)
}
assert.Equal(t, tc.want, cfg.Targets)
})
}
}
Loading