fix(server): pagination keyword fix #1289
Conversation
WalkthroughThe changes in this pull request introduce several new functions and modify existing functionalities in the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
✅ Deploy Preview for reearth-web canceled.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (5)
server/e2e/common.go (2)
246-267: Document and optimize the JSONEqRegexp functionThe function performs multiple JSON conversions and has special character handling that could be improved.
Consider the following improvements:
+// JSONEqRegexp compares two JSON strings using regular expressions. +// It normalizes both strings through marshal/unmarshal cycles to ensure +// consistent formatting before comparison. func JSONEqRegexp(t *testing.T, rx string, str string) bool { + // Helper function to normalize JSON + normalizeJSON := func(s string) (string, error) { + var m map[string]interface{} + if err := json.Unmarshal([]byte(s), &m); err != nil { + return "", err + } + b, err := json.Marshal(m) + if err != nil { + return "", err + } + return string(b), nil + } - var rx1 map[string]interface{} - err := json.Unmarshal([]byte(rx), &rx1) - assert.Nil(t, err) - - rx2, err := json.Marshal(rx1) - assert.Nil(t, err) - - var str1 map[string]interface{} - err = json.Unmarshal([]byte(str), &str1) - assert.Nil(t, err) - - str2, err := json.Marshal(str1) - assert.Nil(t, err) + normalizedRx, err := normalizeJSON(rx) + if err != nil { + t.Fatalf("failed to normalize regex JSON: %v", err) + } + + normalizedStr, err := normalizeJSON(str) + if err != nil { + t.Fatalf("failed to normalize string JSON: %v", err) + } + + // Escape all regex special characters, not just '[' + rxPattern := regexp.QuoteMeta(normalizedRx) + // Unescape any intentional regex patterns (if needed) + rxPattern = strings.ReplaceAll(rxPattern, "\\*", "*") return assert.Regexp( t, - regexp.MustCompile(strings.ReplaceAll(string(rx2), "[", "\\[")), - string(str2), + regexp.MustCompile(rxPattern), + normalizedStr, ) }
Line range hint
1-267: Consider architectural improvements for test utilitiesWhile the new test utilities provide good support for pagination and keyword search testing, consider the following architectural improvements:
- Create a TestServer struct to encapsulate server configuration and common utilities
- Add middleware support for common headers and response assertions
- Consider using test fixtures for common test data
- Add logging/debugging support for test failures
Example structure:
type TestServer struct { t *testing.T expect *httpexpect.Expect config *config.Config } func NewTestServer(t *testing.T, opts ...Option) *TestServer { // Initialize with default config // Apply options // Start server } func (s *TestServer) Request() *RequestBuilder { // Fluent API for building requests with common defaults } func (s *TestServer) WithLanguage(lang language.Tag) *TestServer { // Configure language }server/e2e/gql_project_test.go (3)
267-321: Consider moving the large query string to an external file for better maintainabilityEmbedding large multi-line strings directly in code can reduce readability. Consider moving
GetProjectsQueryto an external.graphqlfile or a test fixture and loading it during tests to improve maintainability.
577-582: Consider refactoring repeated configuration setup into a helper functionThe configuration setup code is repeated in multiple test functions. Extracting this into a helper function can reduce code duplication and improve maintainability.
655-660: Consider refactoring repeated configuration setup into a helper functionThe configuration setup code is repeated in multiple test functions. Extracting this into a helper function can reduce code duplication and improve maintainability.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
server/go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
server/e2e/common.go(3 hunks)server/e2e/gql_project_test.go(4 hunks)server/go.mod(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- server/go.mod
🔇 Additional comments (4)
server/e2e/common.go (2)
Line range hint 5-28: LGTM: New imports are appropriate
The added imports are well-chosen and support the new testing functionality being introduced.
203-244: 🛠️ Refactor suggestion
Refactor request functions to reduce duplication and improve error handling
The request handling functions have several areas for improvement:
- Common headers are duplicated across functions
- Error handling is inconsistent
- HTTP status expectations are hard-coded
Consider refactoring to reduce duplication and improve error handling:
+// commonHeaders adds standard headers used across all requests
+func commonHeaders(req *httpexpect.Request, user string) *httpexpect.Request {
+ return req.
+ WithHeader("Origin", "https://example.com").
+ WithHeader("authorization", "Bearer test").
+ WithHeader("X-Reearth-Debug-User", user).
+ WithHeader("Content-Type", "application/json")
+}
func Request(e *httpexpect.Expect, user string, requestBody GraphQLRequest) *httpexpect.Value {
- return e.POST("/api/graphql").
- WithHeader("Origin", "https://example.com").
- WithHeader("authorization", "Bearer test").
- WithHeader("X-Reearth-Debug-User", user).
- WithHeader("Content-Type", "application/json").
+ return commonHeaders(e.POST("/api/graphql"), user).
WithJSON(requestBody).
Expect().
Status(http.StatusOK).
JSON()
}
func RequestWithMultipart(e *httpexpect.Expect, user string, requestBody map[string]interface{}, filePath string) *httpexpect.Value {
jsonData, err := json.Marshal(requestBody)
+ if err != nil {
+ t.Fatalf("failed to marshal request body: %v", err)
+ }
- return e.POST("/api/graphql").
- WithHeader("Origin", "https://example.com").
- WithHeader("authorization", "Bearer test").
- WithHeader("X-Reearth-Debug-User", user).
+ return commonHeaders(e.POST("/api/graphql"), user).
WithMultipart().
WithFormField("operations", string(jsonData)).
WithFormField("map", `{"0": ["variables.file"]}`).
WithFile("0", filePath).
Expect().
Status(http.StatusOK).
JSON()
}Also, verify that the user parameter is valid:
server/e2e/gql_project_test.go (2)
4-16: Imports added are appropriate and necessary
The newly added import statements are required for the additional functionalities and are properly organized.
365-365: Use of GetProjectsQuery constant enhances code maintainability
Replacing inline query strings with the GetProjectsQuery constant reduces redundancy and improves code readability.
| func Server(t *testing.T, seeder Seeder) *httpexpect.Expect { | ||
| c := &config.Config{ | ||
| Origins: []string{"https://example.com"}, | ||
| AuthSrv: config.AuthSrvConfig{ | ||
| Disabled: true, | ||
| }, | ||
| } | ||
| return StartServer(t, c, true, seeder) | ||
| } | ||
|
|
||
| func ServerLanguage(t *testing.T, lang language.Tag) *httpexpect.Expect { | ||
| c := &config.Config{ | ||
| Origins: []string{"https://example.com"}, | ||
| AuthSrv: config.AuthSrvConfig{ | ||
| Disabled: true, | ||
| }, | ||
| } | ||
| return StartServer(t, c, true, | ||
| func(ctx context.Context, r *repo.Container) error { | ||
| return baseSeederWithLang(ctx, r, lang) | ||
| }, | ||
| ) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add documentation and improve error handling
The new server initialization functions lack documentation explaining their purpose and usage. Additionally, the error from baseSeederWithLang is silently passed through.
Consider adding documentation and explicit error handling:
+// Server initializes a test server with authentication disabled and a custom seeder.
+// It uses a default configuration suitable for testing.
func Server(t *testing.T, seeder Seeder) *httpexpect.Expect {
c := &config.Config{
Origins: []string{"https://example.com"},
AuthSrv: config.AuthSrvConfig{
Disabled: true,
},
}
return StartServer(t, c, true, seeder)
}
+// ServerLanguage initializes a test server with a specific language configuration.
+// It sets up the server using baseSeederWithLang to initialize the database with
+// language-specific test data.
func ServerLanguage(t *testing.T, lang language.Tag) *httpexpect.Expect {
c := &config.Config{
Origins: []string{"https://example.com"},
AuthSrv: config.AuthSrvConfig{
Disabled: true,
},
}
return StartServer(t, c, true,
func(ctx context.Context, r *repo.Container) error {
- return baseSeederWithLang(ctx, r, lang)
+ if err := baseSeederWithLang(ctx, r, lang); err != nil {
+ t.Fatalf("failed to seed database with language %v: %v", lang, err)
+ }
+ return nil
},
)
}📝 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.
| func Server(t *testing.T, seeder Seeder) *httpexpect.Expect { | |
| c := &config.Config{ | |
| Origins: []string{"https://example.com"}, | |
| AuthSrv: config.AuthSrvConfig{ | |
| Disabled: true, | |
| }, | |
| } | |
| return StartServer(t, c, true, seeder) | |
| } | |
| func ServerLanguage(t *testing.T, lang language.Tag) *httpexpect.Expect { | |
| c := &config.Config{ | |
| Origins: []string{"https://example.com"}, | |
| AuthSrv: config.AuthSrvConfig{ | |
| Disabled: true, | |
| }, | |
| } | |
| return StartServer(t, c, true, | |
| func(ctx context.Context, r *repo.Container) error { | |
| return baseSeederWithLang(ctx, r, lang) | |
| }, | |
| ) | |
| } | |
| // Server initializes a test server with authentication disabled and a custom seeder. | |
| // It uses a default configuration suitable for testing. | |
| func Server(t *testing.T, seeder Seeder) *httpexpect.Expect { | |
| c := &config.Config{ | |
| Origins: []string{"https://example.com"}, | |
| AuthSrv: config.AuthSrvConfig{ | |
| Disabled: true, | |
| }, | |
| } | |
| return StartServer(t, c, true, seeder) | |
| } | |
| // ServerLanguage initializes a test server with a specific language configuration. | |
| // It sets up the server using baseSeederWithLang to initialize the database with | |
| // language-specific test data. | |
| func ServerLanguage(t *testing.T, lang language.Tag) *httpexpect.Expect { | |
| c := &config.Config{ | |
| Origins: []string{"https://example.com"}, | |
| AuthSrv: config.AuthSrvConfig{ | |
| Disabled: true, | |
| }, | |
| } | |
| return StartServer(t, c, true, | |
| func(ctx context.Context, r *repo.Container) error { | |
| if err := baseSeederWithLang(ctx, r, lang); err != nil { | |
| t.Fatalf("failed to seed database with language %v: %v", lang, err) | |
| } | |
| return nil | |
| }, | |
| ) | |
| } |
| projects(t, ctx, r, 20, wID, "[wID]project", "ALIAS1") | ||
| projects(t, ctx, r, 20, wId1, "[wId1]project", "ALIAS2") |
There was a problem hiding this comment.
Undefined variable wId1 leads to compilation error
The variable wId1 used here is not defined in the provided code. This will result in a compilation error. If you intended to use a different workspace ID, please define wId1 appropriately. If wId1 is a typo and should be wID1, ensure it's correctly defined.
| @@ -551,3 +554,179 @@ | |||
| Value("id").Raw().(string) | |||
| deleteProject(e, id) // delete | |||
| } | |||
|
|
|||
| func projects(t *testing.T, ctx context.Context, r *repo.Container, count int, wID idx.ID[accountdomain.Workspace], name string, alias string) { | |||
| for i := range make([]int, count) { | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use a traditional for loop for clarity and efficiency
The current loop uses for i := range make([]int, count), which is less efficient and less readable. Consider using a traditional for loop.
Apply this diff to update the loop:
-for i := range make([]int, count) {
+for i := 0; i < count; i++ {This approach is more idiomatic in Go for iterating a specific number of times.
📝 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.
| for i := range make([]int, count) { | |
| for i := 0; i < count; i++ { |
Overview
Pagination and Keyword Search Testing
What I've done
This is a fix for another bug.
reearth/reearthx#58
What I haven't done
How I tested
https://github.com/reearth/reearth-visualizer/pull/1289/files#diff-f7d123401a1a82a94205ef95de13237e750f27d1cce05443924ce5e3fec0e541R576
Which point I want you to review particularly
Memo
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores