Rpc replacement

[MadsRC]

User description

This PR aims to tackle the issue of the codebase containing 2 RPC implementations. One of them can be removed, which should simplify things.

The 2 implementations are:

1. JSON-RPC 2.0 - Used by the user-facing API.
2. GRPC - Used to communicate with backend check services.

It was chosen to get rid of the JSON-RPC 2.0 implementation, as this would result in less code overall, as well as an ability to rely on a project that is maintained by a large organization and widely used.

---

PR Type

Enhancement, Tests

---

Description

• Added new gRPC service definitions for user management, including message types for user requests and responses, and methods for creating, updating, deleting, and retrieving users.
• Added new gRPC service definitions for scan operations, including message types for scan requests and responses, and methods for scanning text and images.
• Added mock implementations for UserServiceServer, including mock methods for creating, updating, deleting, and retrieving users.

---

Changes walkthrough 📝

	Relevant files
Enhancement	users.pb.go Add gRPC service definitions for user management.                internal/grpc/sophrosyne/v0/users.pb.go Added new gRPC service definitions for user management. Implemented message types for user requests and responses. Included methods for creating, updating, deleting, and retrieving users. +1123/-0 scans.pb.go Add gRPC service definitions for scan operations.                internal/grpc/sophrosyne/v0/scans.pb.go Added new gRPC service definitions for scan operations. Implemented message types for scan requests and responses. Included methods for scanning text and images. +394/-0
Tests	UserServiceServer.go Add mock implementations for UserServiceServer.                    internal/mocks/internal_/grpc/sophrosyne/v0/UserServiceServer.go Added mock implementations for UserServiceServer. Included mock methods for creating, updating, deleting, and retrieving users. +425/-0

---

💡 PR-Agent usage:
Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

[qodo-merge-pro]

PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

PR Review 🔍

⏱️ Estimated effort to review [1-5]	4, because the PR introduces a significant amount of new code across multiple files, including gRPC service definitions, message types, and mock implementations. Reviewing this PR requires a deep understanding of gRPC, protocol buffers, and the specific business logic being implemented. Additionally, the changes involve multiple aspects of user and scan management which need to be carefully validated for correctness and integration with existing systems.
🧪 Relevant tests	No
⚡ Possible issues	Possible Bug: The PR introduces extensive changes with new gRPC service definitions and related functionality but does not mention the addition of new tests to cover these changes. This could lead to potential bugs and regressions not being caught early.
	Performance Concern: The addition of multiple new gRPC services and message types could potentially impact the performance of the system. It would be advisable to conduct performance testing to ensure that the system can handle the additional load.
🔒 Security concerns	No

[qodo-merge-pro]

PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

PR Code Suggestions ✨

Category	Suggestion	Score
Best practice	Use the passed context parameter in the getAuthContext method to ensure context propagation The getAuthContext method creates a new context with metadata but does not use the passed ctx parameter. It should use the passed ctx to ensure context propagation. tests/integration/startup_test.go [91] -return metadata.NewOutgoingContext(context.Background(), metadata.New(map[string]string{"authorization": fmt.Sprintf("Bearer %s", te.rootToken)})) +return metadata.NewOutgoingContext(ctx, metadata.New(map[string]string{"authorization": fmt.Sprintf("Bearer %s", te.rootToken)})) Suggestion importance[1-10]: 8 Why: Using the passed context instead of creating a new one is a best practice in Go, as it ensures proper propagation of metadata, cancellation signals, and deadlines.	8
	Add a validation method to ensure all required fields in CreateCheckRequest are set Consider adding a method to validate the CreateCheckRequest struct to ensure all required fields are set before processing, which can prevent potential issues due to missing data. internal/grpc/sophrosyne/v0/checks.pb.go [319-327] type CreateCheckRequest struct { state protoimpl.MessageState sizeCache protoimpl.SizeCache unknownFields protoimpl.UnknownFields Name string `protobuf:"bytes,1,opt,name=name,proto3" json:"name,omitempty"` Profiles []string `protobuf:"bytes,2,rep,name=profiles,proto3" json:"profiles,omitempty"` UpstreamServices []string `protobuf:"bytes,3,rep,name=upstream_services,json=upstreamServices,proto3" json:"upstream_services,omitempty"` } +func (x *CreateCheckRequest) Validate() error { + if x.Name == "" { + return errors.New("Name is required") + } + if len(x.Profiles) == 0 { + return errors.New("At least one profile is required") + } + if len(x.UpstreamServices) == 0 { + return errors.New("At least one upstream service is required") + } + return nil +} + Suggestion importance[1-10]: 7 Why: Implementing a validation method for CreateCheckRequest is essential to ensure that all required fields are provided before processing, which helps in maintaining data consistency and preventing errors.	7
	Add a validation method to ensure the Name field in CreateProfileRequest is not empty Add a method to CreateProfileRequest to validate that the Name field is not empty, ensuring that profiles are created with valid data. internal/grpc/sophrosyne/v0/profiles.pb.go [352-362] func (x *CreateProfileRequest) GetName() string { if x != nil { return x.Name } return "" } func (x *CreateProfileRequest) GetChecks() []string { if x != nil { return x.Checks } return nil } +func (x *CreateProfileRequest) Validate() error { + if x.GetName() == "" { + return fmt.Errorf("name field cannot be empty") + } + return nil +} + Suggestion importance[1-10]: 7 Why: Ensuring that the Name field is not empty is crucial for data integrity and prevents creating profiles with invalid or incomplete data.	7
Possible issue	Add validation to ensure Id and Name fields are not empty in GetCheckRequest Consider adding validation for the Id and Name fields in the GetCheckRequest struct to ensure they are not empty when they are used. This can prevent potential issues when these fields are required but not provided. internal/grpc/sophrosyne/v0/checks.pb.go [92-102] -func (x *GetCheckRequest) GetId() string { +func (x *GetCheckRequest) GetId() (string, error) { if x, ok := x.GetCheck().(*GetCheckRequest_Id); ok { - return x.Id + if x.Id == "" { + return "", errors.New("Id cannot be empty") + } + return x.Id, nil } - return "" + return "", errors.New("Id not set") } -func (x *GetCheckRequest) GetName() string { +func (x *GetCheckRequest) GetName() (string, error) { if x, ok := x.GetCheck().(*GetCheckRequest_Name); ok { - return x.Name + if x.Name == "" { + return "", errors.New("Name cannot be empty") + } + return x.Name, nil } - return "" + return "", errors.New("Name not set") } Suggestion importance[1-10]: 8 Why: Adding validation for Id and Name fields in GetCheckRequest is crucial to ensure data integrity and prevent runtime errors from unhandled empty values.	8
	Add a validation check to ensure only one of Id or Name is set in GetProfileRequest Consider adding a validation check in the GetProfileRequest methods to ensure that either Id or Name is set, but not both, to avoid potential conflicts or unexpected behavior. internal/grpc/sophrosyne/v0/profiles.pb.go [92-102] func (x *GetProfileRequest) GetId() string { if x, ok := x.GetProfile().(*GetProfileRequest_Id); ok { return x.Id } return "" } func (x *GetProfileRequest) GetName() string { if x, ok := x.GetProfile().(*GetProfileRequest_Name); ok { return x.Name } return "" } +func (x *GetProfileRequest) Validate() error { + if x.GetId() != "" && x.GetName() != "" { + return fmt.Errorf("only one of Id or Name should be set") + } + return nil +} + Suggestion importance[1-10]: 8 Why: Adding a validation check to ensure mutual exclusivity between Id and Name fields can prevent logical errors and conflicts, which is crucial for the system's integrity.	8
	Add a check for the error message in the "client remote error logged as non-json" test case The TestStartup function has a test case "client remote error logged as non-json" that does not check the error message. Adding a check for the error message can ensure that the correct error is being logged. tests/integration/startup_test.go [328-329] require.Nil(t, tlsConn) +require.Contains(t, err.Error(), "remote error") Suggestion importance[1-10]: 6 Why: Adding a check for the specific error message can help ensure that the test is catching the correct type of error, thus improving the test's accuracy and reliability.	6
Possible bug	Add error handling to prevent nil pointer dereferences in GetProfileRequest methods Add error handling in the GetProfileRequest methods to handle cases where the Profile field is not set, to prevent potential nil pointer dereferences. internal/grpc/sophrosyne/v0/profiles.pb.go [92-102] func (x *GetProfileRequest) GetId() string { + if x.GetProfile() == nil { + return "" + } if x, ok := x.GetProfile().(*GetProfileRequest_Id); ok { return x.Id } return "" } func (x *GetProfileRequest) GetName() string { + if x.GetProfile() == nil { + return "" + } if x, ok := x.GetProfile().(*GetProfileRequest_Name); ok { return x.Name } return "" } Suggestion importance[1-10]: 8 Why: Adding error handling for nil checks in the GetProfileRequest methods is essential to prevent runtime errors from nil pointer dereferences, which is a significant improvement.	8
	Add error handling in the ProtoReflect method to prevent potential runtime panics Add error handling for the ProtoReflect method to handle cases where MessageInfo is not properly loaded, which can prevent potential runtime panics. internal/grpc/sophrosyne/v0/checks.pb.go [68-77] func (x *GetCheckRequest) ProtoReflect() protoreflect.Message { mi := &file_sophrosyne_v0_checks_proto_msgTypes[0] if protoimpl.UnsafeEnabled && x != nil { ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x)) if ms.LoadMessageInfo() == nil { - ms.StoreMessageInfo(mi) + if err := ms.StoreMessageInfo(mi); err != nil { + log.Printf("Failed to store message info: %v", err) + return nil + } } return ms } return mi.MessageOf(x) } Suggestion importance[1-10]: 6 Why: Adding error handling in the ProtoReflect method is a good practice to ensure stability and prevent potential runtime panics due to unhandled errors.	6
Validation	Add validation to ensure that either Text or Image is provided in the ScanRequest, but not both Consider adding validation logic to ensure that either Text or Image is provided in the ScanRequest but not both. This will help prevent potential misuse of the API where both fields are set or neither is set. internal/grpc/sophrosyne/v0/scans.pb.go [45-56] type ScanRequest struct { ... Profile string `protobuf:"bytes,1,opt,name=profile,proto3" json:"profile,omitempty"` // Types that are assignable to Kind: // // *ScanRequest_Text // *ScanRequest_Image Kind isScanRequest_Kind `protobuf_oneof:"kind"` } +func (x *ScanRequest) Validate() error { + if x.GetText() != "" && x.GetImage() != nil { + return fmt.Errorf("only one of Text or Image should be provided") + } + if x.GetText() == "" && x.GetImage() == nil { + return fmt.Errorf("either Text or Image must be provided") + } + return nil +} + Suggestion importance[1-10]: 8 Why: This suggestion is crucial as it prevents potential misuse of the API by ensuring that either Text or Image is provided, but not both, which is important for the correct functioning of the system.	8
Robustness	Add a retry mechanism with backoff strategy when creating a new gRPC client connection The newClients function creates a new gRPC client connection but does not handle the case where the connection might fail. Adding a retry mechanism with a backoff strategy can improve the robustness of the test setup. tests/integration/startup_test.go [95-100] -conn, err := googlegrpc.NewClient(te.endpoint, googlegrpc.WithTransportCredentials(credentials.NewTLS(&tls.Config{ - InsecureSkipVerify: true, //nolint:gosec // this is just for testing -}))) +var conn *googlegrpc.ClientConn +var err error +for i := 0; i < 3; i++ { + conn, err = googlegrpc.NewClient(te.endpoint, googlegrpc.WithTransportCredentials(credentials.NewTLS(&tls.Config{ + InsecureSkipVerify: true, //nolint:gosec // this is just for testing + }))) + if err == nil { + break + } + time.Sleep(time.Second * time.Duration(i+1)) +} if err != nil { t.Fatalf("could not create client: %s", err) } Suggestion importance[1-10]: 7 Why: Adding a retry mechanism can indeed improve the robustness of establishing a gRPC connection in tests, especially in environments with potential transient network issues.	7
	Add error handling for the ProtoReflect method to handle cases where ms.LoadMessageInfo() returns an error Consider adding error handling for the ProtoReflect method to handle cases where ms.LoadMessageInfo() returns an error. This will make the code more robust and easier to debug. internal/grpc/sophrosyne/v0/scans.pb.go [73-82] func (x *ScanRequest) ProtoReflect() protoreflect.Message { mi := &file_sophrosyne_v0_scans_proto_msgTypes[0] if protoimpl.UnsafeEnabled && x != nil { ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x)) - if ms.LoadMessageInfo() == nil { - ms.StoreMessageInfo(mi) + if err := ms.LoadMessageInfo(); err != nil { + log.Printf("Error loading message info: %v", err) + return nil } + ms.StoreMessageInfo(mi) return ms } return mi.MessageOf(x) } Suggestion importance[1-10]: 6 Why: Adding error handling makes the code more robust and easier to debug, which is important for maintaining the reliability of the system.	6
Performance	Use sync.Pool to reuse instances of GetCheckRequest and GetCheckResponse for better performance To improve performance, consider using a sync.Pool for the GetCheckRequest and GetCheckResponse structs to reuse instances and reduce garbage collection overhead. internal/grpc/sophrosyne/v0/checks.pb.go [53-60] +var getCheckRequestPool = sync.Pool{ + New: func() interface{} { + return &GetCheckRequest{} + }, +} + +var getCheckResponsePool = sync.Pool{ + New: func() interface{} { + return &GetCheckResponse{} + }, +} + func (x *GetCheckRequest) Reset() { *x = GetCheckRequest{} if protoimpl.UnsafeEnabled { mi := &file_sophrosyne_v0_checks_proto_msgTypes[0] ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x)) ms.StoreMessageInfo(mi) } + getCheckRequestPool.Put(x) } func (x *GetCheckResponse) Reset() { *x = GetCheckResponse{} if protoimpl.UnsafeEnabled { mi := &file_sophrosyne_v0_checks_proto_msgTypes[1] ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x)) ms.StoreMessageInfo(mi) } + getCheckResponsePool.Put(x) } Suggestion importance[1-10]: 7 Why: Using sync.Pool for GetCheckRequest and GetCheckResponse can significantly improve performance by reducing garbage collection overhead, which is beneficial in high-throughput environments.	7
	Use sync.Pool to reuse ScanRequest and ScanResponse objects for better performance To improve performance, consider using sync.Pool for reusing ScanRequest and ScanResponse objects, which can help reduce the overhead of frequent allocations and deallocations. internal/grpc/sophrosyne/v0/scans.pb.go [58-154] +var scanRequestPool = sync.Pool{ + New: func() interface{} { + return &ScanRequest{} + }, +} + +var scanResponsePool = sync.Pool{ + New: func() interface{} { + return &ScanResponse{} + }, +} + func (x *ScanRequest) Reset() { *x = ScanRequest{} if protoimpl.UnsafeEnabled { mi := &file_sophrosyne_v0_scans_proto_msgTypes[0] ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x)) ms.StoreMessageInfo(mi) } + scanRequestPool.Put(x) } func (x *ScanResponse) Reset() { *x = ScanResponse{} if protoimpl.UnsafeEnabled { mi := &file_sophrosyne_v0_scans_proto_msgTypes[1] ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x)) ms.StoreMessageInfo(mi) } + scanResponsePool.Put(x) } Suggestion importance[1-10]: 7 Why: Using sync.Pool for object reuse can significantly improve performance by reducing garbage collection overhead, which is beneficial in high-throughput environments.	7
	Preallocate the slice for Profiles in GetProfilesResponse if the total number of profiles is known Optimize the GetProfilesResponse methods by preallocating the slice for Profiles if the total number of profiles is known, to improve performance. internal/grpc/sophrosyne/v0/profiles.pb.go [290-294] func (x *GetProfilesResponse) GetProfiles() []*GetProfileResponse { if x != nil { + if x.Total > 0 && len(x.Profiles) == 0 { + x.Profiles = make([]*GetProfileResponse, 0, x.Total) + } return x.Profiles } return nil } Suggestion importance[1-10]: 6 Why: Preallocating slices based on known totals can enhance performance by reducing the need for slice growth during runtime, although it's a moderate improvement.	6
Maintainability	Refactor repeated code for resetting message states into a helper function to reduce duplication To enhance maintainability, consider refactoring the repeated code for resetting message states into a helper function. This will reduce code duplication and make future changes easier. internal/grpc/sophrosyne/v0/scans.pb.go [58-215] -func (x *ScanRequest) Reset() { - *x = ScanRequest{} +func resetMessage(x protoimpl.Message, mi *protoimpl.MessageInfo) { + *x = protoimpl.Message{} if protoimpl.UnsafeEnabled { - mi := &file_sophrosyne_v0_scans_proto_msgTypes[0] ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x)) ms.StoreMessageInfo(mi) } } +func (x *ScanRequest) Reset() { + resetMessage(x, &file_sophrosyne_v0_scans_proto_msgTypes[0]) +} + func (x *ScanResponse) Reset() { - *x = ScanResponse{} - if protoimpl.UnsafeEnabled { - mi := &file_sophrosyne_v0_scans_proto_msgTypes[1] - ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x)) - ms.StoreMessageInfo(mi) - } + resetMessage(x, &file_sophrosyne_v0_scans_proto_msgTypes[1]) } func (x *CheckResult) Reset() { - *x = CheckResult{} - if protoimpl.UnsafeEnabled { - mi := &file_sophrosyne_v0_scans_proto_msgTypes[2] - ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x)) - ms.StoreMessageInfo(mi) - } + resetMessage(x, &file_sophrosyne_v0_scans_proto_msgTypes[2]) } Suggestion importance[1-10]: 5 Why: Refactoring repeated code into a helper function improves maintainability and reduces the risk of errors in future modifications, although it's a relatively common practice in code organization.	5

[qodo-merge-pro]

PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

CI Failure Feedback 🧐

Action: sonarcloud

Failed stage: Run mise run test:unit [❌]

Failed test name: TestProcessCheckResults_IgnoresCheckResultsWithEmptyNames

Failure summary: The action failed due to the following reasons: Several comments in the codebase did not end with a period, violating the godot linter rule. The test TestProcessCheckResults_IgnoresCheckResultsWithEmptyNames failed because an assertion expected to be true was false.

Relevant error logs: 1: ##[group]Operating System 2: Ubuntu ... 232: ^ 233: internal/pgx/pgx.go:39:48: Comment should end in a period (godot) 234: // Name of the default profile in the database 235: ^ 236: internal/pgx/NewUserService_test.go:34:65: Comment should end in a period (godot) 237: // UserService is successfully created when all inputs are valid 238: ^ 239: internal/pgx/NewUserService_test.go:67:50: Comment should end in a period (godot) 240: // Database connection fails during pool creation 241: ^ 242: internal/pgx/NewUserService_test.go:92:52: Comment should end in a period (godot) 243: // Database query fails when running createRootUser ... 253: ^ 254: internal/pgx/UserService_getUser_test.go:171:43: Comment should end in a period (godot) 255: // Query execution returns exactly one row 256: ^ 257: internal/pgx/getUserDbReturn_ToUser_test.go:32:76: Comment should end in a period (godot) 258: // Verify that all fields from getUserDbReturn are correctly mapped to User 259: ^ 260: internal/pgx/getUserDbReturn_ToUser_test.go:72:68: Comment should end in a period (godot) 261: // Test with a nil DeletedAt to ensure it is handled without errors ... 282: github.com/madsrc/sophrosyne/cmd/dummycheck coverage: 0.0% of statements 283: github.com/madsrc/sophrosyne/cmd/sophrosyne coverage: 0.0% of statements 284: github.com/madsrc/sophrosyne/internal/cedar coverage: 0.0% of statements 285: ok github.com/madsrc/sophrosyne/internal/cache 0.224s coverage: 40.0% of statements 286: ok github.com/madsrc/sophrosyne/internal/configProvider 0.324s coverage: 25.9% of statements 287: github.com/madsrc/sophrosyne/internal/grpc/interceptors coverage: 0.0% of statements 288: github.com/madsrc/sophrosyne/internal/grpc/sophrosyne/v0 coverage: 0.0% of statements 289: github.com/madsrc/sophrosyne/internal/healthchecker coverage: 0.0% of statements 290: --- FAIL: TestProcessCheckResults_IgnoresCheckResultsWithEmptyNames (0.00s) 291: processCheckResults_test.go:67: 292: Error Trace: /home/runner/work/sophrosyne/sophrosyne/internal/grpc/processCheckResults_test.go:67 293: Error: Should be true ... 303: github.com/madsrc/sophrosyne/internal/mocks/internal_/pgx coverage: 0.0% of statements 304: github.com/madsrc/sophrosyne/internal/otel coverage: 0.0% of statements 305: ? github.com/madsrc/sophrosyne/tests/integration [no test files] 306: ok github.com/madsrc/sophrosyne/internal/pgx 0.011s coverage: 6.3% of statements 307: ok github.com/madsrc/sophrosyne/internal/tls 0.818s coverage: 44.0% of statements 308: ok github.com/madsrc/sophrosyne/internal/validator 0.011s coverage: 3.4% of statements 309: FAIL 310: mise [test:unit] exited with code 1 311: ##[error]Process completed with exit code 1.

---

✨ CI feedback usage guide:

---

The CI feedback tool (/checks) automatically triggers when a PR has a failed check.
The tool analyzes the failed checks and provides several feedbacks:

• Failed stage
• Failed test name
• Failure summary
• Relevant error logs

In addition to being automatically triggered, the tool can also be invoked manually by commenting on a PR:

/checks "https://github.com/{repo_name}/actions/runs/{run_number}/job/{job_number}"

where {repo_name} is the name of the repository, {run_number} is the run number of the failed check, and {job_number} is the job number of the failed check.

Configuration options

• enable_auto_checks_feedback - if set to true, the tool will automatically provide feedback when a check is failed. Default is true.
• excluded_checks_list - a list of checks to exclude from the feedback, for example: ["check1", "check2"]. Default is an empty list.
• enable_help_text - if set to true, the tool will provide a help message with the feedback. Default is true.
• persistent_comment - if set to true, the tool will overwrite a previous checks comment with the new feedback. Default is true.
• final_update_message - if persistent_comment is true and updating a previous checks message, the tool will also create a new message: "Persistent checks updated to latest commit". Default is true.

See more information about the checks tool in the docs.

[sonarqubecloud]

Quality Gate failed

Failed conditions
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint