Skip to content

Define Provider interface and ProviderRegistry #32

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.

Sign up for GitHub

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

Merged
renecannao merged 1 commit into from
Mar 24, 2026
Merged

Define Provider interface and ProviderRegistry #32

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
57 changes: 57 additions & 0 deletions providers/provider.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
package providers

import (
"fmt"
"sort"
)

// Provider is the core abstraction for deploying database infrastructure.
// Phase 2a defines a minimal interface (Name, ValidateVersion, DefaultPorts).
// Phase 2b will add CreateSandbox, Start, Stop, Destroy, HealthCheck when
// ProxySQL and other providers need them.
type Provider interface {
Name() string
ValidateVersion(version string) error
DefaultPorts() PortRange
}

type PortRange struct {
BasePort int
PortsPerInstance int
}

type Registry struct {
providers map[string]Provider
}
Comment on lines +23 to +25
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot Mar 24, 2026

Choose a reason for hiding this comment

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

high

The Registry is not safe for concurrent use. The DefaultRegistry is a global variable, which makes concurrent access from multiple goroutines likely as the application grows. This can lead to race conditions when reading from and writing to the providers map.

To fix this, you should add a sync.RWMutex to the Registry struct and use it to protect access to the providers map in Register, Get, and List methods.

For example:

// In Register
r.mu.Lock()
defer r.mu.Unlock()

// In Get and List
r.mu.RLock()
defer r.mu.RUnlock()

You will also need to add "sync" to your imports.

Suggested change
type Registry struct {
providers map[string]Provider
}
type Registry struct {
mu sync.RWMutex
providers map[string]Provider
}


func NewRegistry() *Registry {
return &Registry{providers: make(map[string]Provider)}
}
Comment on lines +23 to +29
Copy link

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

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

The PR/plan refer to a ProviderRegistry, but the exported type is currently named just Registry. Since this is a public API and will be imported from other packages, the generic name can be ambiguous (e.g., providers.Registry). Consider renaming to ProviderRegistry (and NewProviderRegistry, etc.) or aligning the PR title/docs to match the chosen public type name.

Copilot uses AI. Check for mistakes.

func (r *Registry) Register(p Provider) error {
name := p.Name()
if _, exists := r.providers[name]; exists {
return fmt.Errorf("provider %q already registered", name)
}
r.providers[name] = p
return nil
Comment on lines +31 to +37
Copy link

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

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

Register can panic in two common cases: (1) callers may use the exported Registry zero value (so r.providers is nil and assignment panics), and (2) callers may pass a nil Provider (so p.Name() panics). Consider making the zero value usable by initializing r.providers on first use, and validating inputs (nil provider / empty provider name) with a returned error instead of panicking.

Copilot uses AI. Check for mistakes.
}

func (r *Registry) Get(name string) (Provider, error) {
p, exists := r.providers[name]
if !exists {
return nil, fmt.Errorf("provider %q not found", name)
}
return p, nil
}

func (r *Registry) List() []string {
names := make([]string, 0, len(r.providers))
for name := range r.providers {
names = append(names, name)
}
sort.Strings(names)
return names
}

var DefaultRegistry = NewRegistry()
53 changes: 53 additions & 0 deletions providers/provider_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
package providers

import "testing"

type mockProvider struct{ name string }

func (m *mockProvider) Name() string { return m.name }
func (m *mockProvider) ValidateVersion(version string) error { return nil }
func (m *mockProvider) DefaultPorts() PortRange { return PortRange{BasePort: 9999, PortsPerInstance: 1} }

func TestRegistryRegisterAndGet(t *testing.T) {
reg := NewRegistry()
mock := &mockProvider{name: "test"}
if err := reg.Register(mock); err != nil {
t.Fatalf("Register failed: %v", err)
}
p, err := reg.Get("test")
if err != nil {
t.Fatalf("Get failed: %v", err)
}
if p.Name() != "test" {
t.Errorf("expected name 'test', got %q", p.Name())
}
}

func TestRegistryDuplicateRegister(t *testing.T) {
reg := NewRegistry()
mock := &mockProvider{name: "test"}
_ = reg.Register(mock)
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot Mar 24, 2026

Choose a reason for hiding this comment

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

medium

It's a good practice to check for errors even on the first registration call. While it's unlikely to fail in this test, explicitly checking the error makes the test more robust and clearly states the assumption that the first registration should succeed.

	if err := reg.Register(mock); err != nil {
		t.Fatalf("initial register failed: %v", err)
	}

if err := reg.Register(mock); err == nil {
t.Fatal("expected error on duplicate register")
}
}

Copy link

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

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

Registry tests cover the happy path/duplicate/not-found/list, but they don't cover invalid inputs that are currently possible via the public API (e.g., Register(nil) and registering a provider with an empty name). Adding tests for these cases will lock in the intended behavior once Register is hardened to return errors instead of panicking.

Suggested change
func TestRegistryRegisterNilProvider(t *testing.T) {
reg := NewRegistry()
if err := reg.Register(nil); err == nil {
t.Fatal("expected error when registering nil provider")
}
if names := reg.List(); len(names) != 0 {
t.Fatalf("expected no providers to be registered, got %v", names)
}
}
func TestRegistryRegisterEmptyName(t *testing.T) {
reg := NewRegistry()
mock := &mockProvider{name: ""}
if err := reg.Register(mock); err == nil {
t.Fatal("expected error when registering provider with empty name")
}
if names := reg.List(); len(names) != 0 {
t.Fatalf("expected no providers to be registered, got %v", names)
}
}

Copilot uses AI. Check for mistakes.
func TestRegistryGetNotFound(t *testing.T) {
reg := NewRegistry()
if _, err := reg.Get("nonexistent"); err == nil {
t.Fatal("expected error on missing provider")
}
}

func TestRegistryList(t *testing.T) {
reg := NewRegistry()
_ = reg.Register(&mockProvider{name: "b"})
_ = reg.Register(&mockProvider{name: "a"})
Comment on lines +44 to +45
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot Mar 24, 2026

Choose a reason for hiding this comment

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

medium

It's a good practice to check for errors on registration calls. While it's unlikely to fail in this test, explicitly checking the errors makes the test more robust and clearly states the assumption that these registrations should succeed.

	if err := reg.Register(&mockProvider{name: "b"}); err != nil {
		t.Fatalf("register 'b' failed: %v", err)
	}
	if err := reg.Register(&mockProvider{name: "a"}); err != nil {
		t.Fatalf("register 'a' failed: %v", err)
	}

names := reg.List()
if len(names) != 2 {
t.Errorf("expected 2 providers, got %d", len(names))
}
if names[0] != "a" || names[1] != "b" {
t.Errorf("expected sorted [a b], got %v", names)
}
Comment on lines +47 to +52
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot Mar 24, 2026

Choose a reason for hiding this comment

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

medium

For comparing slices, it's more idiomatic and scalable to use reflect.DeepEqual. This also checks the length, so you can combine the length and content checks into one. This makes the test easier to read and maintain.

You will need to add "reflect" to your imports.

	expected := []string{"a", "b"}
	if !reflect.DeepEqual(names, expected) {
		t.Errorf("expected sorted %v, got %v", expected, names)
	}

}
Loading