-
Notifications
You must be signed in to change notification settings - Fork 2
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.
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
musicbrainz: Add GraphQL API #11
Conversation
Signed-off-by: Lewis Marshall <lewis@lmars.net>
Missing graphql-go in vendor |
func TestArtistAPI(t *testing.T) { | ||
// create a test index of artists | ||
x, err := newTestIndex() | ||
if err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it ok to reference test code from other _test.go
files?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, as long as it's in the same package.
musicbrainz/api_test.go
Outdated
|
||
// define a function to execute and assert an artist GraphQL query | ||
assertQuery := func(artist *Artist, query string, args ...interface{}) { | ||
data, _ := json.Marshal(map[string]string{"query": fmt.Sprintf(query, args...)}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please consider including a test that retrieves more than one match pr query
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added an explicit multi-result check in 49f41ca.
var err error | ||
switch { | ||
case args.Name != nil: | ||
rows, err = g.db.Query("SELECT object_id FROM artist WHERE name = ?", *args.Name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I specify Name
AND (IPI
AND/OR ISNI
), name takes precedence. Is this desired behavior?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This API accepts either a Name, IPI or ISNI, so this is the desired behaviour yes.
func (g *Resolver) Artist(args artistArgs) ([]*artistResolver, error) { | ||
var rows *sql.Rows | ||
var err error | ||
switch { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SQL Seems to do only exact matching. Is this desired behavior?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This API currently only does exact matching yes. SQLite does support partial matching with LIKE
but we don't require it here.
"database/sql" | ||
"errors" | ||
|
||
"github.com/ipfs/go-cid" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please consider explicitly aliasing to cid
for clarity
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The package name is already cid
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean:
import (
cid "github.com/ipfs/go-cid"
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure why we would want to "alias" the cid
package to cid
when it is already called cid
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If my eyes are not deceiving me, it says go-cid
not cid
.
For all I know, it may be fair go-style to assume automatic truncation of [-]go[-]
in the package name. But I find it unclear. It is only a suggestion, so do as you wish :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok sorry I think I missed the point you were making.
I don't personally think it is worth the work to alias a package with the same name if the last part of the URL doesn't match, because I think imports should just be managed by goimports
, references resolved using godef
, and aliasing reserved for when you actually need to alias something (e.g. importing two packages named math
for example).
That being said, I don't mind doing so if it is helpful to others, so perhaps we should make that change across the codebase in a separate PR?
musicbrainz/graphql.go
Outdated
if err := rows.Scan(&objectID); err != nil { | ||
return nil, err | ||
} | ||
cid, err := cid.Parse(objectID) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please consider using local var name different from package name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed in 47972e0.
artist *Artist | ||
} | ||
|
||
func (a *artistResolver) Cid() string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are some resolver return values pointers and some not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If a field is optional, it must return a pointer so that nil
represents no result (rather than a zero value which could either be no result or a result which is explicitly the zero value).
} | ||
|
||
// store the artists in a test store | ||
x.store = meta.NewStore(datastore.NewMapDatastore()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please consider explicit alias of go-datastore
in import statement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
go-datastore
is not a valid reference in Go, what is wrong with datastore
which is the actual package name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as cid
above
I have not actually run tests since |
Signed-off-by: Lewis Marshall <lewis@lmars.net>
Signed-off-by: Lewis Marshall <lewis@lmars.net>
Signed-off-by: Lewis Marshall <lewis@lmars.net>
@orenyodfat @nolash thanks for the review, the |
Tests pass, thanks. |
This PR adds a GraphQL API for the MusicBrainz META index.
See the README for an overview.