-
Notifications
You must be signed in to change notification settings - Fork 4
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
chore(server): internal api v1 specification #1392
base: main
Are you sure you want to change the base?
Conversation
WalkthroughA new Protocol Buffers schema file has been added to define the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ReEarthCMS
Client->>ReEarthCMS: CreateProject(request)
ReEarthCMS-->>Client: CreateProjectResponse
Client->>ReEarthCMS: ListProjects(request)
ReEarthCMS-->>Client: ListProjectsResponse
Client->>ReEarthCMS: ListModels(request)
ReEarthCMS-->>Client: ListModelsResponse
Client->>ReEarthCMS: GetModelGeoJSONExportURL(request)
ReEarthCMS-->>Client: ExportURLResponse
Poem
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? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Deploy Preview for reearth-cms canceled.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (14)
server/docs/api.md (14)
4-25
: Table of Contents Indentation Issues:
The unordered list items in the Table of Contents (e.g., lines 7–18) are indented with 4 spaces. According to MD007, a 2-space indent is expected for nested lists. Adjusting these indentations will improve readability and compliance with markdown style guidelines.Consider the following diff snippet:
- - [AuthRequest](#reearth-cms-v1-AuthRequest) - - [AuthResponse](#reearth-cms-v1-AuthResponse) - - [ExportRequest](#reearth-cms-v1-ExportRequest) - - [ExportURLResponse](#reearth-cms-v1-ExportURLResponse) - - [ListModelsRequest](#reearth-cms-v1-ListModelsRequest) - - [ListModelsResponse](#reearth-cms-v1-ListModelsResponse) - - [ListProjectsRequest](#reearth-cms-v1-ListProjectsRequest) - - [ListProjectsResponse](#reearth-cms-v1-ListProjectsResponse) - - [Model](#reearth-cms-v1-Model) - - [Project](#reearth-cms-v1-Project) - - [ProjectPublication](#reearth-cms-v1-ProjectPublication) - - [RefreshTokenRequest](#reearth-cms-v1-RefreshTokenRequest) - - [ProjectPublicationScope](#reearth-cms-v1-ProjectPublicationScope) - - [ReEarthCMS](#reearth-cms-v1-ReEarthCMS) + - [AuthRequest](#reearth-cms-v1-AuthRequest) + - [AuthResponse](#reearth-cms-v1-AuthResponse) + - [ExportRequest](#reearth-cms-v1-ExportRequest) + - [ExportURLResponse](#reearth-cms-v1-ExportURLResponse) + - [ListModelsRequest](#reearth-cms-v1-ListModelsRequest) + - [ListModelsResponse](#reearth-cms-v1-ListModelsResponse) + - [ListProjectsRequest](#reearth-cms-v1-ListProjectsRequest) + - [ListProjectsResponse](#reearth-cms-v1-ListProjectsResponse) + - [Model](#reearth-cms-v1-Model) + - [Project](#reearth-cms-v1-Project) + - [ProjectPublication](#reearth-cms-v1-ProjectPublication) + - [RefreshTokenRequest](#reearth-cms-v1-RefreshTokenRequest) + - [ProjectPublicationScope](#reearth-cms-v1-ProjectPublicationScope) + - [ReEarthCMS](#reearth-cms-v1-ReEarthCMS)🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
7-7: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
8-8: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
9-9: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
10-10: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
11-11: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
12-12: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
13-13: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
14-14: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
15-15: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
16-16: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
17-17: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
18-18: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
20-20: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
22-22: Unordered list indentation
Expected: 2; Actual: 4(MD007, ul-indent)
35-44
: AuthRequest Documentation:
The AuthRequest section is clear and correctly formatted. To further assist developers, consider adding field descriptions in the table’s “Description” column.
51-61
: AuthResponse Documentation:
This section effectively documents the AuthResponse message. Enhancing the “Description” column with usage details could further improve developer understanding.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
60-60: Link fragments should be valid
null(MD051, link-fragments)
68-78
: ExportRequest Documentation:
The ExportRequest table lists the fields accurately. Providing additional descriptions for each field may be beneficial.
85-94
: ExportURLResponse Documentation:
Documentation of the ExportURLResponse message is complete; however, consider including field descriptions to clarify the purpose of each entry.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
93-93: Link fragments should be valid
null(MD051, link-fragments)
101-111
: ListModelsRequest Documentation:
The ListModelsRequest section is neatly documented. Adding examples for pagination parameters might be a useful enhancement.
118-128
: ListModelsResponse Documentation:
The table for ListModelsResponse is informative. Expanding the descriptions ofnext_page_token
andtotal_count
could help clarify their intended use.
135-145
: ListProjectsRequest Documentation:
The ListProjectsRequest table clearly outlines the input parameters. More descriptive details in the “Description” column could improve clarity for API consumers.
152-162
: ListProjectsResponse Documentation:
This section is concise and clear. Consider elaborating on the usage of thenext_page_token
field to aid in understanding the pagination mechanism.
169-184
: Model Documentation:
The Model message documentation is well-structured. Further enriching the table with field descriptions or examples could provide additional clarity.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
182-182: Link fragments should be valid
null(MD051, link-fragments)
183-183: Link fragments should be valid
null(MD051, link-fragments)
191-205
: Project Documentation:
The Project section is correctly organized. Including brief descriptions for each field would enhance the documentation’s usefulness.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
203-203: Link fragments should be valid
null(MD051, link-fragments)
204-204: Link fragments should be valid
null(MD051, link-fragments)
211-222
: ProjectPublication Documentation:
The table for ProjectPublication is clear; however, adding context about thetoken
field could be beneficial.
229-237
: RefreshTokenRequest Documentation:
The RefreshTokenRequest section is concise. Consider expanding it with additional context regarding token refresh workflows when available.
264-275
: ReEarthCMS Service Documentation:
The service method table effectively maps method names to their request and response types. However, several methods have empty descriptions. Providing brief summaries for these RPCs would improve the documentation for API consumers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
server/proto/v1/internal_api.pb.go
is excluded by!**/*.pb.go
server/proto/v1/internal_api_grpc.pb.go
is excluded by!**/*.pb.go
📒 Files selected for processing (2)
server/docs/api.md
(1 hunks)server/proto/v1/internal_api.proto
(1 hunks)
🧰 Additional context used
🪛 Buf (1.47.2)
server/proto/v1/internal_api.proto
3-3: Files with package "reearth.cms.v1" must be within a directory "reearth/cms/v1" relative to root but were in directory "server/proto/v1".
(PACKAGE_DIRECTORY_MATCH)
🪛 markdownlint-cli2 (0.17.2)
server/docs/api.md
7-7: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
8-8: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
9-9: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
10-10: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
11-11: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
12-12: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
13-13: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
14-14: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
15-15: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
16-16: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
17-17: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
18-18: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
20-20: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
22-22: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
60-60: Link fragments should be valid
null
(MD051, link-fragments)
93-93: Link fragments should be valid
null
(MD051, link-fragments)
182-182: Link fragments should be valid
null
(MD051, link-fragments)
183-183: Link fragments should be valid
null
(MD051, link-fragments)
203-203: Link fragments should be valid
null
(MD051, link-fragments)
204-204: Link fragments should be valid
null
(MD051, link-fragments)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: ci-server / test
- GitHub Check: ci-server / lint
🔇 Additional comments (17)
server/proto/v1/internal_api.proto (15)
1-8
: Package and File Location Mismatch:
The package is declared asreearth.cms.v1
, but the file is located underserver/proto/v1
. According to Buf’s guidelines, files with this package should reside underreearth/cms/v1
relative to the repository root. Please consider moving the file or updating the package declaration.🧰 Tools
🪛 Buf (1.47.2)
3-3: Files with package "reearth.cms.v1" must be within a directory "reearth/cms/v1" relative to root but were in directory "server/proto/v1".
(PACKAGE_DIRECTORY_MATCH)
9-18
: Service Definition Clarity:
TheReEarthCMS
service is clearly defined with RPC methods for authentication and core operations. The empty method bodies{}
are acceptable for service definitions in proto files provided that the implementations exist elsewhere; just ensure that the intended implementations are in place.
20-24
: Authentication Notes:
Informative comments on authentication (e.g., use of gRPC interceptors and bearer tokens in metadata) help set expectations. No further action is required unless additional implementation details become necessary.
25-32
: AuthRequest and RefreshTokenRequest Definitions:
BothAuthRequest
andRefreshTokenRequest
messages are succinct and to the point, each containing one required field. Be sure that the API key and refresh token validations are enforced in your service logic.
34-38
: AuthResponse Structure:
TheAuthResponse
message correctly combines access and refresh tokens with an expiration timestamp usinggoogle.protobuf.Timestamp
. This clear structure supports secure token management.
40-48
: Project Message Definition:
TheProject
message is well-structured, capturing necessary project details, including IDs, names, descriptions, workspace links, publication settings, and timestamps. Verify if thepublication
field should be optional based on the business requirements.
51-55
: ProjectPublication Message – Optional Field Consideration:
TheProjectPublication
message includes anoptional string token = 3;
. Since proto3 now supports theoptional
keyword (from version 3.15 onward), ensure that your tooling and runtime environments are compatible with this feature.
57-62
: Enum Definition for ProjectPublicationScope:
The enumeration is clearly defined with sensible values. Just confirm that your application logic correctly handles the defaultSCOPE_UNSPECIFIED
value.
64-73
: Model Message Definition:
TheModel
message is designed with the appropriate fields for model representation, including timestamps for creation and updates. Its clear structure should suit the intended use cases.
75-80
: ListProjectsRequest Message:
TheListProjectsRequest
message includes the essential parameters for project listings (workspace ID, page size, and page token). Its simplicity aids in understanding.
82-86
: ListModelsRequest Message:
TheListModelsRequest
message is concise and clear, suitable for paginated model listings within a project.
88-92
: ExportRequest Message:
TheExportRequest
message includes project and model IDs along with an expiration interval (expires_in_seconds
). Ensure that there is proper input validation for the expiration parameter in your implementation.
94-99
: ListProjectsResponse Message:
The response message returns a repeated list of projects, the next page token, and a total count. This structure will facilitate effective pagination in client applications.
101-105
: ListModelsResponse Message:
Consistent with the projects response, this message provides the necessary fields for paginated model listings.
107-110
: ExportURLResponse Message:
TheExportURLResponse
message is straightforward, returning both the export URL and its expiration timestamp. This should integrate well with client-side expectations for time-limited resources.server/docs/api.md (2)
29-30
: Proto File Reference Section:
The anchored header forproto/v1/internal_api.proto
provides a clear reference point for the rest of the documentation.
246-256
: ProjectPublicationScope Documentation:
The enum documentation is clear and fulfils its purpose. Ensure that any future modifications to enum values are reflected in corresponding documentation updates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Nitpick comments (2)
server/schemas/internel/v1/schema.proto (2)
9-15
: Consider adding pagination and field mask support.The service methods look good, but consider the following improvements:
- Add pagination support for list operations using
page_size
andpage_token
- Consider adding field masks for future-proofing partial updates
Example implementation for pagination:
message ListProjectsRequest { string workspace_id = 1; + int32 page_size = 2; + string page_token = 3; } message ListProjectsResponse { repeated Project projects = 1; int32 total_count = 3; + string next_page_token = 4; }
95-97
: Consider adding metadata to ExportURLResponse.The ExportURLResponse could benefit from additional metadata such as URL expiration time and file format.
message ExportURLResponse { string url = 1; + google.protobuf.Timestamp expires_at = 2; + string format = 3; // e.g., "geojson" + int64 estimated_size_bytes = 4; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/schemas/internel/v1/schema.proto
(1 hunks)
🧰 Additional context used
🪛 Buf (1.47.2)
server/schemas/internel/v1/schema.proto
3-3: Files with package "reearth.cms.v1" must be within a directory "reearth/cms/v1" relative to root but were in directory "server/schemas/internel/v1".
(PACKAGE_DIRECTORY_MATCH)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: ci-server / test
- GitHub Check: ci-server / lint
🔇 Additional comments (1)
server/schemas/internel/v1/schema.proto (1)
17-22
: LGTM! Clear authentication implementation notes.The authentication approach using gRPC interceptors and metadata is well-documented and follows standard practices.
message Model { | ||
string id = 1; | ||
string project_id = 2; | ||
string name = 3; | ||
string description = 4; | ||
string key = 5; | ||
bool public = 6; | ||
google.protobuf.Timestamp created_at = 7; | ||
google.protobuf.Timestamp updated_at = 8; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Rename the "public" field to avoid reserved word conflicts.
The public
field in the Model message is a reserved word in many programming languages, which could cause issues during code generation.
message Model {
string id = 1;
string project_id = 2;
string name = 3;
string description = 4;
string key = 5;
- bool public = 6;
+ bool is_public = 6;
google.protobuf.Timestamp created_at = 7;
google.protobuf.Timestamp updated_at = 8;
}
📝 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.
message Model { | |
string id = 1; | |
string project_id = 2; | |
string name = 3; | |
string description = 4; | |
string key = 5; | |
bool public = 6; | |
google.protobuf.Timestamp created_at = 7; | |
google.protobuf.Timestamp updated_at = 8; | |
} | |
message Model { | |
string id = 1; | |
string project_id = 2; | |
string name = 3; | |
string description = 4; | |
string key = 5; | |
bool is_public = 6; | |
google.protobuf.Timestamp created_at = 7; | |
google.protobuf.Timestamp updated_at = 8; | |
} |
message ListProjectsResponse { | ||
repeated Project projects = 1; | ||
int32 total_count = 3; | ||
} | ||
|
||
message ListModelsResponse { | ||
repeated Model models = 1; | ||
int32 total_count = 3; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Fix field numbering in response messages.
The field numbers in ListProjectsResponse and ListModelsResponse are not sequential (skips number 2), which might indicate missing fields or a mistake.
message ListProjectsResponse {
repeated Project projects = 1;
- int32 total_count = 3;
+ int32 total_count = 2;
}
message ListModelsResponse {
repeated Model models = 1;
- int32 total_count = 3;
+ int32 total_count = 2;
}
📝 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.
message ListProjectsResponse { | |
repeated Project projects = 1; | |
int32 total_count = 3; | |
} | |
message ListModelsResponse { | |
repeated Model models = 1; | |
int32 total_count = 3; | |
} | |
message ListProjectsResponse { | |
repeated Project projects = 1; | |
int32 total_count = 2; | |
} | |
message ListModelsResponse { | |
repeated Model models = 1; | |
int32 total_count = 2; | |
} |
syntax = "proto3"; | ||
|
||
package reearth.cms.v1; | ||
|
||
import "google/protobuf/timestamp.proto"; | ||
|
||
option go_package = "proto/v1"; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix directory structure and typo.
The directory structure needs to be adjusted:
- The package "reearth.cms.v1" should be within a directory "reearth/cms/v1"
- There's a typo in the directory name "internel" (should be "internal")
Please move this file to the correct directory structure and fix the typo:
-server/schemas/internel/v1/schema.proto
+server/schemas/internal/reearth/cms/v1/schema.proto
📝 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.
syntax = "proto3"; | |
package reearth.cms.v1; | |
import "google/protobuf/timestamp.proto"; | |
option go_package = "proto/v1"; | |
// File moved to: server/schemas/internal/reearth/cms/v1/schema.proto | |
syntax = "proto3"; | |
package reearth.cms.v1; | |
import "google/protobuf/timestamp.proto"; | |
option go_package = "proto/v1"; |
🧰 Tools
🪛 Buf (1.47.2)
3-3: Files with package "reearth.cms.v1" must be within a directory "reearth/cms/v1" relative to root but were in directory "server/schemas/internel/v1".
(PACKAGE_DIRECTORY_MATCH)
message ProjectPublication { | ||
ProjectPublicationScope scope = 1; | ||
bool asset_public = 2; | ||
optional string token = 3; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we return the token? 🤔
Overview
What I've done
What I haven't done
How I tested
Screenshot
Which point I want you to review particularly
Memo
Summary by CodeRabbit