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

feat: wire v2 handlers #22112

Merged
merged 32 commits into from
Oct 16, 2024
Merged

feat: wire v2 handlers #22112

merged 32 commits into from
Oct 16, 2024

Conversation

randygrok
Copy link
Collaborator

@randygrok randygrok commented Oct 3, 2024

Description

ref: #21682


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title, you can find examples of the prefixes below:
  • confirmed ! in the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • reviewed "Files changed" and left comments if necessary
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • updated the relevant documentation or specification, including comments for documenting Go code
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic, API design and naming, documentation is accurate, tests and test coverage

Summary by CodeRabbit

  • New Features

    • Introduced a REST API for querying blockchain data, including a QueryBalanceRequest endpoint.
    • Added support for handling RESTful requests in the application command initialization.
    • Enhanced HTTP server configuration options for improved flexibility.
    • Implemented a generic HTTP server for managing RESTful API requests.
  • Documentation

    • Added a README file detailing the usage of the Cosmos SDK REST API, including examples for querying account balances.

Copy link
Contributor

coderabbitai bot commented Oct 3, 2024

📝 Walkthrough
📝 Walkthrough

Walkthrough

The pull request focuses on updates to the go.mod files for the runtime/v2 and server/v2 modules, specifically managing dependencies. In runtime/v2/go.mod, a direct dependency on github.com/stretchr/testify v1.9.0 is added, while an indirect dependency on the same package is removed and replaced with a new indirect dependency on github.com/stretchr/objx v0.5.2. In server/v2/go.mod, a new direct requirement for github.com/gogo/protobuf v1.3.2 is added, and the previous indirect requirement for this package is removed. Additionally, new functionality and documentation are introduced in various files within the server/v2/api/rest directory.

Changes

File Change Summary
runtime/v2/go.mod Added github.com/stretchr/testify v1.9.0, removed indirect dependency on github.com/stretchr/testify v1.9.0, added github.com/stretchr/objx v0.5.2 (indirect).
server/v2/go.mod Added github.com/gogo/protobuf v1.3.2, removed indirect requirement for github.com/gogo/protobuf v1.3.2.
server/v2/api/rest/README.md New documentation added for the Cosmos SDK REST API, detailing endpoint interactions and examples.
server/v2/api/rest/config.go Added Config struct, DefaultConfig, OverwriteDefaultConfig, and Disable methods for server configuration.
server/v2/api/rest/handler.go Implemented a REST API handler using Go generics, added methods for validating requests and creating messages.
server/v2/api/rest/server.go Defined a generic HTTP server struct, added methods for server management and configuration.
simapp/v2/simdv2/cmd/commands.go Added import for rest package and integrated rest.New[T]() into command initialization.

Possibly related PRs

Suggested labels

C:x/accounts, C:x/gov, C:x/staking, C:x/distribution, C:x/mint, C:x/slashing, C:x/upgrade, C:x/feegrant, C:x/authz, C:x/nft, C:x/epochs, C:x/protocolpool, C:x/consensus, C:x/circuit, C:Store, C:orm, C:x/accounts/lockup, C:x/accounts/multisig, backport/v0.52.x

Suggested reviewers

  • tac0turtle
  • sontrinh16
  • aaronc
  • lucaslopezf

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

server/v2/api/rest/handler.go Fixed Show fixed Hide fixed
server/v2/api/rest/server.go Fixed Show fixed Hide fixed
@randygrok randygrok requested a review from julienrbrt October 7, 2024 16:37
@randygrok randygrok marked this pull request as ready for review October 7, 2024 16:37
@randygrok randygrok requested review from a team, kocubinski and hieuvubk as code owners October 7, 2024 16:37
server/v2/go.mod Outdated Show resolved Hide resolved
@randygrok randygrok requested a review from julienrbrt October 14, 2024 10:55
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🧹 Outside diff range and nitpick comments (3)
server/v2/api/rest/config.go (2)

12-18: LGTM with a minor suggestion: Config struct is well-defined.

The Config struct is well-documented and properly annotated for configuration parsing. The field names and types are appropriate for their purposes.

Consider updating the comment for the Config struct to use "REST server" instead of "REST server" for consistency with the component name, as suggested in a past review comment:

- // Config defines configuration for the REST server.
+ // Config defines configuration for the REST server.

27-32: LGTM with a minor suggestion: Disable function is well-implemented.

The Disable function provides a convenient way to disable the REST server and is implemented correctly.

Consider updating the comment to improve clarity:

- // Disable the rest server by default (default enabled).
+ // Disable returns a CfgOption that disables the REST server.

This change more accurately describes the function's behavior and purpose.

server/v2/api/rest/README.md (1)

60-73: LGTM: Helpful curl example, minor formatting fix needed

The Using Tools section provides a clear and helpful example of how to use curl to interact with the API. The curl command is well-formatted and easy to understand.

Please add a newline character at the end of the file to comply with the Markdown linting rules:

   }'

<details>
<summary>🧰 Tools</summary>

<details>
<summary>🪛 Markdownlint</summary><blockquote>

73-73: null
Files should end with a single newline character

(MD047, single-trailing-newline)

</blockquote></details>

</details>

</blockquote></details>

</blockquote></details>

<details>
<summary>📜 Review details</summary>

**Configuration used: .coderabbit.yml**
**Review profile: CHILL**

<details>
<summary>📥 Commits</summary>

Files that changed from the base of the PR and between 40b5baed095db387b55980be0229ccee33f43df5 and 506267d2efaf2417103a4b59c98f76246e01ce2e.

</details>

<details>
<summary>📒 Files selected for processing (6)</summary>

* runtime/v2/go.mod (2 hunks)
* server/v2/api/rest/README.md (1 hunks)
* server/v2/api/rest/config.go (1 hunks)
* server/v2/api/rest/handler.go (1 hunks)
* server/v2/api/rest/server.go (1 hunks)
* simapp/v2/simdv2/cmd/commands.go (2 hunks)

</details>

<details>
<summary>🚧 Files skipped from review as they are similar to previous changes (3)</summary>

* runtime/v2/go.mod
* server/v2/api/rest/server.go
* simapp/v2/simdv2/cmd/commands.go

</details>

<details>
<summary>🧰 Additional context used</summary>



<details>
<summary>📓 Path-based instructions (3)</summary><blockquote>

<details>
<summary>server/v2/api/rest/README.md (1)</summary>

Pattern `**/*.md`: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"


</details>
<details>
<summary>server/v2/api/rest/config.go (1)</summary>

Pattern `**/*.go`: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

</details>
<details>
<summary>server/v2/api/rest/handler.go (1)</summary>

Pattern `**/*.go`: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

</details>

</blockquote></details>

<details>
<summary>🪛 LanguageTool</summary><blockquote>

<details>
<summary>server/v2/api/rest/README.md</summary><blockquote>

[grammar] ~7-~7: The usual collocation for “returned” is “to”, not “in”.
Context: ...ol message (`proto`), and responses are returned in JSON format.  ## Example  ### 1. `Query...

(RETURN_IN_THE)

---

[uncategorized] ~32-~32: Loose punctuation mark.
Context: ..._DENOMINATION>"   }   ```    - `address`: Account address on the Cosmos network. ...

(UNLIKELY_OPENING_PUNCTUATION)

---

[uncategorized] ~33-~33: Loose punctuation mark.
Context: ...dress on the Cosmos network.   - `denom`: Token denomination (e.g., `stake`).  - ...

(UNLIKELY_OPENING_PUNCTUATION)

</blockquote></details>

</blockquote></details>
<details>
<summary>🪛 Markdownlint</summary><blockquote>

<details>
<summary>server/v2/api/rest/README.md</summary><blockquote>

15-15: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)

---

17-17: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)

---

19-19: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)

---

21-21: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)

---

23-23: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)

---

32-32: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)

---

33-33: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)

---

35-35: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)

---

47-47: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)

---

21-21: Expected: 4; Actual: 2
Unordered list indentation

(MD007, ul-indent)

---

32-32: Expected: 4; Actual: 2
Unordered list indentation

(MD007, ul-indent)

---

33-33: Expected: 4; Actual: 2
Unordered list indentation

(MD007, ul-indent)

---

37-37: null
Fenced code blocks should have a language specified

(MD040, fenced-code-language)

---

73-73: null
Files should end with a single newline character

(MD047, single-trailing-newline)

</blockquote></details>

</blockquote></details>

</details>

<details>
<summary>🔇 Additional comments (7)</summary><blockquote>

<details>
<summary>server/v2/api/rest/config.go (3)</summary><blockquote>

`3-8`: **LGTM: Default configuration looks good.**

The `DefaultConfig` function provides sensible default values for the REST server configuration. Enabling the server by default and using "localhost:8080" as the default address is a common and reasonable choice for local development.

---

`10-10`: **LGTM: Good use of functional options pattern.**

The `CfgOption` type alias is well-defined and follows the functional options pattern, which is a good practice in Go for flexible and extensible configuration.

---

`20-25`: **LGTM: OverwriteDefaultConfig function implemented as requested.**

The `OverwriteDefaultConfig` function has been implemented correctly, addressing the request from a previous review comment. It properly overwrites the entire configuration, and the function name and behavior match the provided comment.

</blockquote></details>
<details>
<summary>server/v2/api/rest/README.md (2)</summary><blockquote>

`5-7`: **LGTM: Clear and concise General Description**

The General Description section provides a clear and concise overview of the service's functionality. It effectively communicates the purpose and basic operation of the API.

<details>
<summary>🧰 Tools</summary>

<details>
<summary>🪛 LanguageTool</summary><blockquote>

[grammar] ~7-~7: The usual collocation for “returned” is “to”, not “in”.
Context: ...ol message (`proto`), and responses are returned in JSON format.  ## Example  ### 1. `Query...

(RETURN_IN_THE)

</blockquote></details>

</details>

---

`1-73`: **Excellent documentation for the Cosmos SDK REST API**

This README provides a comprehensive and well-structured overview of the Cosmos SDK REST API. It effectively covers all the essential aspects:

1. A clear introduction and general description of the service.
2. A detailed example demonstrating the usage of a specific endpoint (`QueryBalanceRequest`).
3. Practical information on using tools like curl to interact with the API.

The document is informative and will be very helpful for developers working with the Cosmos SDK REST API. The minor formatting and grammar issues mentioned in previous comments are the only improvements needed.

Great job on creating this valuable documentation!

<details>
<summary>🧰 Tools</summary>

<details>
<summary>🪛 LanguageTool</summary><blockquote>

[grammar] ~7-~7: The usual collocation for “returned” is “to”, not “in”.
Context: ...ol message (`proto`), and responses are returned in JSON format.  ## Example  ### 1. `Query...

(RETURN_IN_THE)

---

[uncategorized] ~32-~32: Loose punctuation mark.
Context: ..._DENOMINATION>"   }   ```    - `address`: Account address on the Cosmos network. ...

(UNLIKELY_OPENING_PUNCTUATION)

---

[uncategorized] ~33-~33: Loose punctuation mark.
Context: ...dress on the Cosmos network.   - `denom`: Token denomination (e.g., `stake`).  - ...

(UNLIKELY_OPENING_PUNCTUATION)

</blockquote></details>
<details>
<summary>🪛 Markdownlint</summary><blockquote>

15-15: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)

---

17-17: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)

---

19-19: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)

---

21-21: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)

---

23-23: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)

---

32-32: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)

---

33-33: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)

---

35-35: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)

---

47-47: Expected: asterisk; Actual: dash
Unordered list style

(MD004, ul-style)

---

21-21: Expected: 4; Actual: 2
Unordered list indentation

(MD007, ul-indent)

---

32-32: Expected: 4; Actual: 2
Unordered list indentation

(MD007, ul-indent)

---

33-33: Expected: 4; Actual: 2
Unordered list indentation

(MD007, ul-indent)

---

37-37: null
Fenced code blocks should have a language specified

(MD040, fenced-code-language)

---

73-73: null
Files should end with a single newline character

(MD047, single-trailing-newline)

</blockquote></details>

</details>

</blockquote></details>
<details>
<summary>server/v2/api/rest/handler.go (2)</summary><blockquote>

`13-14`: **Previous suggestion on replacing deprecated `jsonpb` is still applicable**

The use of `github.com/cosmos/gogoproto/jsonpb` is deprecated. Consider replacing it with `google.golang.org/protobuf/encoding/protojson` for better maintainability.

---

`85-86`: **Minimize the use of reflection for better performance and safety**

Using reflection can impact performance and may introduce runtime errors. Consider alternative methods to create new message instances without reflection, such as maintaining a registry or map of constructor functions keyed by message type.

</blockquote></details>

</blockquote></details>

</details>

<!-- This is an auto-generated comment by CodeRabbit for review status -->

server/v2/api/rest/README.md Show resolved Hide resolved
Comment on lines +1 to +7
# Cosmos SDK REST API

This document describes how to use a service that exposes endpoints based on Cosmos SDK Protobuf message types. Each endpoint responds with data in JSON format.

## General Description

The service allows querying the blockchain using any type of Protobuf message available in the Cosmos SDK application through HTTP `POST` requests. Each endpoint corresponds to a Cosmos SDK protocol message (`proto`), and responses are returned in JSON format.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Improve grammar in the introduction

The introduction is clear and informative. However, there's a minor grammar issue in the last sentence.

Apply this change to improve the grammar:

-The service allows querying the blockchain using any type of Protobuf message available in the Cosmos SDK application through HTTP `POST` requests. Each endpoint corresponds to a Cosmos SDK protocol message (`proto`), and responses are returned in JSON format.
+The service allows querying the blockchain using any type of Protobuf message available in the Cosmos SDK application through HTTP `POST` requests. Each endpoint corresponds to a Cosmos SDK protocol message (`proto`), and responses are returned as JSON format.
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
# Cosmos SDK REST API
This document describes how to use a service that exposes endpoints based on Cosmos SDK Protobuf message types. Each endpoint responds with data in JSON format.
## General Description
The service allows querying the blockchain using any type of Protobuf message available in the Cosmos SDK application through HTTP `POST` requests. Each endpoint corresponds to a Cosmos SDK protocol message (`proto`), and responses are returned in JSON format.
# Cosmos SDK REST API
This document describes how to use a service that exposes endpoints based on Cosmos SDK Protobuf message types. Each endpoint responds with data in JSON format.
## General Description
The service allows querying the blockchain using any type of Protobuf message available in the Cosmos SDK application through HTTP `POST` requests. Each endpoint corresponds to a Cosmos SDK protocol message (`proto`), and responses are returned as JSON format.
🧰 Tools
🪛 LanguageTool

[grammar] ~7-~7: The usual collocation for “returned” is “to”, not “in”.
Context: ...ol message (proto), and responses are returned in JSON format. ## Example ### 1. `Query...

(RETURN_IN_THE)


msg, err := h.createMessage(r)
if err != nil {
http.Error(w, err.Error(), http.StatusBadRequest)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Avoid exposing internal error details to clients

Revealing internal error messages to clients can expose sensitive information. Return a generic error message to the client and log the detailed error internally.

Apply this diff to return a generic error message:

-		http.Error(w, err.Error(), http.StatusBadRequest)
+		http.Error(w, "Bad Request", http.StatusBadRequest)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
http.Error(w, err.Error(), http.StatusBadRequest)
http.Error(w, "Bad Request", http.StatusBadRequest)


w.Header().Set("Content-Type", ContentTypeJSON)
if err := json.NewEncoder(w).Encode(query); err != nil {
http.Error(w, fmt.Sprintf("Error encoding response: %v", err), http.StatusInternalServerError)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Avoid exposing internal error details when encoding response

Including internal error details in the HTTP response may leak sensitive information. Return a generic error message to the client and log the detailed error internally.

Apply this diff to return a generic error message:

-		http.Error(w, fmt.Sprintf("Error encoding response: %v", err), http.StatusInternalServerError)
+		http.Error(w, "Internal Server Error", http.StatusInternalServerError)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
http.Error(w, fmt.Sprintf("Error encoding response: %v", err), http.StatusInternalServerError)
http.Error(w, "Internal Server Error", http.StatusInternalServerError)

Comment on lines 3 to 15
import (
"encoding/json"
"fmt"
"io"
"net/http"
"reflect"
"strings"

"cosmossdk.io/core/transaction"
"cosmossdk.io/server/v2/appmanager"
"github.com/cosmos/gogoproto/jsonpb"
gogoproto "github.com/cosmos/gogoproto/proto"
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Reorganize imports according to Go import conventions

To conform with the Uber Go Style Guide, organize imports into three groups separated by blank lines: standard library packages, third-party packages, and local packages.

Apply this diff to reorganize the imports:

 import (
 	"encoding/json"
 	"fmt"
 	"io"
 	"net/http"
 	"reflect"
 	"strings"

+	gogoproto "github.com/cosmos/gogoproto/proto"
+	"github.com/cosmos/gogoproto/jsonpb"

+	"cosmossdk.io/core/transaction"
+	"cosmossdk.io/server/v2/appmanager"
 )
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
import (
"encoding/json"
"fmt"
"io"
"net/http"
"reflect"
"strings"
"cosmossdk.io/core/transaction"
"cosmossdk.io/server/v2/appmanager"
"github.com/cosmos/gogoproto/jsonpb"
gogoproto "github.com/cosmos/gogoproto/proto"
)
import (
"encoding/json"
"fmt"
"io"
"net/http"
"reflect"
"strings"
gogoproto "github.com/cosmos/gogoproto/proto"
"github.com/cosmos/gogoproto/jsonpb"
"cosmossdk.io/core/transaction"
"cosmossdk.io/server/v2/appmanager"
)

Copy link
Member

@julienrbrt julienrbrt Oct 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

super nit, for consistency for the other, can we get the logger error lowercase?
additionally, could you fix conflicts?

@randygrok randygrok enabled auto-merge October 16, 2024 07:16
"fmt"
"io"
"net/http"
"reflect"

Check notice

Code scanning / CodeQL

Sensitive package import Note

Certain system packages contain functions which may be a possible source of non-determinism
@randygrok randygrok added this pull request to the merge queue Oct 16, 2024
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

📜 Review details

Configuration used: .coderabbit.yml
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between c700ad9 and db3ac74.

📒 Files selected for processing (2)
  • server/v2/api/rest/handler.go (1 hunks)
  • server/v2/api/rest/server.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • server/v2/api/rest/server.go
🧰 Additional context used
📓 Path-based instructions (1)
server/v2/api/rest/handler.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

Comment on lines +48 to +52
query, err := h.appManager.Query(r.Context(), 0, msg)
if err != nil {
http.Error(w, "Error querying", http.StatusInternalServerError)
return
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Log internal errors for better diagnostics

In the ServeHTTP method, when h.appManager.Query returns an error, the error is not logged. Logging internal errors while returning a generic message to the client aids in debugging without exposing sensitive information.

Apply this diff to log the error internally:

 if err != nil {
+	log.Printf("Error querying: %v", err)
 	http.Error(w, "Error querying", http.StatusInternalServerError)
 	return
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
query, err := h.appManager.Query(r.Context(), 0, msg)
if err != nil {
http.Error(w, "Error querying", http.StatusInternalServerError)
return
}
query, err := h.appManager.Query(r.Context(), 0, msg)
if err != nil {
log.Printf("Error querying: %v", err)
http.Error(w, "Error querying", http.StatusInternalServerError)
return
}

Comment on lines +93 to +96
err := jsonpb.Unmarshal(limitedReader, msg)
if err != nil {
return nil, fmt.Errorf("error parsing body: %w", err)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Use io.ReadAll for better error handling

Reading from io.LimitReader and then unmarshalling can lead to partial reads if the body exceeds the MaxBodySize. Instead, read the full body into a byte slice and check for io.EOF to ensure the entire body is read.

Apply this diff to improve request body handling:

 defer r.Body.Close()
-limitedReader := io.LimitReader(r.Body, MaxBodySize)
-err := jsonpb.Unmarshal(limitedReader, msg)
+body, err := io.ReadAll(io.LimitReader(r.Body, MaxBodySize))
+if err != nil {
+	return nil, fmt.Errorf("error reading body: %w", err)
+}
+err = jsonpb.Unmarshal(bytes.NewReader(body), msg)
 if err != nil {
 	return nil, fmt.Errorf("error parsing body: %w", err)
 }

Don't forget to import the bytes package:

 import (
 	"encoding/json"
 	"fmt"
 	"io"
+	"bytes"
 	"net/http"
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
err := jsonpb.Unmarshal(limitedReader, msg)
if err != nil {
return nil, fmt.Errorf("error parsing body: %w", err)
}
body, err := io.ReadAll(io.LimitReader(r.Body, MaxBodySize))
if err != nil {
return nil, fmt.Errorf("error reading body: %w", err)
}
err = jsonpb.Unmarshal(bytes.NewReader(body), msg)
if err != nil {
return nil, fmt.Errorf("error parsing body: %w", err)
}

Comment on lines +62 to +64
if r.Method != http.MethodPost {
return fmt.Errorf("method not allowed")
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Improve error message for unsupported methods

In the validateMethodIsPOST function, the error message "method not allowed" lacks specificity. To enhance clarity and adhere to Go's error handling conventions, consider returning the HTTP status text corresponding to the error.

Apply this diff to improve the error message:

-		return fmt.Errorf("method not allowed")
+		return fmt.Errorf(http.StatusText(http.StatusMethodNotAllowed))
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if r.Method != http.MethodPost {
return fmt.Errorf("method not allowed")
}
if r.Method != http.MethodPost {
return fmt.Errorf(http.StatusText(http.StatusMethodNotAllowed))
}

Comment on lines +80 to +84
path := strings.TrimPrefix(r.URL.Path, "/")
requestType := gogoproto.MessageType(path)
if requestType == nil {
return nil, fmt.Errorf("unknown request type")
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Validate and sanitize request path

Using r.URL.Path directly can lead to security issues if the path contains unexpected segments or escape characters. Ensure the path is sanitized and validated to prevent potential misuse.

Consider applying this diff to safely extract the message type:

 path := strings.TrimPrefix(r.URL.Path, "/")
+if path == "" {
+	return nil, fmt.Errorf("request path cannot be empty")
+}
+if strings.Contains(path, "/") {
+	return nil, fmt.Errorf("invalid request path")
+}
 requestType := gogoproto.MessageType(path)
 if requestType == nil {
 	return nil, fmt.Errorf("unknown request type")
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
path := strings.TrimPrefix(r.URL.Path, "/")
requestType := gogoproto.MessageType(path)
if requestType == nil {
return nil, fmt.Errorf("unknown request type")
}
path := strings.TrimPrefix(r.URL.Path, "/")
if path == "" {
return nil, fmt.Errorf("request path cannot be empty")
}
if strings.Contains(path, "/") {
return nil, fmt.Errorf("invalid request path")
}
requestType := gogoproto.MessageType(path)
if requestType == nil {
return nil, fmt.Errorf("unknown request type")
}

Comment on lines +70 to +75
contentType := r.Header.Get("Content-Type")
if contentType != ContentTypeJSON {
return fmt.Errorf("unsupported content type, expected %s", ContentTypeJSON)
}

return nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Handle Content-Type header with parameters

The current validation checks if the Content-Type header exactly matches application/json. However, Content-Type may include parameters like charset=utf-8, causing valid requests to fail. Use mime.ParseMediaType to robustly parse the media type.

Apply this diff to enhance content type validation:

-import "strings"
+import (
+	"mime"
+	"strings"
+)

 func (h *DefaultHandler[T]) validateContentTypeIsJSON(r *http.Request) error {
 	contentType := r.Header.Get("Content-Type")
-	if contentType != ContentTypeJSON {
+	mediaType, _, err := mime.ParseMediaType(contentType)
+	if err != nil || mediaType != ContentTypeJSON {
 		return fmt.Errorf("unsupported content type, expected %s", ContentTypeJSON)
 	}

Committable suggestion was skipped due to low confidence.

Merged via the queue into main with commit e666764 Oct 16, 2024
74 of 75 checks passed
@randygrok randygrok deleted the feat/wire-v2-handlers branch October 16, 2024 14:26
@coderabbitai coderabbitai bot mentioned this pull request Oct 25, 2024
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:server/v2 api C:server/v2 Issues related to server/v2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants