Skip to content

Commit ba946ac

Browse files
blktblkt
committed
PR comments
1 parent a398b44 commit ba946ac

File tree

3 files changed

+104
-127
lines changed

3 files changed

+104
-127
lines changed

‎internal/service/db/impl.go‎

Lines changed: 61 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ package database
44
import (
55
"context"
66
"encoding/base64"
7+
"errors"
78
"fmt"
89
"time"
910

@@ -17,6 +18,11 @@ import (
1718
"github.com/stacklok/toolhive-registry-server/internal/service"
1819
)
1920

21+
var (
22+
// ErrBug is returned when a server is not found
23+
ErrBug = errors.New("bug")
24+
)
25+
2026
// options holds configuration options for the database service
2127
type options struct {
2228
pool *pgxpool.Pool
@@ -37,24 +43,6 @@ func WithConnectionPool(pool *pgxpool.Pool) Option {
3743
}
3844
}
3945

40-
// WithConnectionString creates a new database-backed registry service with
41-
// the given connection string.
42-
func WithConnectionString(connString string) Option {
43-
return func(o *options) error {
44-
if connString == "" {
45-
return fmt.Errorf("connection string is required")
46-
}
47-
48-
pool, err := pgxpool.New(context.Background(), connString)
49-
if err != nil {
50-
return fmt.Errorf("failed to create pgx pool: %w", err)
51-
}
52-
53-
o.pool = pool
54-
return nil
55-
}
56-
}
57-
5846
// dbService implements the RegistryService interface using a database backend
5947
type dbService struct {
6048
pool *pgxpool.Pool
@@ -98,7 +86,6 @@ func (s *dbService) ListServers(
9886
ctx context.Context,
9987
opts ...service.Option[service.ListServersOptions],
10088
) ([]*upstreamv0.ServerJSON, error) {
101-
// TODO: implement
10289
options := &service.ListServersOptions{}
10390
for _, opt := range opts {
10491
if err := opt(options); err != nil {
@@ -108,22 +95,24 @@ func (s *dbService) ListServers(
10895

10996
decoded, err := base64.StdEncoding.DecodeString(options.Cursor)
11097
if err != nil {
111-
return nil, err
98+
return nil, fmt.Errorf("invalid cursor format: %w", err)
11299
}
113100
nextTime, err := time.Parse(time.RFC3339, string(decoded))
114101
if err != nil {
115-
return nil, err
102+
return nil, fmt.Errorf("invalid cursor format: %w", err)
116103
}
117104

118-
querierFunc := func(querier sqlc.Querier) ([]helper, error) {
105+
// Note: this function fetches a list of servers. In case no records are
106+
// found, the called function should return an empty slice as it's
107+
// customary in Go.
108+
querierFunc := func(ctx context.Context, querier sqlc.Querier) ([]helper, error) {
119109
servers, err := querier.ListServers(
120110
ctx,
121111
sqlc.ListServersParams{
122112
Next: &nextTime,
123113
Size: int64(options.Limit),
124114
},
125115
)
126-
127116
if err != nil {
128117
return nil, err
129118
}
@@ -151,7 +140,10 @@ func (s *dbService) ListServerVersions(
151140
}
152141
}
153142

154-
querierFunc := func(querier sqlc.Querier) ([]helper, error) {
143+
// Note: this function fetches a list of server versions. In case no records are
144+
// found, the called function should return an empty slice as it's
145+
// customary in Go.
146+
querierFunc := func(ctx context.Context, querier sqlc.Querier) ([]helper, error) {
155147
servers, err := querier.ListServerVersions(
156148
ctx,
157149
sqlc.ListServerVersionsParams{
@@ -188,7 +180,10 @@ func (s *dbService) GetServerVersion(
188180
}
189181
}
190182

191-
querierFunc := func(querier sqlc.Querier) ([]helper, error) {
183+
// Note: this function fetches a single record given name and version.
184+
// In case no record is found, the called function should return an
185+
// `sql.ErrNoRows` error as it's customary in Go.
186+
querierFunc := func(ctx context.Context, querier sqlc.Querier) ([]helper, error) {
192187
server, err := querier.GetServerVersion(
193188
ctx,
194189
sqlc.GetServerVersionParams{
@@ -208,25 +203,62 @@ func (s *dbService) GetServerVersion(
208203
return nil, err
209204
}
210205

206+
// Note: the `queryFunc` function is expected to return an error
207+
// sooner if no records are found, so getting this far with
208+
// a length result slice other than 1 means there's a bug.
209+
if len(res) != 1 {
210+
return nil, fmt.Errorf("%w: number of servers returned is not 1", ErrBug)
211+
}
212+
211213
return res[0], nil
212214
}
213215

216+
// querierFunction is a function that uses the given querier object to run the
217+
// main extraction. As of the time of this writing, its main use is accessing
218+
// the `mcp_server` table in a type-agnostic way. This is to overcome a
219+
// limitation of sqlc that prevents us from having the exact same go type
220+
// despite the fact that the underlying columns returned are the same.
221+
//
222+
// Note that the underlying table does not have to be the `mcp_server` table
223+
// as it is used now, as long as the result is a slice of helpers.
224+
type querierFunction func(ctx context.Context, querier sqlc.Querier) ([]helper, error)
225+
226+
// sharedListServers is a helper function to list servers and mapping them to
227+
// the API schema.
228+
//
229+
// Its responsibilities are:
230+
// * Begin a transaction
231+
// * Execute the querier function
232+
// * List packages and remotes using the server IDs
233+
// * Map the results to the API schema
234+
// * Return the results
235+
//
236+
// The argument `querierFunc` is a function that uses the given querier object
237+
// to run the main extraction. Note that the underlying table does not have
238+
// to be the `mcp_server` table as it is used now, as long as the result is a
239+
// slice of helpers.
214240
func (s *dbService) sharedListServers(
215241
ctx context.Context,
216-
querierFunc func(querier sqlc.Querier) ([]helper, error),
242+
querierFunc querierFunction,
217243
) ([]*upstreamv0.ServerJSON, error) {
218244
tx, err := s.pool.BeginTx(ctx, pgx.TxOptions{
219245
IsoLevel: pgx.ReadCommitted,
220246
AccessMode: pgx.ReadOnly,
221247
})
222248
if err != nil {
223-
return nil, err
249+
return nil, fmt.Errorf("failed to begin transaction: %w", err)
224250
}
225-
defer tx.Rollback(ctx)
251+
defer func() {
252+
err := tx.Rollback(ctx)
253+
if err != nil && !errors.Is(err, pgx.ErrTxClosed) {
254+
// TODO: log the rollback error (add proper logging)
255+
_ = err
256+
}
257+
}()
226258

227259
querier := sqlc.New(tx)
228260

229-
servers, err := querierFunc(querier)
261+
servers, err := querierFunc(ctx, querier)
230262
if err != nil {
231263
return nil, err
232264
}

‎internal/service/db/impl_test.go‎

Lines changed: 2 additions & 73 deletions
Original file line numberDiff line numberDiff line change
@@ -730,77 +730,6 @@ func TestWithConnectionPool(t *testing.T) {
730730
}
731731
}
732732

733-
func TestWithConnectionString(t *testing.T) {
734-
t.Parallel()
735-
736-
tests := []struct {
737-
name string
738-
setupFunc func(*testing.T) string
739-
validateFunc func(*testing.T, error, *options)
740-
}{
741-
{
742-
name: "success with valid connection string",
743-
//nolint:thelper // We want to see these lines in the test output
744-
setupFunc: func(t *testing.T) string {
745-
ctx := context.Background()
746-
db, cleanupFunc := database.SetupTestDB(t)
747-
t.Cleanup(cleanupFunc)
748-
749-
connStr := db.Config().ConnString()
750-
pool, err := pgxpool.New(ctx, connStr)
751-
require.NoError(t, err)
752-
t.Cleanup(func() { pool.Close() })
753-
754-
return connStr
755-
},
756-
//nolint:thelper // We want to see these lines in the test output
757-
validateFunc: func(t *testing.T, err error, o *options) {
758-
require.NoError(t, err)
759-
require.NotNil(t, o.pool)
760-
// Clean up the pool created by the option
761-
if o.pool != nil {
762-
o.pool.Close()
763-
}
764-
},
765-
},
766-
{
767-
name: "failure with empty connection string",
768-
//nolint:thelper // We want to see these lines in the test output
769-
setupFunc: func(_ *testing.T) string {
770-
return ""
771-
},
772-
//nolint:thelper // We want to see these lines in the test output
773-
validateFunc: func(t *testing.T, err error, _ *options) {
774-
require.Error(t, err)
775-
},
776-
},
777-
{
778-
name: "failure with invalid connection string",
779-
//nolint:thelper // We want to see these lines in the test output
780-
setupFunc: func(_ *testing.T) string {
781-
return "invalid-connection-string"
782-
},
783-
//nolint:thelper // We want to see these lines in the test output
784-
validateFunc: func(t *testing.T, err error, _ *options) {
785-
require.Error(t, err)
786-
},
787-
},
788-
}
789-
790-
for _, tt := range tests {
791-
t.Run(tt.name, func(t *testing.T) {
792-
t.Parallel()
793-
794-
connString := tt.setupFunc(t)
795-
opt := WithConnectionString(connString)
796-
o := &options{}
797-
err := opt(o)
798-
799-
tt.validateFunc(t, err, o)
800-
})
801-
}
802-
}
803-
804733
func TestNew(t *testing.T) {
805734
t.Parallel()
806735

@@ -843,7 +772,7 @@ func TestNew(t *testing.T) {
843772
require.NoError(t, err)
844773
t.Cleanup(func() { pool.Close() })
845774

846-
return []Option{WithConnectionString(connStr)}
775+
return []Option{WithConnectionPool(pool)}
847776
},
848777
//nolint:thelper // We want to see these lines in the test output
849778
validateFunc: func(t *testing.T, svc service.RegistryService, err error) {
@@ -855,7 +784,7 @@ func TestNew(t *testing.T) {
855784
name: "failure with invalid connection string",
856785
//nolint:thelper // We want to see these lines in the test output
857786
setupFunc: func(_ *testing.T) []Option {
858-
return []Option{WithConnectionString("invalid-connection-string")}
787+
return []Option{WithConnectionPool(nil)}
859788
},
860789
//nolint:thelper // We want to see these lines in the test output
861790
validateFunc: func(t *testing.T, svc service.RegistryService, err error) {

‎internal/service/db/types.go‎

Lines changed: 41 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package database
33
import (
44
"time"
55

6+
"github.com/aws/smithy-go/ptr"
67
"github.com/google/uuid"
78
upstreamv0 "github.com/modelcontextprotocol/registry/pkg/api/v0"
89
model "github.com/modelcontextprotocol/registry/pkg/model"
@@ -113,29 +114,44 @@ func helperToServer(
113114
server := upstreamv0.ServerJSON{
114115
Schema: "https://static.modelcontextprotocol.io/schemas/2025-10-17/server.schema.json",
115116
Name: dbServer.Name,
116-
Description: *dbServer.Description,
117-
Title: *dbServer.Title,
118-
Repository: &model.Repository{
119-
URL: *dbServer.RepositoryUrl,
120-
Source: *dbServer.RepositoryType,
121-
ID: *dbServer.RepositoryID,
122-
Subfolder: *dbServer.RepositorySubfolder,
123-
},
124-
Version: dbServer.Version,
125-
WebsiteURL: *dbServer.Website,
126-
Packages: toPackages(packages),
127-
Remotes: toRemotes(remotes),
117+
Description: ptr.ToString(dbServer.Description),
118+
Title: ptr.ToString(dbServer.Title),
119+
Version: dbServer.Version,
120+
WebsiteURL: ptr.ToString(dbServer.Website),
121+
Packages: toPackages(packages),
122+
Remotes: toRemotes(remotes),
123+
}
124+
125+
if dbServer.RepositoryUrl != nil {
126+
server.Repository = &model.Repository{
127+
URL: ptr.ToString(dbServer.RepositoryUrl),
128+
Source: ptr.ToString(dbServer.RepositoryType),
129+
ID: ptr.ToString(dbServer.RepositoryID),
130+
Subfolder: ptr.ToString(dbServer.RepositorySubfolder),
131+
}
128132
}
129133

130134
server.Meta = &upstreamv0.ServerMeta{
131135
PublisherProvided: make(map[string]any),
132136
}
133-
server.Meta.PublisherProvided["upstream_meta"] = dbServer.UpstreamMeta
134-
server.Meta.PublisherProvided["server_meta"] = dbServer.ServerMeta
135-
server.Meta.PublisherProvided["repository_url"] = dbServer.RepositoryUrl
136-
server.Meta.PublisherProvided["repository_id"] = dbServer.RepositoryID
137-
server.Meta.PublisherProvided["repository_subfolder"] = dbServer.RepositorySubfolder
138-
server.Meta.PublisherProvided["repository_type"] = dbServer.RepositoryType
137+
if len(dbServer.UpstreamMeta) > 0 {
138+
server.Meta.PublisherProvided["upstream_meta"] = dbServer.UpstreamMeta
139+
}
140+
if len(dbServer.ServerMeta) > 0 {
141+
server.Meta.PublisherProvided["server_meta"] = dbServer.ServerMeta
142+
}
143+
if dbServer.RepositoryUrl != nil {
144+
server.Meta.PublisherProvided["repository_url"] = ptr.ToString(dbServer.RepositoryUrl)
145+
}
146+
if dbServer.RepositoryID != nil {
147+
server.Meta.PublisherProvided["repository_id"] = ptr.ToString(dbServer.RepositoryID)
148+
}
149+
if dbServer.RepositorySubfolder != nil {
150+
server.Meta.PublisherProvided["repository_subfolder"] = ptr.ToString(dbServer.RepositorySubfolder)
151+
}
152+
if dbServer.RepositoryType != nil {
153+
server.Meta.PublisherProvided["repository_type"] = ptr.ToString(dbServer.RepositoryType)
154+
}
139155

140156
return server
141157
}
@@ -150,11 +166,11 @@ func toPackages(
150166
RegistryBaseURL: dbPackage.PkgRegistryUrl,
151167
Identifier: dbPackage.PkgIdentifier,
152168
Version: dbPackage.PkgVersion,
153-
FileSHA256: *dbPackage.Sha256Hash,
154-
RunTimeHint: *dbPackage.RuntimeHint,
169+
FileSHA256: ptr.ToString(dbPackage.Sha256Hash),
170+
RunTimeHint: ptr.ToString(dbPackage.RuntimeHint),
155171
Transport: model.Transport{
156172
Type: dbPackage.Transport,
157-
URL: *dbPackage.TransportUrl,
173+
URL: ptr.ToString(dbPackage.TransportUrl),
158174
Headers: toKeyValueInputs(dbPackage.TransportHeaders),
159175
},
160176
RuntimeArguments: toArguments(dbPackage.RuntimeArguments),
@@ -183,9 +199,9 @@ func toKeyValueInputs(
183199
strings []string,
184200
) []model.KeyValueInput {
185201
result := make([]model.KeyValueInput, len(strings))
186-
for i, string := range strings {
202+
for i, str := range strings {
187203
result[i] = model.KeyValueInput{
188-
Name: string,
204+
Name: str,
189205
}
190206
}
191207
return result
@@ -195,9 +211,9 @@ func toArguments(
195211
strings []string,
196212
) []model.Argument {
197213
result := make([]model.Argument, len(strings))
198-
for i, string := range strings {
214+
for i, str := range strings {
199215
result[i] = model.Argument{
200-
Name: string,
216+
Name: str,
201217
}
202218
}
203219
return result

0 commit comments

Comments
 (0)