[sql-55] firewalldb: actions mig handle empty RPCParamsJson #1156
Conversation
If there are no RPCParamsJson set for an action, the value is represented differently in KVDB vs SQL. In the SQL DB, empty RPCParamsJson are represented as nil, while they are represented as an empty array in the KVDB version. Therefore, we need to override the RPCParamsJson in that scenario, so that they are set to the same representation when a KVDB and an SQL action is compared.
Summary of ChangesHello @ViktorT-11, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical data representation discrepancy encountered during the migration of firewall actions from KVDB to SQL. Specifically, it rectifies the inconsistent handling of empty Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly addresses the discrepancy in representing empty RPCParamsJson between KVDB and SQL databases during migration by overriding the value for comparison. The main logic in sql_migration.go is sound. However, I've found a couple of issues in the test files that need to be addressed. One is an incorrect modification in a test helper function that affects the validation logic, and another is a helper function that generates invalid JSON. My review provides specific suggestions to resolve these points.
| // In case the actionReq's RPCParamsJson wasn't set, we need to set the | ||
| // expected action's RPCParamsJson to nil as that's how such | ||
| // RPCParamsJson are represented in | ||
| if len(actionReq.RPCParamsJson) == 0 { | ||
| action.RPCParamsJson = nil | ||
| } |
There was a problem hiding this comment.
This logic incorrectly sets action.RPCParamsJson to nil for the expected action. According to the pull request description, an empty RPCParamsJson in KVDB is represented as an empty slice ([]byte{}), not nil. The expected action (kvAction) used for validation should reflect the state as it is in KVDB. This change wrongly aligns the expected KVDB action with the SQL representation, which undermines the purpose of the overrideRPCParamsJson function and could lead to incorrect test outcomes. Additionally, the comment on line 2002 is incomplete.
This block should be removed to ensure the test correctly validates the migration logic.
| func randomJSON(n int) (string, error) { | ||
| // When 0 pairs are requested, we can return immediately. | ||
| if n <= 0 { | ||
| return "", nil |
There was a problem hiding this comment.
The function randomJSON should return a valid JSON string. For n=0, it should return "{}" (an empty JSON object), but it currently returns "" (an empty string), which is not a valid JSON representation for an object. This is problematic as the StructuredJsonData field is expected to contain a serialized JSON string.
If the intention is to test cases with empty RPCParamsJson, it would be cleaner to handle this explicitly in the randomActions test helper, rather than having randomJSON produce invalid data.
| return "", nil | |
| return "{}", nil |
1 participant
If there are no RPCParamsJson set for an action, the value is represented differently in KVDB vs SQL.
In the SQL DB, empty RPCParamsJson are represented as
nil, while they are represented as an empty array in the KVDB version. This PR addresses that issue, by overriding the RPCParamsJson in that scenario, so that they are set to the same representation when a KVDB and an SQL action is compared.