Experiment: FDC ExecuteGraphql and ExecuteGraphqlRead
#716
Conversation
This commit introduces the Firebase Data Connect service to the Go Admin SDK. It adds a new `dataconnect` package with a `Client` for interacting with the Data Connect API. The client supports executing GraphQL queries and mutations via the `ExecuteGraphql` and `ExecuteGraphqlRead` methods. Key features include: - A new `DataConnect` method on the `firebase.App` to get a service client. - Custom error handling to parse GraphQL errors from successful (200 OK) HTTP responses. - A public `IsQueryError` function to check for this specific error type. - Unit tests covering the new functionality. - An integration test demonstrating usage with placeholder values.
jonathanedey
changed the title
ExecuteGraphql and ExecuteGraphqlReadExecuteGraphql and ExecuteGraphqlRead
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for Firebase Data Connect by adding a new dataconnect package and integrating it into the main firebase.App. The implementation is solid and includes both unit and integration tests. My review focuses on improving error handling in the core logic and enhancing the maintainability and reliability of the tests. Specifically, I've suggested using Go's standard error wrapping and pointed out a potential test flakiness issue due to a missing cleanup call.
| } | ||
| func TestExecuteGraphql(t *testing.T) { | ||
| initializeDatabase(t) | ||
| // defer cleanupDatabase(t) |
There was a problem hiding this comment.
The call to cleanupDatabase is commented out. This can lead to test state leaking between tests, causing flakiness and non-deterministic failures. It's crucial for integration tests to clean up after themselves to ensure they are isolated and repeatable. Please uncomment this line to ensure test reliability.
| // defer cleanupDatabase(t) | |
| defer cleanupDatabase(t) |
| } | ||
| if response != nil { | ||
| if err := json.Unmarshal(result.Data, &response); err != nil { | ||
| return fmt.Errorf("error while parsing response: %v", err) |
There was a problem hiding this comment.
When returning an error that wraps another error, it's a Go best practice to use fmt.Errorf with the %w verb. This preserves the original error, allowing callers to inspect it using errors.Is or errors.As. This can be very helpful for debugging and for programmatic error handling by users of the SDK.
| return fmt.Errorf("error while parsing response: %v", err) | |
| return fmt.Errorf("error while parsing response: %w", err) |
| upsertFredUser string = "mutation user { user_upsert(data: {id: \"" + fredUser.ID + "\", address: \"" + fredUser.Address + "\", name: \"" + fredUser.Name + "\"})}" | ||
| upsertJeffUser string = "mutation user { user_upsert(data: {id: \"" + jeffUser.ID + "\", address: \"" + jeffUser.Address + "\", name: \"" + jeffUser.Name + "\"})}" | ||
| upsertFredEmail string = "mutation email {" + "email_upsert(data: {" + "id:\"" + fredEmail.ID + "\"," + "subject: \"" + fredEmail.Subject + "\"," + "date: \"" + fredEmail.Date + "\"," + "text: \"" + fredEmail.Text + "\"," + "fromId: \"" + fredEmail.From.ID + "\"" + "})}" |
There was a problem hiding this comment.
Building query strings using repeated string concatenation is hard to read and maintain, making it difficult to spot errors in the GraphQL syntax.
Consider using fmt.Sprintf with raw string literals to build these strings. This would improve readability and make the code less error-prone. For example:
// At the top of the file:
import "fmt"
// In the var block:
upsertFredUser = fmt.Sprintf(`mutation user { user_upsert(data: {id: "%s", address: "%s", name: "%s"})}`, fredUser.ID, fredUser.Address, fredUser.Name)This would need an import for the fmt package.
1 participant
Experimental output of the port of the Admin Node SDK implementation of the FDC execute GraphQL functions. This PR still contains some general SDK issues but reasonably manually actionable as the final 10-20%.