Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

putjson and getjson #21

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

putjson and getjson #21

wants to merge 1 commit into from

Conversation

JasonWei530
Copy link

putjson and getjson by Zexuan Wei.
English comments.

Copy link
Owner

c4pt0r commented Apr 11, 2023

Hello, I am a serverless review bot on flows.network. Here are my reviews of code commits in this PR.


@c4pt0r
Copy link
Owner

c4pt0r commented Apr 11, 2023

summary

Copy link
Owner

c4pt0r commented Apr 11, 2023

Hello, I am a serverless review bot on flows.network. Here are my reviews of code commits in this PR.


@c4pt0r
Copy link
Owner

c4pt0r commented Apr 11, 2023

summary

Copy link
Owner

c4pt0r commented Apr 11, 2023

Hello, I am a serverless review bot on flows.network. Here are my reviews of code commits in this PR.


Commit 1

This patch adds two new features, putjson and getjson, to the existing repository. The key changes are as follows:

  1. Added new files for putjson and getjson: kvcmds/cmd_put_json.go and kvcmds/cmd_get_json.go.

  2. Modified the main.go file to include the new putjson and getjson commands.

  3. Implemented PutJson and GetJson methods in the rawkv_client.go and the txnkv_client.go files.

  4. Imported "github.com/oliveagle/jsonpath" package in go.mod and added entries in go.sum.

Potential problems:

  1. The json_separator constant's string value ("Xuan123#@!xuan") might not be unique enough and may collide with actual user input. It could be problematic if a user's input also includes the same string, causing incorrect parsing.

  2. The PutJson feature reads input in a special way, explicitly treating it as a multiline input. This approach, while functional, might not be the best way to handle this type of input (e.g., working with JSON values that do not span across multiple lines).

  3. There is no clear input validation for JSON values or error handling if an invalid JSON is entered.

  4. No tests are provided in the patch to ensure that the new putjson and getjson features work as expected. Without tests, it's hard to be certain that the code functions without any issues.

It is advised to address these potential problems, add tests for the new features, and provide clear documentation on how to use the new commands.

@c4pt0r
Copy link
Owner

c4pt0r commented Apr 11, 2023

summary

Copy link
Owner

c4pt0r commented Apr 11, 2023

Hello, I am a serverless review bot on flows.network. Here are my reviews of code commits in this PR.


Commit 1

Summary of changes:

  1. Added new commands for JSON data support: PutJsonCmd and GetJsonCmd.
  2. Updated the client interface and added new methods, PutJson and GetJson, to support JSON data operations.
  3. Modified the rawkv_client.go and txnkv_client.go files to implement these new methods.
  4. The cli/main.go file was updated to handle multi-line input for the PutJson command, using a custom separator (Json_separator).
  5. Dependency for "github.com/oliveagle/jsonpath" was included in go.mod and go.sum files.

Potential problems:

  1. It is not clear if the custom separator (Json_separator) will work in all cases. If the user input contains the same value as Json_separator, it may cause issues.
  2. The added dependency, "github.com/oliveagle/jsonpath," has no direct use in the current patch. This could be an unnecessary addition that increases the project size and maintenance.

@c4pt0r
Copy link
Owner

c4pt0r commented Apr 11, 2023

summary

Copy link
Owner

c4pt0r commented Apr 11, 2023

Hello, I am a serverless review bot on flows.network. Here are my reviews of code commits in this PR.


Commit 1

The key changes introduced in this patch are:

  1. Added PutJson and GetJson methods to the Client interface in client.go.
  2. Implemented PutJson and GetJson methods for rawkvClient and txnkvClient in rawkv_client.go and txnkv_client.go, respectively.
  3. Modified the main.go file to include the PutJson and GetJson methods in the command-line interface (CLI) and handle multi-line JSON inputs.
  4. Created new Go files: cmd_put_json.go and cmd_get_json.go in the kvcmds directory, which handle the PutJson and GetJson methods, respectively.

Potential problems or concerns:

  1. The JSON separator defined in the main.go file (Json_separator) is a hard-coded string, which could result in conflicts if a user input contains the same string. Consider using a more unique or dynamic separator.
  2. The code is not consistent with how the context is passed to different methods. Sometimes it uses context.TODO(), and sometimes it uses the provided ctx. This may lead to potential issues if the context is supposed to be shared or if the context's values are required among these function calls. A possible solution would be to keep the context usage uniform across the application.
  3. The error handling for PutJson and GetJson methods might need to be reviewed to ensure that it is consistent with the existing methods, such as Put and Get. Make sure error reporting is appropriate and consistent throughout the files.
  4. The handling of multi-line input from the user for the PutJson command might be improved or replaced by a more robust solution. This could minimize potential input issues or edge cases.

Overall, the patch introduces new functionality for handling JSON values in the KeyValue store with new PutJson and GetJson methods. The notable changes are the additions to the Client interface, the implementations in rawkv_client.go and txnkv_client.go, the new cmd_put_json.go and cmd_get_json.go files, and the CLI updates in main.go.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants