Refactor and cleanup DatabaseClient

mrheinen commented 1 month ago

User description

Update and cleanup databaseclient and it's usage.

PR Type

Enhancement, Bug fix

Description

Refactored database queries to use search-based methods instead of bulk retrieval
Removed several 'GetAll*' endpoints and methods across the codebase
Added external versioning (ext_version) and UUID (ext_uuid) fields to Content, ContentRule, and Application models
Updated database schema to include new external versioning fields
Modified API endpoints in the UI to use new segment-based search
Improved error handling and logging in various parts of the codebase
Updated tests to reflect changes in error handling and database queries
Added new Bazel build file for database rules package
Removed dependency on KSQL library

Changes walkthrough 📝

Relevant files

Enhancement

8 files

api_server.go `Streamline API endpoints and remove bulk retrieval` cmd/api/api_server.go Removed several 'HandleGetAll*' endpoints for various resources Updated routing to use segment-based search endpoints instead of 'all' endpoints Simplified some endpoint handlers	+0/-6
server.go `Refactor API server handlers and remove bulk retrieval methods` pkg/api/server.go Removed several 'HandleGetAll*' functions Updated 'HandleGetWhoisForIP' to use 'SearchWhois' instead of 'GetWhoisByIP' Simplified error handling in some functions	+9/-73
backend.go `Refactor backend to use search-based queries` pkg/backend/backend.go Updated 'SendStatus' and 'HandleProbe' to use 'SearchHoneypots' instead of 'GetHoneypotByIP' Modified 'HandleUploadFile' to use 'SearchDownloads' instead of 'GetDownloadBySum' Updated 'LoadRules' to use 'SearchContentRules' instead of 'GetContentRules'	+17/-11
database.go `Refactor database models and client interface` pkg/database/database.go Added 'ExternalDataModel' interface with version and UUID fields Updated 'Content', 'ContentRule', and 'Application' structs with external version and UUID fields Removed several 'Get*' methods from the DatabaseClient interface Added 'SearchWhois' method to the DatabaseClient interface	+86/-157
rdap.go `Update WHOIS lookup to use search-based query` pkg/whois/rdap.go Updated 'LookupIP' to use 'SearchWhois' instead of 'GetWhoisByIP' Improved error handling and logging	+12/-6
RuleForm.vue `Update RuleForm to use new app search endpoint` ui/src/components/container/RuleForm.vue - Updated API endpoint for loading apps to use segment-based search	+1/-1
RulesList.vue `Update RulesList to use new app search endpoint` ui/src/components/container/RulesList.vue - Updated API endpoint for loading apps to use segment-based search	+1/-1
database.sql `Update database schema for external versioning` config/database.sql Added 'ext_version' and 'ext_uuid' columns to content, content_rule, and app tables	+6/-2

Tests

1 files

backend_test.go

Update backend tests for new search-based queries

pkg/backend/backend_test.go

Updated test cases for 'SendStatus' to reflect changes in error
handling

Modified expectations for honeypot insertion and updates

+18/-7

Documentation

1 files

SEARCH_KEYWORDS.md

Update search keywords documentation

SEARCH_KEYWORDS.md

Added 'ext_uuid' and 'ext_version' fields to Content, ContentRule, and
Application sections

+6/-0

Dependencies

1 files

BUILD.bazel

Remove KSQL dependency from API package

pkg/api/BUILD.bazel - Removed dependency on 'com_github_vingarcia_ksql'

+0/-1

Configuration changes

1 files

BUILD.bazel

Add Bazel build file for database rules

pkg/database/rules/BUILD.bazel - Added new Bazel build file for database rules package

+16/-0

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

github-actions[bot] commented 1 month ago

Preparing PR description...

mrheinen commented 1 month ago

This prepares for moving to using a database client with generics

github-actions[bot] commented 1 month ago

Preparing review...

codiumai-pr-agent-pro[bot] commented 1 month ago

PR Reviewer Guide 🔍

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Key issues to review

API Changes
Significant changes to the DatabaseClient interface, including removal of several methods and addition of new ones. This may affect other parts of the codebase that depend on these methods. Data Model Changes
New fields (ExtVersion and ExtUuid) added to Content, ContentRule, and Application structs. Ensure these changes are properly reflected in the database schema and all relevant code. Error Handling
The error handling in the LookupIP function has changed. Ensure that all possible error cases are properly handled and logged.

codiumai-pr-agent-pro[bot] commented 1 month ago

PR Code Suggestions ✨

Category	Suggestion	Score
Performance	Implement pagination for loading rules to improve performance with large datasets ___ Consider adding pagination to the `LoadRules` function to avoid potential performance issues when dealing with a large number of rules. This can be achieved by implementing a batch processing approach. [pkg/backend/backend.go [742-743]](https://github.com/mrheinen/lophiid/pull/36/files#diff-c65bcfe9bb457434c3e69ba3f0576d7669935f350d24e2c2c58b05b4f9c510b2R742-R743) ```diff -// TODO: Add logic that allowes only active rules to be selected here -rules, err := s.dbClient.SearchContentRules(0, 9999999, "") +const batchSize = 1000 +var allRules []database.ContentRule +offset := 0 +for { + rules, err := s.dbClient.SearchContentRules(offset, batchSize, "") + if err != nil { + return err + } + allRules = append(allRules, rules...) + if len(rules) < batchSize { + break + } + offset += batchSize +} + ``` - [ ] Apply this suggestion Suggestion importance[1-10]: 9 Why: Implementing pagination for loading rules is crucial for performance when dealing with large datasets, preventing potential memory issues.	9
Error handling	Improve error handling for honeypot search results ___ Consider adding error handling for the case when no honeypots are found in the `SearchHoneypots` method. This will provide more robust error handling and prevent potential nil pointer dereferences. [pkg/database/database.go [652-660]](https://github.com/mrheinen/lophiid/pull/36/files#diff-1adb887d06a44193c36fc1c5708be385f3129cd59c2f2aa555faa065941ed877R652-R660) ```diff dms, err := s.dbClient.SearchHoneypots(0, 1, fmt.Sprintf("ip:%s", sReq.HoneypotIP)) -if err != nil \|\| len(hps) == 0 { - slog.Warn("error finding honeypot", slog.String("error", err.Error()), slog.String("honeypot", sReq.HoneypotIP)) +if err != nil { + slog.Error("error searching for honeypot", slog.String("error", err.Error()), slog.String("honeypot", sReq.HoneypotIP)) + matchedRule = s.safeRules.Get()[0] +} else if len(dms) == 0 { + slog.Warn("no honeypot found", slog.String("honeypot", sReq.HoneypotIP)) matchedRule = s.safeRules.Get()[0] } else { - matchedRule.ContentID = hps[0].DefaultContentID + matchedRule.ContentID = dms[0].DefaultContentID matchedRule.ID = 0 } ``` - [ ] Apply this suggestion Suggestion importance[1-10]: 8 Why: The suggestion improves error handling and provides more detailed logging, which enhances debugging and robustness of the code.	8
Best practice	Use pointer receiver for setter method to modify the original struct ___ Consider using a pointer receiver for the `SetModelID` method of `Content` struct. This allows for modifying the original struct instance rather than a copy. [pkg/database/database.go [88]](https://github.com/mrheinen/lophiid/pull/36/files#diff-1adb887d06a44193c36fc1c5708be385f3129cd59c2f2aa555faa065941ed877R88-R88) ```diff +func (c Content) SetModelID(id int64) { c.ID = id } func (c Content) SetModelID(id int64) { c.ID = id } ``` - [ ] Apply this suggestion Suggestion importance[1-10]: 7 Why: Using a pointer receiver for the SetModelID method is a good practice as it allows modifying the original struct instance, improving efficiency.	7
Code structure	Restructure database query and error handling for improved readability ___ Consider moving the database query outside of the error handling block to improve code readability and avoid unnecessary nesting. This will make the logic flow more clear and easier to maintain. [pkg/whois/rdap.go [166-177]](https://github.com/mrheinen/lophiid/pull/36/files#diff-e28100c5c0bcf097b7a414bde2c3d8f1b0fa075281de07b908ace5e022520848R166-R177) ```diff hps, err := c.dbClient.SearchWhois(0, 1, fmt.Sprintf("ip:%s", ip)) if err != nil { slog.Error("Failed to query whois in database", slog.String("error", err.Error())) - -} else { - if len(hps) != 0 { - // Update the cache to recored we already have an entry. In the future we - // might want to look at the age of the entry. - c.ipCache.Store(ip, true) - return nil - } + return err } +if len(hps) != 0 { + // Update the cache to record we already have an entry. In the future we + // might want to look at the age of the entry. + c.ipCache.Store(ip, true) + return nil +} + ``` - [ ] Apply this suggestion Suggestion importance[1-10]: 6 Why: The suggestion improves code readability and structure, making it easier to maintain, but doesn't address a critical issue or bug.	6

mrheinen / lophiid