Open
Conversation
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- There appear to be two separate generated protobuf trees for auth (api/proto/pb and api/proto/proto/pb) with differing message definitions (e.g., ActivateRequest.Key vs ActivateRequest.LicenseKey); consider consolidating to a single canonical proto location and regenerating code to avoid package collisions and field name drift.
- The license key format is expressed in multiple places (HTML pattern attribute, frontend isLicenseKey regex, and backend licenseKeyPattern); it would be good to ensure they stay strictly aligned (e.g., via a shared constant or clearly documented single source) to avoid subtle validation mismatches between client and server.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- There appear to be two separate generated protobuf trees for auth (api/proto/pb and api/proto/proto/pb) with differing message definitions (e.g., ActivateRequest.Key vs ActivateRequest.LicenseKey); consider consolidating to a single canonical proto location and regenerating code to avoid package collisions and field name drift.
- The license key format is expressed in multiple places (HTML pattern attribute, frontend isLicenseKey regex, and backend licenseKeyPattern); it would be good to ensure they stay strictly aligned (e.g., via a shared constant or clearly documented single source) to avoid subtle validation mismatches between client and server.
## Individual Comments
### Comment 1
<location path="assets/js/components/Config/SponsorModal.vue" line_range="87-96" />
<code_context>
+ </div>
+ <div v-else-if="activeTab === 'direct'">
+ <label for="sponsorEmail" class="my-2">{{ $t("config.sponsor.email") }}</label>
+ <input
+ id="sponsorEmail"
+ v-model.trim="values.email"
+ type="email"
+ class="form-control mb-1"
+ required
+ />
+ <i18n-t
+ tag="small"
+ keypath="config.sponsor.emailHint"
+ scope="global"
+ class="mb-3 d-block text-muted"
+ >
+ <template #url>
+ <a href="https://sponsor.evcc.io/" target="_blank">direct sponsoring</a>
+ </template>
+ </i18n-t>
+ <label for="licenseKey" class="my-2">{{
+ $t("config.sponsor.activationKey")
}}</label>
+ <input
+ id="licenseKey"
+ v-model.trim="values.token"
+ class="form-control mb-1 font-monospace"
+ required
+ spellcheck="false"
+ pattern="[A-Z0-9]{5}-[A-Z0-9]{5}-[A-Z0-9]{5}-[A-Z0-9]{5}-[A-Z0-9]{5}"
+ title="XXXXX-XXXXX-XXXXX-XXXXX-XXXXX"
+ />
</code_context>
<issue_to_address>
**issue (bug_risk):** HTML pattern is case-sensitive and may reject lowercase keys that the backend accepts
The client pattern restricts input to `[A-Z0-9]`, but the backend first uppercases the value before matching, so lowercase keys are valid server-side but rejected in the browser. To align behavior, either allow letters in both cases in the pattern (e.g. `[A-Za-z0-9]`) and normalize to uppercase in JS, or force uppercase input via auto-transform / `text-transform: uppercase`.
</issue_to_address>
### Comment 2
<location path="api/proto/proto/pb/auth.pb.go" line_range="7" />
<code_context>
+// protoc v6.33.2
+// source: auth.proto
+
+package pb
+
+import (
</code_context>
<issue_to_address>
**issue (complexity):** Consider removing this duplicated auth proto/service definition and instead reuse the existing canonical auth proto with small adapter types where needed.
The added file is a full, independent copy of an auth proto with its own types, descriptors, and service. That does add real complexity (two near-identical `Auth*` APIs, two sources of truth, sync risk). You can keep the new functionality while avoiding the parallel proto universe by:
1. **Reusing the existing generated package** and:
2. **Adding small, focused adapters** where you need different field naming or import paths.
### 1. Import the canonical auth proto instead of duplicating it
Instead of committing a second generated `auth.pb.go`, depend on the existing one and alias the package:
```go
// instead of: package pb
package yourpkg
import (
authpb "your.org/yourrepo/api/proto/pb" // existing canonical proto package
)
```
Then use `authpb.AuthRequest`, `authpb.AuthReply`, etc., everywhere inside this module.
If you need this module to expose the same service under a different import path, keep the Go types from `authpb`, and expose them via your own package, rather than re-generating them locally.
### 2. Use thin adapter types instead of a second proto
If your new API needs slightly different field names (e.g. `LicenseKey` vs `Key`) or JSON shapes, add small wrapper/adapter structs in this module and map to the canonical proto types, instead of redefining the proto schema:
```go
// Local API surface (e.g. HTTP JSON or module boundary)
type ActivateRequestDTO struct {
Key string `json:"key"`
Email string `json:"email"`
MachineID string `json:"machine_id"`
}
type ActivateReplyDTO struct {
Token string `json:"token"`
Error string `json:"error"`
}
// Mapping to/from canonical proto messages
func (dto *ActivateRequestDTO) ToProto() *authpb.ActivateRequest {
return &authpb.ActivateRequest{
LicenseKey: dto.Key,
Email: dto.Email,
MachineId: dto.MachineID,
}
}
func ActivateReplyFromProto(p *authpb.ActivateReply) *ActivateReplyDTO {
return &ActivateReplyDTO{
Token: p.GetToken(),
Error: p.GetError(),
}
}
```
Your gRPC service implementation can now be expressed purely in terms of `authpb`:
```go
type AuthServer struct {
authpb.UnimplementedAuthServer
}
func (s *AuthServer) Activate(ctx context.Context, req *authpb.ActivateRequest) (*authpb.ActivateReply, error) {
// business logic using canonical proto
}
```
The external surface (e.g. HTTP handler, CLI, or another module) uses the DTOs and these mapping helpers, but there is only **one** proto definition and **one** generated `Auth` service to maintain.
### 3. If you must generate code in this module
If generation in this module is required for build reasons, consider:
- Storing a **single `auth.proto`** in a shared location.
- Making this module’s `auth.pb.go` generated from that same file with the **same** `go_package` option, so the package name and types are identical, not parallel:
```proto
option go_package = "your.org/yourrepo/api/proto/pb;pb";
```
Then don’t check in a “forked” `auth.proto` with diverging field names; keep only one schema and generate wherever needed.
---
Net effect: you preserve all behavior, but eliminate the duplicate service/types and make the canonical `auth.proto` the only source of truth, with small, localized adapters where differences are required.
</issue_to_address>
### Comment 3
<location path="api/proto/proto/pb/auth_grpc.pb.go" line_range="1" />
<code_context>
+// Code generated by protoc-gen-go-grpc. DO NOT EDIT.
+// versions:
+// - protoc-gen-go-grpc v1.6.0
</code_context>
<issue_to_address>
**issue (complexity):** Consider avoiding a second generated Auth gRPC service here by reusing the existing canonical service via type aliases and thin wrappers instead of duplicating all client/server definitions.
You’re effectively introducing a second, independent `Auth` gRPC service surface that duplicates `api/proto/pb/auth_grpc.pb.go` (different descriptors, same shape). That does increase complexity: two `AuthClient`/`AuthServer` types, two `RegisterAuthServer` funcs, two `Auth_IsAuthorized` methods, etc.
If the goal is just to make the existing service accessible from this package, you can avoid generating (and maintaining) a second service definition and instead:
1. Treat `api/proto/pb/auth_grpc.pb.go` as the canonical generated code.
2. Use type aliases and thin wrappers in this package to keep the local API shape, while delegating to the canonical types.
Example of a small, hand-written wrapper instead of this whole generated file:
```go
// In this package, instead of generating a new service:
import api_pb "your/module/path/api/proto/pb"
// Re-export client/server types so call sites don't have to change much.
type AuthClient = api_pb.AuthClient
type AuthServer = api_pb.AuthServer
// Optional: re-expose constructor/registration to keep local call sites simple.
func NewAuthClient(cc grpc.ClientConnInterface) AuthClient {
return api_pb.NewAuthClient(cc)
}
func RegisterAuthServer(s grpc.ServiceRegistrar, srv AuthServer) {
api_pb.RegisterAuthServer(s, srv)
}
// Re-export message types if needed:
type AuthRequest = api_pb.AuthRequest
type AuthReply = api_pb.AuthReply
type ActivateRequest = api_pb.ActivateRequest
type ActivateReply = api_pb.ActivateReply
```
If the different proto path/package is the reason for the second generation, consider instead:
- Adjusting the proto’s `go_package` option so both consumers can import the same generated package, or
- Updating your build/protoc invocation so only one `auth_grpc.pb.go` is generated and reused.
This keeps all current behavior but removes the need to maintain two divergent `Auth` service definitions over time.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Member
Author
|
@sourcery-ai dismiss |
Member
Author
|
@sourcery-ai dismiss |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.