refactor(server): return detailed error by lang #1342
Conversation
WalkthroughThis pull request introduces modifications across various files to enhance error handling, internationalization (i18n), and language management within the server application. Key changes include the addition of JSON file support in the build configuration, improvements to error messages for GraphQL operations, and the introduction of middleware for language extraction. The updates also encompass new error types for better localization and refined context management for language settings, ultimately aiming to improve the application's responsiveness to user language preferences and error reporting. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
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.
|
soneda-yuya
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (13)
server/pkg/error/error.go (1)
53-59: Ensure consistent ordering of error messagesIn the
Error()method, iterating over a map (LocalesError) does not guarantee order, which may lead to inconsistent error message outputs. To provide consistent ordering, consider sorting the keys before appending the messages.Apply this diff to sort the locales:
func (e *AppError) Error() string { var errorMessage []string + langs := make([]string, 0, len(e.LocalesError)) + for locale := range e.LocalesError { + langs = append(langs, locale) + } + sort.Strings(langs) - for locale, err := range e.LocalesError { + for _, locale := range langs { + err := e.LocalesError[locale] errorMessage = append(errorMessage, fmt.Sprintf("[%s] %s", locale, err.Message)) } return strings.Join(errorMessage, ",") }Don't forget to import the
sortpackage at the beginning of the file:import ( "fmt" "strings" + "sort" "github.com/reearth/reearth/server/internal/locales" )server/internal/locales/cache.go (1)
7-9: Define a concrete type instead of usingmap[string]interface{}Using
map[string]interface{}forfileCachecan lead to type safety issues and makes the code harder to understand. Consider defining a concrete type or struct for the cached data to improve code readability and maintainability.For example, define a struct for the cached data:
type LocaleData struct { // Define the fields according to the structure of your locale JSON files // For example: Errors map[string]ErrorDefinition `json:"errors"` // Add other relevant fields }Then update
fileCacheto use this type:type LocalesCache struct { mu sync.RWMutex - fileCache map[string]map[string]interface{} + fileCache map[string]*LocaleData }server/internal/locales/locales.go (3)
90-91: Translate the Japanese comment to English.For consistency and maintainability, all comments should be in English.
- // map[string]interface{} 型であることを確認しつつ検索を進める + // Continue searching while confirming the type is map[string]interface{}
93-94: Translate the Japanese comment to English.For consistency and maintainability, all comments should be in English.
- } else if len(keys) == 1 { // 最終キーの場合 + } else if len(keys) == 1 { // If it's the final key
95-96: Translate the Japanese comment to English.For consistency and maintainability, all comments should be in English.
- return nil // 存在しないキーの場合 + return nil // If the key doesn't existserver/internal/usecase/repo/container.go (1)
18-18: Consider adding error context.The
ResourceNotFounderror could benefit from additional context about which resource was not found.- ResourceNotFound = apperror.NewAppError("repo.not_found", nil) + ResourceNotFound = apperror.NewAppError("repo.not_found", map[string]interface{}{ + "details": "The requested resource could not be found in the repository", + })server/internal/infrastructure/mongo/project.go (1)
51-62: LGTM! Consider similar improvements for other methods.The improved error handling for
FindByIDis good. The explicit handling ofmongo.ErrNoDocumentsprovides clearer feedback.Consider applying similar error handling improvements to other methods that use
findOne, such asFindBySceneandFindByPublicName.server/internal/app/app.go (1)
119-134: LGTM! Consider improving language fallback handling.The language middleware implementation looks good. It correctly prioritizes user language settings over browser language when available.
Consider adding a fallback chain for language selection:
- User's explicitly set language (if not "und")
- Browser's language header
- Default application language
Example implementation:
func(c echo.Context) error { - lang := c.Request().Header.Get("lang") + // Default language + lang := "en" + // Browser language + if browserLang := c.Request().Header.Get("lang"); browserLang != "" { + lang = browserLang + } // User language u := adapter.User(c.Request().Context()) if u != nil && u.Lang().String() != "und" { lang = u.Lang().String() } c.SetRequest(c.Request().WithContext(adapter.AttachLang(c.Request().Context(), lang))) return next(c) }server/internal/locales/ja/error.json (2)
6-11: Consider expanding repository error typesThe repo section currently only handles "not_found". Consider adding other common repository errors like "already_exists", "validation_failed", etc.
12-19: Project error messages need expansionThe pkg section only includes project.invalid_alias. Consider adding other common project-related errors for completeness.
Example structure:
"pkg": { "project": { "invalid_alias": { "message": "無効なエイリアス名です。", "description": "エイリアスは5-32文字で、英数字、アンダースコア、ハイフンのみ使用できます。" - } + }, + "already_exists": { + "message": "プロジェクトは既に存在します。", + "description": "同じ名前のプロジェクトが既に存在します。" + }, + "not_found": { + "message": "プロジェクトが見つかりません。", + "description": "指定されたプロジェクトは存在しないか、アクセス権限がありません。" + } } }server/internal/locales/en/error.json (3)
2-6: Enhance the unknown error descriptionThe description should provide more context than the message to help developers understand when and why this error occurs.
"unknown": { "code": "unknown", "message": "An unknown error occurred.", - "description": "An unknown error occurred." + "description": "An unexpected error occurred that couldn't be mapped to a specific error type. This typically indicates an unhandled edge case or system error." },
7-13: Improve the repo not_found error detailsThe error message and description should be more specific and include troubleshooting guidance.
"repo": { "not_found": { "code": "not_found", - "message": "Resource not found.", - "description": "Resource not found." + "message": "The requested resource could not be found.", + "description": "The system could not locate the requested resource. This may occur if the resource was deleted, moved, or never existed. Please verify the resource identifier and try again." } },
14-22: Enhance the invalid_alias error message and descriptionWhile the validation rules are clear, the message could be more user-friendly, and the description could include examples.
"invalid_alias": { "code": "invalid_alias", - "message": "invalid alias name", - "description": "The alias must be 5-32 characters long and can only contain alphanumeric characters, underscores, and hyphens." + "message": "The alias name format is invalid", + "description": "The alias must be 5-32 characters long and can only contain alphanumeric characters, underscores, and hyphens. Examples of valid aliases: 'project-2023', 'my_awesome_project', 'team123'." }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
server/.air.toml(1 hunks)server/e2e/gql_storytelling_test.go(1 hunks)server/e2e/gql_workspace_test.go(5 hunks)server/internal/adapter/context.go(3 hunks)server/internal/adapter/gql/context.go(1 hunks)server/internal/app/app.go(2 hunks)server/internal/app/graphql.go(4 hunks)server/internal/infrastructure/mongo/project.go(2 hunks)server/internal/locales/cache.go(1 hunks)server/internal/locales/en/error.json(1 hunks)server/internal/locales/ja/error.json(1 hunks)server/internal/locales/locales.go(1 hunks)server/internal/usecase/interactor/project.go(3 hunks)server/internal/usecase/repo/container.go(2 hunks)server/pkg/error/error.go(1 hunks)server/pkg/project/project.go(1 hunks)
🔇 Additional comments (10)
server/internal/adapter/gql/context.go (1)
39-39: Update to useadapter.LangByTagReplacing
adapter.Lang(ctx, lang)withadapter.LangByTag(ctx, lang)ensures that the language is correctly extracted from the providedlangparameter. This is appropriate for accurate localization handling.server/internal/app/graphql.go (1)
67-75: Handle missing translations gracefully.The code assumes that
appErr.LocalesError[lang]always exists for the current language.Add a fallback mechanism:
if ok := errors.As(e, &appErr); ok { - localesErr := appErr.LocalesError[lang] + localesErr, exists := appErr.LocalesError[lang] + if !exists { + // Fallback to English if translation is missing + localesErr = appErr.LocalesError["en"] + } graphqlErr = &gqlerror.Error{Let's verify the locales structure:
server/internal/adapter/context.go (1)
71-76: Consider validating language tag format.The function accepts any string as a language tag without validation.
Add validation for language tag format:
func LangByTag(ctx context.Context, lang *language.Tag) string { if lang != nil && !lang.IsRoot() { + // Verify the language tag is well-formed + if _, err := language.Parse(lang.String()); err != nil { + return defaultLang.String() + } return lang.String() } return Lang(ctx) }Let's verify the language tags in use:
server/e2e/gql_workspace_test.go (1)
44-44: LGTM! Consider adding more test cases.The simplified error messages are clearer and more maintainable. However, consider adding test cases for different languages to verify the localization functionality works as expected.
Run this script to check if there are similar test files that might need the same updates:
Also applies to: 70-70, 96-96, 117-117, 140-140
server/internal/usecase/interactor/project.go (2)
207-207: Error handling improvement: Using GraphQL error contextThe change enhances error handling by adding errors to the GraphQL context instead of immediately returning, allowing for better error aggregation.
284-287: Verify accumulated errors before savingGood addition of error check before proceeding with save operation. This ensures that any validation errors are properly handled before persisting changes.
server/e2e/gql_storytelling_test.go (1)
854-854: Simplified error message assertionThe test has been updated to match the new simplified error message format, aligning with the improved error handling approach.
server/.air.toml (1)
16-16: Added JSON files to build monitoringIncluding JSON files in the build process is essential for the new localization system to detect changes in error message definitions.
server/internal/locales/ja/error.json (1)
2-5: Well-structured unknown error fallbackThe unknown error message provides a good fallback for unexpected errors.
server/internal/locales/en/error.json (1)
1-23: Well-structured error definitions!The hierarchical organization and consistent structure make it easy to maintain and extend error definitions.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1342 +/- ##
==========================================
- Coverage 23.00% 23.00% -0.01%
==========================================
Files 1041 1046 +5
Lines 107732 107892 +160
Branches 663 663
==========================================
+ Hits 24786 24818 +32
- Misses 81687 81815 +128
Partials 1259 1259
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
server/internal/app/graphql.go (1)
91-104:⚠️ Potential issueImprove security of error handling.
System errors are still being exposed to clients, which could leak sensitive information.
🧹 Nitpick comments (13)
server/internal/locales/locales_test.go (5)
3-12: LGTM! Consider adding package documentation.The setup with embedded filesystem for test locales is well-structured. Consider adding a package documentation comment to explain the purpose of these tests and the structure of the embedded locale files.
Add this documentation at the top of the file:
package locales +// Package locales_test validates the localization system's functionality, +// specifically testing the loading and retrieval of error messages from +// embedded locale files organized by language (en/*, ja/*).
14-15: Translate comments to English for consistency.The comment
// モック用の localesJson を置き換えるshould be in English to maintain codebase consistency.- // モック用の localesJson を置き換える + // Replace localesJson with test mock data
17-50: Add more test cases for edge scenarios.While the current test cases cover basic scenarios, consider adding tests for:
- Empty key (
"")- Malformed keys (e.g.,
"pkg..project")- Keys with special characters
- Very long keys
Example additional test cases:
{ name: "Empty key", key: "", isPanic: true, }, { name: "Malformed key", key: "pkg..project", isPanic: true, }, { name: "Key with special chars", key: "pkg.project.@invalid", isPanic: true, },
67-108: Enhance test coverage with edge cases.The test cases are well-structured, but consider adding scenarios for:
- Deeper nesting levels (>3 levels)
- Empty maps at intermediate levels
- nil values in the nested structure
Example additional test cases:
{ name: "Deep nesting", keys: []string{"pkg", "project", "category", "subcategory", "item"}, expected: nil, }, { name: "Empty map at intermediate level", keys: []string{"pkg", "empty_map", "key"}, expected: nil, }, { name: "Nil value in nested structure", keys: []string{"pkg", "project", "nil_value"}, expected: nil, },And add corresponding mock data:
mockData := map[string]interface{}{ "pkg": map[string]interface{}{ "project": map[string]interface{}{ "nil_value": nil, }, "empty_map": map[string]interface{}{}, }, }
1-108: Consider extracting test data to separate files.To improve maintainability and readability, consider moving the test data (mock objects and expected results) to separate test fixture files. This would make it easier to maintain the test data, especially as more localized error messages are added.
Suggested structure:
server/internal/locales/ ├── testdata/ │ ├── mock_errors.json │ └── mock_nested_data.json └── locales_test.goThis would also make it easier to maintain consistency between test data and actual locale files.
server/internal/locales/cache_test.go (4)
9-23: Consider enhancing test data and type safety.While the current mock data is good for basic testing, consider these improvements:
- Add test cases for empty maps and special characters
- Consider using a strongly typed structure instead of
map[string]interface{}- Add test cases for nested translation structures that might be needed for complex error messages
Example of a strongly typed approach:
type TranslationEntry struct { Message string Params map[string]string // For interpolation if needed } mockDataEN := map[string]TranslationEntry{ "hello": {Message: "Hello"}, "error.notFound": { Message: "Resource %s not found", Params: map[string]string{"resource": "user"}, }, }
24-35: Add edge cases and concurrent access tests.The basic functionality is well tested, but consider adding these scenarios:
- Empty string as language code
- Concurrent access to the cache
- Case sensitivity handling for language codes
Example of a concurrent access test:
t.Run("concurrent access", func(t *testing.T) { var wg sync.WaitGroup for i := 0; i < 100; i++ { wg.Add(1) go func() { defer wg.Done() cache.SetFileCache("en", mockDataEN) _, _ = cache.GetFromFileCache("en") }() } wg.Wait() })
52-60: Enhance error case coverage.While the basic error case is covered, consider adding tests for:
- Similar language codes (e.g., "en" vs "en-US")
- Invalid language codes (e.g., "invalid", "123", "")
- Special characters in language codes
Example test cases:
testCases := []struct { name string langCode string wantOk bool }{ {"empty string", "", false}, {"invalid format", "en_US", false}, {"special chars", "en@#$", false}, {"similar code", "en-US", false}, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { data, ok := cache.GetFromFileCache(tc.langCode) assert.Equal(t, tc.wantOk, ok) assert.Nil(t, data) }) }
1-60: Add test documentation for maintainability.Consider adding a package-level comment that describes:
- The purpose and scope of these tests
- Any assumptions about the cache behavior
- How these tests relate to the larger localization system
Example documentation:
// Package locales_test verifies the behavior of the localization cache system. // The cache is designed to store and retrieve language-specific translation data // loaded from JSON files at server startup. These tests ensure that: // - Language data can be stored and retrieved correctly // - Multiple languages can coexist in the cache // - Missing languages are handled appropriatelyserver/pkg/apperror/error.go (1)
21-31: Improve variable naming for clarityThe variable
localesErrorrepresents a map of errors for different locales, but the name suggests it is a singular error. Consider renaming it tolocalizedErrorsorlocalesErrorsMapto better reflect that it contains multiple errors, enhancing code readability.Apply this diff to improve variable naming:
func buildLocalesError(localesErrors map[string]*locales.Error) map[string]*LocalesError { - localesError := make(map[string]*LocalesError) + localizedErrors := make(map[string]*LocalesError) for lang, locale := range localesErrors { - localesError[lang] = &LocalesError{ + localizedErrors[lang] = &LocalesError{ Code: locale.Code, Message: locale.Message, Description: locale.Description, } } - return localesError + return localizedErrors }server/internal/app/graphql_test.go (2)
15-16: Translate Japanese comments to English for consistency.The codebase appears to use English as the primary language. Consider translating these comments:
- // モック用のコンテキストとロケール + // Mock context and locale for testing - // アサーション: GraphQL エラーの内容を検証 + // Assertion: Verify GraphQL error contentAlso applies to: 34-34
31-40: Enhance test coverage for error scenarios.While the test covers basic functionality, consider adding test cases for:
- Missing locale for the requested language
- Invalid language tags
- Empty or malformed error messages
server/internal/app/lang_test.go (1)
63-83: Enhance middleware test coverage.The middleware test only covers the happy path. Consider adding test cases for:
- Missing language header
- Invalid language tags
- Multiple language headers
- Language negotiation with Accept-Language header
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
server/internal/adapter/context.go(3 hunks)server/internal/app/app.go(2 hunks)server/internal/app/graphql.go(5 hunks)server/internal/app/graphql_test.go(1 hunks)server/internal/app/lang.go(1 hunks)server/internal/app/lang_test.go(1 hunks)server/internal/locales/cache_test.go(1 hunks)server/internal/locales/en/error.json(1 hunks)server/internal/locales/locales.go(1 hunks)server/internal/locales/locales_test.go(1 hunks)server/internal/usecase/repo/container.go(2 hunks)server/pkg/apperror/error.go(1 hunks)server/pkg/apperror/error_test.go(1 hunks)server/pkg/project/project.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- server/internal/app/app.go
- server/pkg/project/project.go
- server/internal/locales/en/error.json
- server/internal/usecase/repo/container.go
- server/internal/locales/locales.go
🔇 Additional comments (6)
server/internal/locales/cache_test.go (2)
1-8: LGTM! Good choice of testing framework.The package structure and imports are well-organized. Using
testify/assertmakes the test assertions clear and readable.
36-51: Add cache manipulation and boundary tests.Consider adding these test scenarios:
- Overwriting existing language data
- Cache size limits (if any)
- Memory cleanup/eviction strategies
Let's check if there are any cache size limits or cleanup mechanisms in the implementation:
server/internal/app/lang.go (2)
26-34: Middleware implementation is correctThe
AttachLanguageMiddlewarefunction correctly attaches the detected language to the request context, facilitating localized responses.
11-24: Ensure robust language extraction and default handlingIn
LanguageExtractor, consider the following improvements:
- Verify that the
"lang"header is the correct header for language specification. Typically, the"Accept-Language"header is used to indicate the client's preferred languages.- Implement validation to ensure the extracted language code is supported by the application.
- Provide a default language (e.g., English) if the extracted language is empty or unsupported to avoid potential issues with undefined behavior.
Run the following script to check for usage of the
"lang"header across the codebase and ensure consistency:server/pkg/apperror/error_test.go (1)
27-35: Test forErrormethod is accurateThe
TestErrorMethodfunction correctly verifies that theErrormethod returns a formatted error message containing the localized messages.server/internal/adapter/context.go (1)
78-88: Reconsider empty string handling in Lang function.The function returns
defaultLangfor empty strings, which might mask configuration errors. Consider:
- Distinguishing between "no language set" and "explicitly set to empty"
- Logging a warning when falling back to default language
Run this script to check for empty language usage:
✅ Verification successful
Empty string handling in Lang function is appropriate
The current implementation provides a safe fallback to English when no language is set or when an empty string is encountered. This approach:
- Prevents runtime errors
- Maintains consistent behavior
- Follows common i18n patterns
No instances of empty string usage were found in the codebase, indicating proper language value handling throughout the application.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for direct calls to AttachLang with empty string rg "AttachLang\([^,]+,\s*\"\"" -A 2Length of output: 35
Script:
#!/bin/bash # Search for all AttachLang calls rg "AttachLang\(" -A 2 # Search for test files containing language-related tests fd ".*_test\.go$" --exec rg -l "TestLang|Lang\(" {} # Search for defaultLang usage rg "defaultLang" -A 2Length of output: 1268
soneda-yuya
force-pushed
the
refactor/error_handling_be
branch
from
834688e to
ada5a33
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (11)
server/internal/app/i18n/message/errmsg/errmsg_generated.go (1)
9-12: LGTM! Consider adding godoc comments.The error keys follow a consistent naming pattern and cover essential error cases. Consider adding godoc comments to document the usage of each error key.
const ( + // ErrKeyUnknown represents an unspecified or unexpected error ErrKeyUnknown message.ErrKey = "unknown" + // ErrKeyRepoResourceNotFound indicates that the requested resource was not found ErrKeyRepoResourceNotFound message.ErrKey = "repo.resource_not_found" )server/pkg/i18n/message/message.go (3)
14-21: Add documentation for exported types.The exported types
EntityKey,EntityMessage,ErrKey, andErrorMessageshould have proper documentation comments explaining their purpose and usage.
27-28: Translate Japanese comments to English.For better maintainability and collaboration, translate the Japanese comment "関数を評価し、結果を processedData に格納" to English.
53-57: Add documentation for MultiLocaleTemplateData function.The exported function
MultiLocaleTemplateDatashould have a documentation comment explaining its purpose, parameters, and return value.server/pkg/verror/verror.go (1)
41-42: Fix incorrect comment.The comment "Error returns the error message" is incorrect for the
AddTemplateDatamethod.-// Error returns the error message. +// AddTemplateData adds template data for all supported locales and returns the error for chaining. func (e *VError) AddTemplateData(key string, value interface{}) *VError {server/internal/app/graphql_test.go (1)
16-87: Consider using a table-driven test approach.While the current test structure is good, using a table-driven approach could make the tests more maintainable and reduce code duplication.
Example refactor:
func TestCustomErrorPresenter(t *testing.T) { tests := []struct { name string ctx context.Context err error devMode bool expected struct { message string code string sysError interface{} } }{ { name: "vErr with English language", ctx: adapter.AttachLang(context.Background(), language.English), err: vErr, devMode: false, expected: struct{ message: "An unknown error occurred.", code: string(errmsg.ErrKeyUnknown), sysError: nil, }, }, // ... other test cases } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { graphqlErr := customErrorPresenter(tt.ctx, tt.err, tt.devMode) assert.NotNil(t, graphqlErr) assert.Equal(t, tt.expected.message, graphqlErr.Message) assert.Equal(t, tt.expected.code, graphqlErr.Extensions["code"]) assert.Equal(t, tt.expected.sysError, graphqlErr.Extensions["system_error"]) }) } }server/internal/app/graphql.go (4)
65-67: Consider adding panic recovery to the error presenter.While the error presenter setup is correct, consider wrapping it with panic recovery to handle potential panics gracefully:
srv.SetErrorPresenter(func(ctx context.Context, e error) *gqlerror.Error { + defer func() { + if r := recover(); r != nil { + log.Errorfc(ctx, "panic recovered in error presenter: %v", r) + return graphql.DefaultErrorPresenter(ctx, fmt.Errorf("internal server error")) + } + }() return customErrorPresenter(ctx, e, dev) })
87-92: Enhance function documentation.While the function has a basic comment, it would benefit from more detailed documentation including:
- Parameter descriptions
- Return value description
- Example usage
- Error handling behavior
-// customErrorPresenter handles custom GraphQL error presentation. +// customErrorPresenter handles custom GraphQL error presentation by converting various error types +// into localized GraphQL errors. +// +// Parameters: +// - ctx: The context containing language preferences +// - e: The error to be presented +// - devMode: If true, includes additional debugging information +// +// Returns a GraphQL error with localized messages and optional debug information.
93-95: Move systemError declaration closer to usage.The
systemErrorvariable is declared far from its first usage. Consider moving it closer to where it's needed for better code readability.
135-137: Enhance error logging with more context.The error logging could be more specific about the source and nature of the system error:
-log.Errorfc(ctx, "system error: %+v", e) +log.Errorfc(ctx, "graphql system error occurred during error presentation: %+v", e)server/pkg/project/project.go (1)
17-27: Define constants for magic numbers.Consider extracting the magic numbers into named constants for better maintainability:
+const ( + aliasMinLength = 5 + aliasMaxLength = 32 +) ErrInvalidAlias = verror.NewVError( errmsg.ErrKeyPkgProjectInvalidAlias, errmsg.ErrorMessages[errmsg.ErrKeyPkgProjectInvalidAlias], message.MultiLocaleTemplateData(map[string]interface{}{ - "minLength": 5, - "maxLength": 32, + "minLength": aliasMinLength, + "maxLength": aliasMaxLength, "allowedChars": func(locale language.Tag) string { return entitymsg.GetLocalizedEntityMessage(entitymsg.EntityKeyPkgProjectAliasAllowedChars, locale) }, }), nil)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
server/pkg/i18n/gen/entitymsg/gen.gois excluded by!**/gen/**server/pkg/i18n/gen/errmsg/gen.gois excluded by!**/gen/**
📒 Files selected for processing (19)
server/internal/app/app.go(3 hunks)server/internal/app/graphql.go(5 hunks)server/internal/app/graphql_test.go(1 hunks)server/internal/app/i18n/locales/errmsg/en.json(1 hunks)server/internal/app/i18n/locales/errmsg/ja.json(1 hunks)server/internal/app/i18n/message/errmsg/errmsg_generated.go(1 hunks)server/internal/app/i18n/message/messageg.go(1 hunks)server/internal/usecase/repo/container.go(1 hunks)server/pkg/i18n/locales.go(1 hunks)server/pkg/i18n/locales/entitymsg/en.json(1 hunks)server/pkg/i18n/locales/entitymsg/ja.json(1 hunks)server/pkg/i18n/locales/errmsg/en.json(1 hunks)server/pkg/i18n/locales/errmsg/ja.json(1 hunks)server/pkg/i18n/message/entitymsg/entitymsg_generated.go(1 hunks)server/pkg/i18n/message/errmsg/errmsg_generated.go(1 hunks)server/pkg/i18n/message/message.go(1 hunks)server/pkg/project/project.go(2 hunks)server/pkg/verror/verror.go(1 hunks)server/pkg/verror/verror_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- server/pkg/i18n/locales.go
- server/internal/app/app.go
- server/internal/app/i18n/message/messageg.go
- server/pkg/i18n/locales/errmsg/ja.json
- server/pkg/i18n/locales/entitymsg/ja.json
- server/internal/usecase/repo/container.go
- server/pkg/i18n/locales/entitymsg/en.json
- server/pkg/i18n/locales/errmsg/en.json
- server/pkg/i18n/message/errmsg/errmsg_generated.go
- server/internal/app/i18n/locales/errmsg/en.json
- server/pkg/i18n/message/entitymsg/entitymsg_generated.go
- server/internal/app/i18n/locales/errmsg/ja.json
🧰 Additional context used
📓 Learnings (1)
server/pkg/verror/verror.go (1)
Learnt from: soneda-yuya
PR: reearth/reearth-visualizer#1342
File: server/pkg/verror/verror.go:14-0
Timestamp: 2025-01-16T10:43:15.903Z
Learning: In the verror package, NewVError function:
1. Should not validate or provide fallbacks for invalid keys as issues should be caught during development
2. Accepts nil errors as valid input
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: deploy-server / Deploy app to test env
🔇 Additional comments (6)
server/internal/app/i18n/message/errmsg/errmsg_generated.go (1)
1-7: Verify the code generation process.Since this is a generated file, please ensure:
- The generation source (JSON files) is properly version controlled
- The generation process is documented
- The
go generatecommand is included in the build pipeline✅ Verification successful
Code generation setup is properly configured
The error message generation system is well-structured with:
- Source JSON files in
server/internal/app/i18n/locales/errmsg/- Generator at
server/pkg/i18n/gen/errmsg/gen.go- Proper
go:generatecommand inmessageg.go🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for the presence of source files and generation command echo "Checking for source files and generation command..." fd -e json . "server/internal/app/i18n/locales/errmsg" rg "//go:generate" "server/internal/app"Length of output: 407
Script:
#!/bin/bash # Check if generator exists and examine JSON structure echo "=== Checking generator ===" cat pkg/i18n/gen/errmsg/gen.go || echo "Generator not found" echo -e "\n=== Checking JSON structure ===" head -n 10 server/internal/app/i18n/locales/errmsg/en.json head -n 10 server/internal/app/i18n/locales/errmsg/ja.jsonLength of output: 915
Script:
#!/bin/bash # Check for the generator using the correct relative path echo "=== Checking for generator from message directory ===" ls -l server/pkg/i18n/gen/errmsg/gen.go || echo "Generator not found in server path" echo -e "\n=== Checking go:generate command context ===" rg -A 2 "//go:generate" server/internal/app/i18n/message/messageg.goLength of output: 492
server/pkg/verror/verror_test.go (1)
14-39: 🛠️ Refactor suggestionAdd test cases for Japanese localization.
The test only covers English localization. Add test cases for Japanese localization to ensure proper i18n support.
Example addition:
func TestNewVError(t *testing.T) { errmsg.ErrorMessages = map[message.ErrKey]map[language.Tag]message.ErrorMessage{ errmsg.ErrKeyPkgProjectInvalidAlias: { language.English: { Message: "Invalid alias name: {{.aliasName}}", Description: "The alias '{{.aliasName}}' must be {{.minLength}}-{{.maxLength}} characters long.", }, + language.Japanese: { + Message: "無効なエイリアス名: {{.aliasName}}", + Description: "エイリアス '{{.aliasName}}' は {{.minLength}}-{{.maxLength}} 文字である必要があります。", + }, }, }Likely invalid or redundant comment.
server/internal/app/graphql.go (2)
18-21: LGTM!The new imports are correctly organized and necessary for implementing localized error handling.
139-139: Review warning log for potential security concerns.The warning log might expose sensitive information in the GraphQL error. Consider logging only non-sensitive fields or redacting sensitive information.
✅ Verification successful
GraphQL error logging implementation is secure
The warning log is internal and doesn't expose sensitive information to clients. The error handling implementation includes proper sanitization:
- System errors are only exposed in development mode
- User-facing messages are controlled through i18n templates
- GraphQL responses are properly structured with controlled fields
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for potential sensitive information in GraphQL errors ast-grep --pattern 'log.$$(_, "graphqlErr: %+v", $_)'Length of output: 55
Script:
#!/bin/bash # Search for GraphQL error related code rg -l "graphqlErr" --type go # Find error logging patterns rg "log\.(Warn|Error|Info).*Error" --type go -A 2 # Look for GraphQL error type definitions rg "type.*Error" --type go | grep -i "graphql" # Check for error handling middleware or utils fd -e go -x grep -l "func.*Error" {}Length of output: 1996
Script:
#!/bin/bash # Check the GraphQL error context rg -A 5 -B 5 "graphqlErr" server/internal/app/graphql.go # Look for error handling patterns cat server/internal/app/graphql.go # Check for error sanitization rg -l "sanitize|redact|mask" --type goLength of output: 5502
server/pkg/project/project.go (2)
8-13: LGTM!The new imports are correctly organized and necessary for implementing localized error handling.
216-216: LGTM!The error handling correctly uses the new localized error pattern with template data.
soneda-yuya
force-pushed
the
refactor/error_handling_be
branch
from
ada5a33 to
91e8f7b
Compare
soneda-yuya
force-pushed
the
refactor/error_handling_be
branch
7 times, most recently
from
51ba37d to
eb619f0
Compare
|
some Nitpick comments is fixed. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
server/pkg/i18n/message/message.go (2)
25-50: Consider improvements to template handling and error reporting.While the implementation is functional, consider these improvements:
- Instead of returning an empty string on errors, consider returning a fallback message or error indicator.
- Template parsing could be cached to improve performance.
- Add input validation for nil maps to prevent panics.
func ApplyTemplate(ctx context.Context, tmpl string, data map[language.Tag]map[string]interface{}, locale language.Tag) string { + if data == nil { + log.Warnfc(ctx, "template data is nil") + return "[TEMPLATE_ERROR]" + } + + localeData, ok := data[locale] + if !ok { + log.Warnfc(ctx, "no data for locale: %s", locale) + return "[MISSING_LOCALE]" + } + processedData := make(map[string]interface{}) - for key, value := range data[locale] { + for key, value := range localeData {
52-57: Add documentation about memory implications.The implementation is clean, but consider adding documentation about:
- Memory implications when dealing with large data sets (as the function creates copies for each locale)
- Input validation behavior with nil data
// MultiLocaleTemplateData creates a map of template data for multiple locales. +// Note: This function creates a copy of the data for each supported locale, +// which may have memory implications for large data sets. +// Returns an empty map if input data is nil. func MultiLocaleTemplateData(data map[string]interface{}) map[language.Tag]map[string]interface{} { + if data == nil { + return make(map[language.Tag]map[string]interface{}) + } return lo.SliceToMap(i18n.LocaleTypes(), func(locale language.Tag) (language.Tag, map[string]interface{}) { return locale, data })server/internal/app/graphql_test.go (1)
16-87: Enhance test coverage with additional edge cases.The test suite is well-structured but could benefit from additional test cases:
- Test handling of invalid language tags
- Test behavior when error messages map is empty
- Test fallback to default language when requested language is unavailable
Consider refactoring the tests using a table-driven approach:
func TestCustomErrorPresenter(t *testing.T) { + tests := []struct { + name string + ctx context.Context + err error + devMode bool + want *gqlerror.Error + wantCode string + }{ + { + name: "invalid language tag", + ctx: adapter.AttachLang(context.Background(), language.Make("invalid")), + err: vErr, + want: &gqlerror.Error{Message: "An unknown error occurred."}, + wantCode: string(errmsg.ErrKeyUnknown), + }, + // ... existing test cases ... + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := customErrorPresenter(tt.ctx, tt.err, tt.devMode) + assert.Equal(t, tt.want.Message, got.Message) + assert.Equal(t, tt.wantCode, got.Extensions["code"]) + }) + }server/pkg/verror/verror_test.go (1)
15-40: Improve test maintainability and coverage.The test setup could be improved for better maintainability:
- Extract test data to constants or test fixtures
- Add negative test cases for error handling
- Test error message validation
Consider extracting test data:
+const ( + testAliasName = "test_alias" + testMinLength = 5 + testMaxLength = 32 +) + +var ( + testTemplateData = map[language.Tag]map[string]interface{}{ + language.English: { + "aliasName": testAliasName, + "minLength": testMinLength, + "maxLength": testMaxLength, + }, + } +) + func TestNewVError(t *testing.T) { - templateData := map[language.Tag]map[string]interface{}{ - language.English: { - "aliasName": "test_alias", - "minLength": 5, - "maxLength": 32, - }, - } + templateData := testTemplateDataserver/internal/app/graphql.go (2)
149-154: Improve error logging.The logging could be more structured and informative:
- Log entries should include more context
- Warning log for graphqlErr might be too verbose
Consider this improvement:
if systemError != "" { - log.Errorfc(ctx, "system error: %+v", e) + log.Errorfc(ctx, "GraphQL system error: %+v, code: %s, path: %v", + e, + graphqlErr.Extensions["code"], + graphqlErr.Path, + ) } -log.Warnfc(ctx, "graphqlErr: %+v", graphqlErr) +// Only log non-system errors at debug level +if systemError == "" { + log.Debugfc(ctx, "GraphQL error: code=%s, message=%s", + graphqlErr.Extensions["code"], + graphqlErr.Message, + ) +}
65-73: Improve panic recovery logging.The panic recovery could provide more context about the error that was being processed.
srv.SetErrorPresenter(func(ctx context.Context, e error) *gqlerror.Error { defer func() { if r := recover(); r != nil { - log.Errorfc(ctx, "panic recovered in error presenter: %v", r) + log.Errorfc(ctx, "panic recovered in error presenter while processing error: %v, panic: %v", e, r) return } }() return customErrorPresenter(ctx, e, dev) })server/internal/usecase/repo/container_errkey_test.go (1)
12-19: Enhance test coverage with specific message validation.While the test verifies non-empty messages, consider enhancing it with:
- Validation of actual message content against expected strings
- Testing with different template data variations
- Verification of error code consistency across locales
Example enhancement:
func TestRepoErrResourceNotFound(t *testing.T) { ctx := context.Background() vErr := ErrResourceNotFound + testCases := []struct { + templateData map[string]interface{} + expected map[i18n.Locale]string + }{ + { + templateData: map[string]interface{}{"resource": "test"}, + expected: map[i18n.Locale]string{ + i18n.LocaleEn: "Resource test not found", + i18n.LocaleJa: "リソース test が見つかりません", + }, + }, + } + + for _, tc := range testCases { + vErr := vErr.AddTemplateData("resource", tc.templateData["resource"]) for _, locale := range i18n.LocaleTypes() { - assert.NotEqual(t, "", message.ApplyTemplate(ctx, vErr.ErrMsg[locale].Message, vErr.TemplateData, locale)) + actual := message.ApplyTemplate(ctx, vErr.ErrMsg[locale].Message, vErr.TemplateData, locale) + assert.Equal(t, tc.expected[locale], actual) assert.NotEqual(t, "", message.ApplyTemplate(ctx, vErr.ErrMsg[locale].Description, vErr.TemplateData, locale)) } + } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
server/pkg/i18n/gen/entitymsg/gen.gois excluded by!**/gen/**server/pkg/i18n/gen/errmsg/gen.gois excluded by!**/gen/**
📒 Files selected for processing (22)
server/e2e/gql_storytelling_test.go(1 hunks)server/internal/app/app.go(3 hunks)server/internal/app/graphql.go(5 hunks)server/internal/app/graphql_test.go(1 hunks)server/internal/app/i18n/locales/errmsg/en.json(1 hunks)server/internal/app/i18n/locales/errmsg/ja.json(1 hunks)server/internal/app/i18n/message/errmsg/errmsg_generated.go(1 hunks)server/internal/app/i18n/message/messageg.go(1 hunks)server/internal/usecase/repo/container.go(1 hunks)server/internal/usecase/repo/container_errkey_test.go(1 hunks)server/pkg/i18n/locales.go(1 hunks)server/pkg/i18n/locales/entitymsg/en.json(1 hunks)server/pkg/i18n/locales/entitymsg/ja.json(1 hunks)server/pkg/i18n/locales/errmsg/en.json(1 hunks)server/pkg/i18n/locales/errmsg/ja.json(1 hunks)server/pkg/i18n/message/entitymsg/entitymsg_generated.go(1 hunks)server/pkg/i18n/message/errmsg/errmsg_generated.go(1 hunks)server/pkg/i18n/message/message.go(1 hunks)server/pkg/project/project.go(2 hunks)server/pkg/project/project_errkey_test.go(1 hunks)server/pkg/verror/verror.go(1 hunks)server/pkg/verror/verror_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (16)
- server/internal/app/app.go
- server/pkg/i18n/locales.go
- server/internal/app/i18n/message/messageg.go
- server/pkg/i18n/locales/errmsg/ja.json
- server/pkg/i18n/locales/entitymsg/ja.json
- server/pkg/i18n/locales/entitymsg/en.json
- server/e2e/gql_storytelling_test.go
- server/internal/app/i18n/message/errmsg/errmsg_generated.go
- server/internal/usecase/repo/container.go
- server/pkg/i18n/message/entitymsg/entitymsg_generated.go
- server/pkg/i18n/locales/errmsg/en.json
- server/pkg/i18n/message/errmsg/errmsg_generated.go
- server/internal/app/i18n/locales/errmsg/en.json
- server/pkg/project/project.go
- server/internal/app/i18n/locales/errmsg/ja.json
- server/pkg/verror/verror.go
🧰 Additional context used
📓 Learnings (2)
server/internal/app/graphql.go (1)
Learnt from: soneda-yuya
PR: reearth/reearth-visualizer#1342
File: server/internal/app/graphql.go:0-0
Timestamp: 2025-01-17T12:40:02.613Z
Learning: In the GraphQL error presenter, when a localized error message is not available for the requested language, the code falls back to using graphql.DefaultErrorPresenter(ctx, e) instead of attempting to use alternative languages. This ensures that error messages are always presented, even when localization is not available.
server/internal/app/graphql_test.go (1)
Learnt from: soneda-yuya
PR: reearth/reearth-visualizer#1342
File: server/internal/app/graphql.go:0-0
Timestamp: 2025-01-17T12:40:02.613Z
Learning: In the GraphQL error presenter, when a localized error message is not available for the requested language, the code falls back to using graphql.DefaultErrorPresenter(ctx, e) instead of attempting to use alternative languages. This ensures that error messages are always presented, even when localization is not available.
🔇 Additional comments (3)
server/pkg/i18n/message/message.go (2)
16-23: Well-structured type definitions for i18n messages.The type definitions provide a clear separation between entity and error messages, with ErrorMessage properly separating the message from its description. This aligns well with the PR's goal of centralizing message management.
1-14: Verify the existence and functionality of message generators.Let's ensure the referenced generator scripts exist and are functional.
✅ Verification successful
Generator scripts and their dependencies are properly configured
The message generation system is complete with:
- Functional generator scripts
- Required locale files (en, ja)
- Generated output files in place
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the generator scripts exist fd -t f "gen\.go$" server/pkg/i18n/gen/ # Check their content to understand the generation logic for f in $(fd -t f "gen\.go$" server/pkg/i18n/gen/); do echo "=== $f ===" cat "$f" doneLength of output: 11144
Script:
#!/bin/bash # Check if the locale directories and JSON files exist echo "=== Checking locale directories ===" fd -t d "locales" server/pkg/i18n/ fd -t f "\.json$" server/pkg/i18n/locales/ # Check if the generated files exist echo -e "\n=== Checking generated files ===" fd -t f "generated\.go$" server/pkg/i18n/Length of output: 580
server/pkg/project/project_errkey_test.go (1)
7-10: Consider adding test coverage for missing locales.The current tests only verify existing locales. Consider adding tests for:
- Handling of unsupported locales
- Fallback behavior when a locale is missing
soneda-yuya
force-pushed
the
refactor/error_handling_be
branch
from
eb619f0 to
0ae0aed
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
server/pkg/verror/verror_test.go (3)
15-40: Consider extracting test data to constants.The test data (error messages, template data) could be moved to test constants for better maintainability and reuse across tests.
+const ( + testErrCode = "invalid_alias" + testErrMsg = "Invalid alias name: {{.aliasName}}" + testErrDesc = "The alias '{{.aliasName}}' must be {{.minLength}}-{{.maxLength}} characters long." +) + +var testTemplateData = map[language.Tag]map[string]interface{}{ + language.English: { + "aliasName": "test_alias", + "minLength": 5, + "maxLength": 32, + }, +} func TestNewVError(t *testing.T) { errmsg.ErrorMessages = map[message.ErrKey]map[language.Tag]message.ErrorMessage{ errmsg.ErrKeyPkgProjectInvalidAlias: { language.English: { - Message: "Invalid alias name: {{.aliasName}}", - Description: "The alias '{{.aliasName}}' must be {{.minLength}}-{{.maxLength}} characters long.", + Message: testErrMsg, + Description: testErrDesc, }, }, } - templateData := map[language.Tag]map[string]interface{}{ - language.English: { - "aliasName": "test_alias", - "minLength": 5, - "maxLength": 32, - }, - }
42-59: Convert to table-driven test for better test coverage.Consider using table-driven tests to cover more test cases and make it easier to add new ones.
func TestAddTemplateData(t *testing.T) { + tests := []struct { + name string + data []struct{ key string; value interface{} } + expected map[language.Tag]map[string]interface{} + }{ + { + name: "multiple values", + data: []struct{ key string; value interface{} }{ + {key: "key1", value: "value1"}, + {key: "key2", value: 123}, + }, + expected: map[language.Tag]map[string]interface{}{ + language.English: { + "key1": "value1", + "key2": 123, + }, + language.Japanese: { + "key1": "value1", + "key2": 123, + }, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { ve := verror.NewVError(errmsg.ErrKeyPkgProjectInvalidAlias, errmsg.ErrorMessages[errmsg.ErrKeyPkgProjectInvalidAlias], nil, nil) - ve = ve.AddTemplateData("key1", "value1") - ve = ve.AddTemplateData("key2", 123) + for _, d := range tt.data { + ve = ve.AddTemplateData(d.key, d.value) + } - expectedData := map[language.Tag]map[string]interface{}{ - language.English: { - "key1": "value1", - "key2": 123, - }, - language.Japanese: { - "key1": "value1", - "key2": 123, - }, - } - assert.Equal(t, expectedData, ve.TemplateData) + assert.Equal(t, tt.expected, ve.TemplateData) + }) + } }
61-75: Add test cases for error scenarios.Consider adding test cases for:
- Missing template variables
- Invalid template syntax
- Empty template string
func TestApplyTemplate(t *testing.T) { + tests := []struct { + name string + template string + data map[language.Tag]map[string]interface{} + expected string + expectError bool + }{ + { + name: "valid template", + template: "Invalid alias name: {{.aliasName}}, must be between {{.minLength}} and {{.maxLength}} characters.", + data: map[language.Tag]map[string]interface{}{ + language.English: { + "aliasName": "test_alias", + "minLength": 5, + "maxLength": 32, + }, + }, + expected: "Invalid alias name: test_alias, must be between 5 and 32 characters.", + }, + { + name: "missing variable", + template: "Invalid alias name: {{.missing}}", + data: map[language.Tag]map[string]interface{}{ + language.English: {}, + }, + expected: "Invalid alias name: <no value>", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctx := context.Background() + result := message.ApplyTemplate(ctx, tt.template, tt.data, language.English) + assert.Equal(t, tt.expected, result) + }) + }server/internal/app/graphql.go (1)
102-156: Consider enhancing error handling and logging.While the implementation is solid, consider these improvements:
- Add structured logging fields for better error tracking
- Consider using constants for extension keys
- Add trace ID to extensions in development mode for better debugging
+const ( + extensionKeyCode = "code" + extensionKeyMessage = "message" + extensionKeyDescription = "description" + extensionKeySystemError = "system_error" +) func customErrorPresenter(ctx context.Context, e error, devMode bool) *gqlerror.Error { // ... existing code ... if devMode { if fieldCtx := graphql.GetFieldContext(ctx); fieldCtx != nil { graphqlErr.Path = fieldCtx.Path() } else { graphqlErr.Path = ast.Path{} } - graphqlErr.Extensions["system_error"] = systemError + graphqlErr.Extensions[extensionKeySystemError] = systemError + // Add trace ID for better debugging + if traceID := log.GetTraceID(ctx); traceID != "" { + graphqlErr.Extensions["trace_id"] = traceID + } } if systemError != "" { - log.Errorfc(ctx, "system error: %+v", e) + log.Errorfc(ctx, "system error", + "error", e, + "code", graphqlErr.Extensions[extensionKeyCode], + "path", graphqlErr.Path, + ) } - log.Warnfc(ctx, "graphqlErr: %+v", graphqlErr) + log.Warnfc(ctx, "graphql error occurred", + "message", graphqlErr.Message, + "path", graphqlErr.Path, + "extensions", graphqlErr.Extensions, + ) return graphqlErr }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
server/pkg/i18n/gen/entitymsg/gen.gois excluded by!**/gen/**server/pkg/i18n/gen/errmsg/gen.gois excluded by!**/gen/**
📒 Files selected for processing (7)
server/internal/app/graphql.go(5 hunks)server/internal/app/i18n/locales/errmsg/en.json(1 hunks)server/internal/app/i18n/locales/errmsg/ja.json(1 hunks)server/internal/usecase/repo/container_errkey_test.go(1 hunks)server/pkg/i18n/message/message.go(1 hunks)server/pkg/verror/verror.go(1 hunks)server/pkg/verror/verror_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- server/internal/usecase/repo/container_errkey_test.go
- server/pkg/i18n/message/message.go
- server/internal/app/i18n/locales/errmsg/ja.json
- server/internal/app/i18n/locales/errmsg/en.json
- server/pkg/verror/verror.go
🧰 Additional context used
📓 Learnings (1)
server/internal/app/graphql.go (2)
Learnt from: soneda-yuya
PR: reearth/reearth-visualizer#1342
File: server/internal/app/graphql.go:0-0
Timestamp: 2025-01-17T12:40:02.613Z
Learning: In the GraphQL error presenter, when a localized error message is not available for the requested language, the code falls back to using graphql.DefaultErrorPresenter(ctx, e) instead of attempting to use alternative languages. This ensures that error messages are always presented, even when localization is not available.
Learnt from: soneda-yuya
PR: reearth/reearth-visualizer#1342
File: server/internal/app/graphql.go:109-125
Timestamp: 2025-01-17T12:51:23.131Z
Learning: In the GraphQL error presenter, when handling VError with translations, if the error message generation fails (e.g., missing translation), fallback to graphql.DefaultErrorPresenter(ctx, e) by checking if graphqlErr is nil. This pattern is more maintainable than implementing complex language fallback logic.
🔇 Additional comments (4)
server/pkg/verror/verror_test.go (2)
103-108: LGTM!The test is well-focused and correctly verifies the error unwrapping functionality.
77-101: 🛠️ Refactor suggestionAdd test cases for Japanese localization.
The test only covers English localization. Add test cases for Japanese to ensure proper i18n support.
func TestError(t *testing.T) { - en := language.English + tests := []struct { + name string + lang language.Tag + message string + desc string + expected string + }{ + { + name: "English localization", + lang: language.English, + message: "Invalid alias name: {{.aliasName}}", + desc: "The alias '{{.aliasName}}' must be {{.minLength}}-{{.maxLength}} characters long.", + expected: "Invalid alias name: test_alias", + }, + { + name: "Japanese localization", + lang: language.Japanese, + message: "無効なエイリアス名: {{.aliasName}}", + desc: "エイリアス '{{.aliasName}}' は {{.minLength}}-{{.maxLength}} 文字である必要があります。", + expected: "無効なエイリアス名: test_alias", + }, + } - errmsg.ErrorMessages = map[message.ErrKey]map[language.Tag]message.ErrorMessage{ - errmsg.ErrKeyPkgProjectInvalidAlias: { - en: { - Message: "Invalid alias name: {{.aliasName}}", - Description: "The alias '{{.aliasName}}' must be {{.minLength}}-{{.maxLength}} characters long.", - }, - }, - } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + errmsg.ErrorMessages = map[message.ErrKey]map[language.Tag]message.ErrorMessage{ + errmsg.ErrKeyPkgProjectInvalidAlias: { + tt.lang: { + Message: tt.message, + Description: tt.desc, + }, + }, + }Likely invalid or redundant comment.
server/internal/app/graphql.go (2)
65-73: LGTM! Well-implemented error handling with panic recovery.The error presenter setup with panic recovery is a good practice to prevent crashes.
93-102: LGTM! Clear and comprehensive documentation.The function documentation follows Go standards and clearly describes the purpose, parameters, and return values.
server/pkg/verror/verror.go
Outdated
| Err error | ||
| } | ||
|
|
||
| // GetErrCode returns the error code. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
server/internal/app/graphql.go (4)
5-5: Consider organizing imports into logical groups.Consider organizing imports into standard library, external packages, and internal packages for better readability:
import ( // Standard library "context" "errors" "time" // External packages "github.com/99designs/gqlgen/graphql" "github.com/99designs/gqlgen/graphql/handler" "github.com/99designs/gqlgen/graphql/handler/extension" "github.com/99designs/gqlgen/graphql/handler/lru" "github.com/99designs/gqlgen/graphql/handler/transport" "github.com/labstack/echo/v4" "github.com/ravilushqa/otelgqlgen" "github.com/vektah/gqlparser/v2/ast" "github.com/vektah/gqlparser/v2/gqlerror" "golang.org/x/text/language" // Internal packages "github.com/reearth/reearth/server/internal/adapter" "github.com/reearth/reearth/server/internal/adapter/gql" "github.com/reearth/reearth/server/internal/app/config" "github.com/reearth/reearth/server/pkg/i18n/message" "github.com/reearth/reearth/server/pkg/verror" "github.com/reearth/reearthx/log" )Also applies to: 18-23
65-73: Enhance panic recovery logging.The panic recovery logging could be more detailed to aid in debugging:
srv.SetErrorPresenter(func(ctx context.Context, e error) *gqlerror.Error { defer func() { if r := recover(); r != nil { - log.Errorfc(ctx, "panic recovered in error presenter: %v", r) + log.Errorfc(ctx, "panic recovered in error presenter: %v, original error: %+v, stack: %s", r, e, debug.Stack()) return } }() return customErrorPresenter(ctx, e, dev) })
93-94: Remove unnecessary function comment.Based on past review comments, the implementation is self-documenting and doesn't need a long comment.
-// customErrorPresenter handles custom GraphQL error presentation by converting various error types -// into localized GraphQL errors. func customErrorPresenter(ctx context.Context, e error, devMode bool) *gqlerror.Error {
142-147: Adjust logging levels based on error severity.Currently, all errors are logged at WARN level, which might create noise in logs. Consider:
- Log system errors at ERROR level (already done)
- Log validation/business errors at INFO level
- Only log unexpected errors at WARN level
if systemError != "" { log.Errorfc(ctx, "system error: %+v", e) + log.Warnfc(ctx, "graphqlErr: %+v", graphqlErr) + } else if vError != nil { + log.Infofc(ctx, "graphqlErr: %+v", graphqlErr) + } else { + log.Warnfc(ctx, "unexpected graphqlErr: %+v", graphqlErr) } - - log.Warnfc(ctx, "graphqlErr: %+v", graphqlErr)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
server/internal/app/graphql.go(5 hunks)server/pkg/verror/verror.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- server/pkg/verror/verror.go
🧰 Additional context used
📓 Learnings (1)
server/internal/app/graphql.go (2)
Learnt from: soneda-yuya
PR: reearth/reearth-visualizer#1342
File: server/internal/app/graphql.go:0-0
Timestamp: 2025-01-17T12:40:02.613Z
Learning: In the GraphQL error presenter, when a localized error message is not available for the requested language, the code falls back to using graphql.DefaultErrorPresenter(ctx, e) instead of attempting to use alternative languages. This ensures that error messages are always presented, even when localization is not available.
Learnt from: soneda-yuya
PR: reearth/reearth-visualizer#1342
File: server/internal/app/graphql.go:109-125
Timestamp: 2025-01-17T12:51:23.131Z
Learning: In the GraphQL error presenter, when handling VError with translations, if the error message generation fails (e.g., missing translation), fallback to graphql.DefaultErrorPresenter(ctx, e) by checking if graphqlErr is nil. This pattern is more maintainable than implementing complex language fallback logic.
🔇 Additional comments (1)
server/internal/app/graphql.go (1)
102-118: LGTM! Robust error handling implementation.The error handling implementation correctly:
- Handles localization with proper fallback (as confirmed by retrieved learnings)
- Safely handles template data
- Includes proper error code and description
server/internal/app/graphql.go
Outdated
| var vError *verror.VError | ||
| lang := adapter.Lang(ctx, nil) | ||
|
|
||
| // Handle application-specific errors |
server/internal/app/graphql.go
Outdated
| systemError = e.Error() | ||
| } | ||
|
|
||
| // Ensure Extensions map exists |
server/internal/app/graphql.go
Outdated
| } | ||
|
|
||
| if graphqlErr == nil { | ||
| // Fallback to default GraphQL error presenter |
server/internal/app/graphql.go
Outdated
| graphqlErr.Extensions = make(map[string]interface{}) | ||
| } | ||
|
|
||
| // Add debugging information in development mode |
server/internal/app/lang.go
Outdated
| // Extract user language from the adapter | ||
| // if user language is not "und", use user language | ||
| // if user language is "und", use browser language |
server/internal/app/lang.go
Outdated
| return tag | ||
| } | ||
|
|
||
| // AttachLanguageMiddleware attaches the detected language to the request context. |
server/pkg/verror/verror.go
Outdated
| "golang.org/x/text/language" | ||
| ) | ||
|
|
||
| // VError represents an error with a code identifier. |
| return parts[len(parts)-1] | ||
| } | ||
|
|
||
| // NewVError creates a new VError with the given key and error. |
server/pkg/verror/verror.go
Outdated
| } | ||
| } | ||
|
|
||
| // AddTemplateData adds template data to the cloned VError. |
server/pkg/verror/verror.go
Outdated
| return clone | ||
| } | ||
|
|
||
| // Error returns the error message. |
server/pkg/verror/verror.go
Outdated
| return "" | ||
| } | ||
|
|
||
| // Unwrap returns the underlying error. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
server/internal/app/graphql.go (4)
5-5: Consider organizing imports by groups.Consider organizing imports into standard library, external packages, and internal packages for better readability:
import ( "context" "errors" "time" "golang.org/x/text/language" "github.com/99designs/gqlgen/graphql" "github.com/99designs/gqlgen/graphql/handler" "github.com/99designs/gqlgen/graphql/handler/extension" "github.com/99designs/gqlgen/graphql/handler/lru" "github.com/99designs/gqlgen/graphql/handler/transport" "github.com/labstack/echo/v4" "github.com/ravilushqa/otelgqlgen" "github.com/vektah/gqlparser/v2/ast" "github.com/vektah/gqlparser/v2/gqlerror" "github.com/reearth/reearth/server/internal/adapter" "github.com/reearth/reearth/server/internal/adapter/gql" "github.com/reearth/reearth/server/internal/app/config" "github.com/reearth/reearth/server/pkg/i18n/message" "github.com/reearth/reearth/server/pkg/verror" "github.com/reearth/reearthx/log" )Also applies to: 18-21, 23-23
93-94: Remove unnecessary function comment.Based on past review comments from pyshx, we should remove long comments as the implementation is already understandable.
-// customErrorPresenter handles custom GraphQL error presentation by converting various error types -// into localized GraphQL errors. func customErrorPresenter(ctx context.Context, e error, devMode bool) *gqlerror.Error {
108-111: Consider defining error codes as constants.Error codes should be defined as constants to ensure consistency and prevent typos:
+const ( + ExtCodeKey = "code" + ExtMessageKey = "message" + ExtDescriptionKey = "description" +) Extensions: map[string]interface{}{ - "code": vError.GetErrCode(), - "message": messageText, - "description": message.ApplyTemplate(ctx, errMsg.Description, vError.TemplateData, language.Make(lang)), + ExtCodeKey: vError.GetErrCode(), + ExtMessageKey: messageText, + ExtDescriptionKey: message.ApplyTemplate(ctx, errMsg.Description, vError.TemplateData, language.Make(lang)), }
138-143: Adjust logging levels for better observability.Consider adjusting the logging levels:
- System errors should remain at ERROR level
- GraphQL errors could be at DEBUG level in production as they're expected errors
if systemError != "" { log.Errorfc(ctx, "system error: %+v", e) } -log.Warnfc(ctx, "graphqlErr: %+v", graphqlErr) +if devMode { + log.Warnfc(ctx, "graphqlErr: %+v", graphqlErr) +} else { + log.Debugfc(ctx, "graphqlErr: %+v", graphqlErr) +}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
server/internal/app/graphql.go(5 hunks)server/internal/app/lang.go(1 hunks)server/pkg/verror/verror.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- server/internal/app/lang.go
- server/pkg/verror/verror.go
🧰 Additional context used
📓 Learnings (1)
server/internal/app/graphql.go (2)
Learnt from: soneda-yuya
PR: reearth/reearth-visualizer#1342
File: server/internal/app/graphql.go:0-0
Timestamp: 2025-01-17T12:40:02.613Z
Learning: In the GraphQL error presenter, when a localized error message is not available for the requested language, the code falls back to using graphql.DefaultErrorPresenter(ctx, e) instead of attempting to use alternative languages. This ensures that error messages are always presented, even when localization is not available.
Learnt from: soneda-yuya
PR: reearth/reearth-visualizer#1342
File: server/internal/app/graphql.go:109-125
Timestamp: 2025-01-17T12:51:23.131Z
Learning: In the GraphQL error presenter, when handling VError with translations, if the error message generation fails (e.g., missing translation), fallback to graphql.DefaultErrorPresenter(ctx, e) by checking if graphqlErr is nil. This pattern is more maintainable than implementing complex language fallback logic.
🔇 Additional comments (1)
server/internal/app/graphql.go (1)
65-73: LGTM! Robust error handling with panic recovery.The error presenter implementation is well-designed with:
- Panic recovery to prevent crashes
- Proper logging of recovered panics
- Clean delegation to customErrorPresenter
Overview
This document outlines the design for localizing GraphQL errors on the backend. The goal is to reduce the cost of managing error messages by centralizing their localization on the backend, instead of handling them separately in the frontend.
Approach
Frontend Request Handling
langto Request Header:lang=ja.lang=en.errorsfield of the GraphQL response.Problem Statement
Current Challenges
Proposed Solution
Implementation Details
Error Definition Files
Error messages are managed in JSON files, organized by language and path.
server/internal/locales/ja/error.jsonserver/internal/locales/en/error.jsonSample Error Definition
{ "unknown": { "message": "An unknown error occurred.", "description": "An unknown error occurred." }, "repo": { "not_found": { "message": "Resource not found.", "description": "Resource not found." } }, "pkg": { "project": { "invalid_alias": { "message": "Invalid alias name", "description": "The alias must be 5-32 characters long and can only contain alphanumeric characters, underscores, and hyphens." } } } }Error Display Workflow
graph TD A[Frontend] -->|Attach lang to header| B[Backend] B --> C{Error occurs} C -->|Fetch localized message| D[Retrieve error message from locales] D -->|GraphQL response| A A -->|Loop over errors and display| E[Display error to user]Directory Structure
internal/i18nis internal locales filepkg/i18nis public locales fileEditing or Adding Error Messages
To add or edit error messages, modify the following files:
•
server/internal/app/i18n/locales/errmsg/ja.json•
server/internal/app/i18n/locales/errmsg/en.json•
server/pkg/i18n/locales/errmsg/ja.json•
server/pkg/i18n/locales/errmsg/en.jsonAfter updating these files, you must run the go generate command to regenerate the error key constants.
Steps to Regenerate Constants
server/pkg/i18n/genxxx/i18n/messages/errmsg/errmsg_generated.gois updated like this.Implementation Rules
• Use hierarchical keys based on the originating path (e.g.,
repo.not_found,pkg.project.invalid_alias).• All error definitions are loaded at startup.
• The server will fail to start if definitions are missing.
Error Definition Example on BE
• First argument: Error key (e.g., "repo.not_found").
• Second argument: Error object (nil for static errors or dynamic values when necessary).
GraphQL Response Example
{ "errors": [ { "message": "Invalid alias name", "path": ["updateProject"], "extensions": { "code": "invalid_alias", "description": "The alias must be 5-32 characters long and can only contain alphanumeric characters, underscores, and hyphens.", "system_error": "input: updateProject [en] Invalid alias name, [ja] 無効なエイリアス名です。" } } ], "data": { "updateProject": { "project": { "id": "01jgz89mdmm5p25t2867wpembv", "name": "test", "alias": "test1" } } } }Conclusion
• Centralized Error Management:
• By managing error localization on the backend, we simplify maintenance and reduce redundancy.
• Localized Error Messages:
• The frontend can display backend-provided errors directly, ensuring consistency and reducing complexity.
• Implementation Benefits:
• Streamlined error handling process.
• Faster development by avoiding duplication between frontend and backend.
What I've done
implement backend according to design.
What I haven't done
implement frontend
How I tested
return error from backend like this on my local
{ "errors": [ { "message": "Invalid alias name", "path": ["updateProject"], "extensions": { "code": "invalid_alias", "description": "The alias must be 5-32 characters long and can only contain alphanumeric characters, underscores, and hyphens.", "system_error": "input: updateProject [en] Invalid alias name, [ja] 無効なエイリアス名です。" } } ], "data": { "updateProject": { "project": { "id": "01jgz89mdmm5p25t2867wpembv", "name": "test", "alias": "test1" } } } }Which point I want you to review particularly
Memo
Summary by CodeRabbit
Release Notes
Language Support
Error Handling
Project Management
Testing
These changes focus on improving the application's error handling, language support, and overall user experience.