carverauto/serviceradar

Fork 0

linted #2245

Merged

mfreeman451 merged 1 commit from refs/pull/2245/head into main

2025-09-28 21:48:11 +00:00

mfreeman451 commented

2025-09-28 21:47:42 +00:00

(Migrated from github.com)

Owner

Imported from GitHub pull request.

Original GitHub pull request: #1665
Original author: @mfreeman451
Original URL: https://github.com/carverauto/serviceradar/pull/1665
Original created: 2025-09-28T21:47:42Z
Original updated: 2025-09-28T21:49:11Z
Original head: carverauto/serviceradar:chore/linter_updates_jwks
Original base: main
Original merged: 2025-09-28T21:48:11Z by @mfreeman451

PR Type

Other

Description

Fix indentation from spaces to tabs throughout file
Add blank lines for better code readability
Remove trailing whitespace and unused code

Diagram Walkthrough

flowchart LR
  A["Spaces"] --> B["Tabs"]
  C["Missing blank lines"] --> D["Added spacing"]
  E["Trailing whitespace"] --> F["Clean code"]

File Walkthrough

Relevant files

Formatting

server.go

Code formatting and indentation fixes

pkg/core/api/server.go

Convert space indentation to tab indentation throughout file
Add blank lines between logical code blocks for readability
Remove trailing whitespace and clean up formatting
Remove unused SRQL-related code comments at end of file

+119/-91

Imported from GitHub pull request. Original GitHub pull request: #1665 Original author: @mfreeman451 Original URL: https://github.com/carverauto/serviceradar/pull/1665 Original created: 2025-09-28T21:47:42Z Original updated: 2025-09-28T21:49:11Z Original head: carverauto/serviceradar:chore/linter_updates_jwks Original base: main Original merged: 2025-09-28T21:48:11Z by @mfreeman451 --- ### **PR Type** Other ___ ### **Description** - Fix indentation from spaces to tabs throughout file - Add blank lines for better code readability - Remove trailing whitespace and unused code ___ ### Diagram Walkthrough ```mermaid flowchart LR A["Spaces"] --> B["Tabs"] C["Missing blank lines"] --> D["Added spacing"] E["Trailing whitespace"] --> F["Clean code"] ``` <details> <summary><h3> File Walkthrough</h3></summary> <table><thead><tr><th></th><th align="left">Relevant files</th></tr></thead><tbody><tr><td><strong>Formatting</strong></td><td><table> <tr> <td> <details> <summary><strong>server.go</strong><dd><code>Code formatting and indentation fixes</code>                                        </dd></summary> <hr> pkg/core/api/server.go <ul><li>Convert space indentation to tab indentation throughout file<br> <li> Add blank lines between logical code blocks for readability<br> <li> Remove trailing whitespace and clean up formatting<br> <li> Remove unused SRQL-related code comments at end of file</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/1665/files#diff-1bb99367fdd853c728b7cfbf5893a293f6d217144dfb5282cb8dd32e5261021e">+119/-91</a></td> </tr> </table></td></tr></tr></tbody></table> </details> ___

qodo-code-review[bot] commented

2025-09-28 21:48:11 +00:00

(Migrated from github.com)

Author

Owner

Imported GitHub PR comment.

Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/1665#issuecomment-3344291071
Original created: 2025-09-28T21:48:11Z

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review

Possible Issue

In NewAPIServer, variable s is now initialized (tabs vs spaces change surfaced it). Ensure s is declared before it's used for kvPutFn/kvGetFn; otherwise this would not compile. Verify that no merge conflict removed the declaration outside of the shown hunk.

func NewAPIServer(config models.CORSConfig, options ...func(server *APIServer)) *APIServer {
	s := &APIServer{
		pollers:     make(map[string]*PollerStatus),
		router:      mux.NewRouter(),
		corsConfig:  config,
		kvEndpoints: make(map[string]*KVEndpoint),
	}

	// Default kvPutFn dials KV and performs a Put
	s.kvPutFn = func(ctx context.Context, key string, value []byte, ttl int64) error {
		if s.kvAddress == "" {

Type Assertion Risk

handleJWKS type-asserts s.authService to *auth.Auth to access Config(). If a different implementation is used, JWKS returns an empty set. Confirm Config() exists and exported, and consider a safer interface-based method to obtain public keys to avoid brittle coupling.

func (s *APIServer) handleJWKS(w http.ResponseWriter, _ *http.Request) {
	// Auth service must be present and must expose config
	// Our concrete implementation stores config in auth.Auth; we access via exported helper
	// Build JWKS JSON using the server's configured auth settings
	if s.authService == nil {
		http.Error(w, "auth service not configured", http.StatusServiceUnavailable)
		return
	}

	// We don’t have direct access to config here; use a small adapter: the auth package exposes a function
	// that needs the config, which we can obtain by type assertion to *auth.Auth.
	// If not the concrete type, we return an empty set.
	var cfg *models.AuthConfig

	if a, ok := s.authService.(*auth.Auth); ok {
		// Access unexported field via method would be cleaner, but keep minimal by adding a tiny getter.
		// Fall back to empty set if not available
		cfg = a.Config()
	}

	if cfg == nil {
		w.Header().Set("Content-Type", "application/json")
		_, _ = w.Write([]byte(`{"keys":[]}`))

		return
	}

	data, err := auth.PublicJWKSJSON(cfg)
	if err != nil {
		s.logger.Error().Err(err).Msg("failed to build JWKS")

Content Construction

handleOIDCDiscovery builds JSON via string concatenation. This risks formatting/escaping issues and is harder to maintain. Prefer struct + json.Marshal to ensure valid JSON and correct escaping.

func (s *APIServer) handleOIDCDiscovery(w http.ResponseWriter, r *http.Request) {
	scheme := "http"
	if r.TLS != nil {
		scheme = "https"
	}

	host := r.Host
	issuer := scheme + "://" + host
	jwks := issuer + "/auth/jwks.json"

	w.Header().Set("Content-Type", "application/json")
	_, _ = w.Write([]byte("{\n" +
		"  \"issuer\": \"" + issuer + "\",\n" +
		"  \"jwks_uri\": \"" + jwks + "\",\n" +
		"  \"id_token_signing_alg_values_supported\": [\"RS256\"],\n" +
		"  \"response_types_supported\": [\"code\", \"token\", \"id_token\"],\n" +
		"  \"subject_types_supported\": [\"public\"],\n" +
		"  \"claims_supported\": [\"sub\", \"email\", \"roles\", \"exp\", \"iat\"]\n" +
		"}"))

Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/1665#issuecomment-3344291071 Original created: 2025-09-28T21:48:11Z --- ## PR Reviewer Guide 🔍 Here are some key observations to aid the review process: <table> <tr><td>⏱️ <strong>Estimated effort to review</strong>: 2 🔵🔵⚪⚪⚪</td></tr> <tr><td>🧪 <strong>No relevant tests</strong></td></tr> <tr><td>🔒 <strong>No security concerns identified</strong></td></tr> <tr><td>⚡ <strong>Recommended focus areas for review</strong><br><br> <details><summary><a href='https://github.com/carverauto/serviceradar/pull/1665/files#diff-1bb99367fdd853c728b7cfbf5893a293f6d217144dfb5282cb8dd32e5261021eR62-R72'><strong>Possible Issue</strong></a> In NewAPIServer, variable s is now initialized (tabs vs spaces change surfaced it). Ensure s is declared before it's used for kvPutFn/kvGetFn; otherwise this would not compile. Verify that no merge conflict removed the declaration outside of the shown hunk. </summary> ```go func NewAPIServer(config models.CORSConfig, options ...func(server *APIServer)) *APIServer { s := &APIServer{ pollers: make(map[string]*PollerStatus), router: mux.NewRouter(), corsConfig: config, kvEndpoints: make(map[string]*KVEndpoint), } // Default kvPutFn dials KV and performs a Put s.kvPutFn = func(ctx context.Context, key string, value []byte, ttl int64) error { if s.kvAddress == "" { ``` </details> <details><summary><a href='https://github.com/carverauto/serviceradar/pull/1665/files#diff-1bb99367fdd853c728b7cfbf5893a293f6d217144dfb5282cb8dd32e5261021eR623-R652'><strong>Type Assertion Risk</strong></a> handleJWKS type-asserts s.authService to *auth.Auth to access Config(). If a different implementation is used, JWKS returns an empty set. Confirm Config() exists and exported, and consider a safer interface-based method to obtain public keys to avoid brittle coupling. </summary> ```go func (s *APIServer) handleJWKS(w http.ResponseWriter, _ *http.Request) { // Auth service must be present and must expose config // Our concrete implementation stores config in auth.Auth; we access via exported helper // Build JWKS JSON using the server's configured auth settings if s.authService == nil { http.Error(w, "auth service not configured", http.StatusServiceUnavailable) return } // We don’t have direct access to config here; use a small adapter: the auth package exposes a function // that needs the config, which we can obtain by type assertion to *auth.Auth. // If not the concrete type, we return an empty set. var cfg *models.AuthConfig if a, ok := s.authService.(*auth.Auth); ok { // Access unexported field via method would be cleaner, but keep minimal by adding a tiny getter. // Fall back to empty set if not available cfg = a.Config() } if cfg == nil { w.Header().Set("Content-Type", "application/json") _, _ = w.Write([]byte(`{"keys":[]}`)) return } data, err := auth.PublicJWKSJSON(cfg) if err != nil { s.logger.Error().Err(err).Msg("failed to build JWKS") ``` </details> <details><summary><a href='https://github.com/carverauto/serviceradar/pull/1665/files#diff-1bb99367fdd853c728b7cfbf5893a293f6d217144dfb5282cb8dd32e5261021eR663-R681'><strong>Content Construction</strong></a> handleOIDCDiscovery builds JSON via string concatenation. This risks formatting/escaping issues and is harder to maintain. Prefer struct + json.Marshal to ensure valid JSON and correct escaping. </summary> ```go func (s *APIServer) handleOIDCDiscovery(w http.ResponseWriter, r *http.Request) { scheme := "http" if r.TLS != nil { scheme = "https" } host := r.Host issuer := scheme + "://" + host jwks := issuer + "/auth/jwks.json" w.Header().Set("Content-Type", "application/json") _, _ = w.Write([]byte("{\n" + " \"issuer\": \"" + issuer + "\",\n" + " \"jwks_uri\": \"" + jwks + "\",\n" + " \"id_token_signing_alg_values_supported\": [\"RS256\"],\n" + " \"response_types_supported\": [\"code\", \"token\", \"id_token\"],\n" + " \"subject_types_supported\": [\"public\"],\n" + " \"claims_supported\": [\"sub\", \"email\", \"roles\", \"exp\", \"iat\"]\n" + "}")) ``` </details> </td></tr> </table>

qodo-code-review[bot] commented

2025-09-28 21:49:11 +00:00

(Migrated from github.com)

Author

Owner

Imported GitHub PR comment.

Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/1665#issuecomment-3344291787
Original created: 2025-09-28T21:49:11Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
High-level	Automate formatting instead of manual changes Instead of submitting large manual formatting changes, the project should adopt an automated formatting tool like `gofmt`. This should be enforced via CI or pre-commit hooks to maintain consistency without creating noisy commits. Examples: pkg/core/api/server.go [1-2034] `/* * Copyright 2025 Carver Automation Corporation. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. * You may obtain a copy of the License at * * http://www.apache.org/licenses/LICENSE-2.0 * * Unless required by applicable law or agreed to in writing, software ... (clipped 2024 lines)` Solution Walkthrough: Before: `// Developer manually reformats a file // 1. Changes all space indentations to tabs. // 2. Adds blank lines for readability. // 3. Removes trailing whitespace. // A pull request is created containing thousands of lines // of purely stylistic changes, making code review difficult // and polluting the git history. // Example from pkg/core/api/server.go - s := &APIServer{ - pollers: make(map[string]PollerStatus), - router: mux.NewRouter(), - }` After: // 1. A formatting tool is configured for the project (e.g., in a pre-commit hook or CI pipeline). / .github/workflows/lint.yml */ - name: Run gofmt run: if [ "$(gofmt -s -l . \| wc -l)" -gt 0 ]; then exit 1; fi // 2. Developers make logical code changes. // 3. The tool automatically formats the code upon commit or fails the CI check if not formatted. // Result: // - Formatting is always consistent. // - PRs contain only meaningful, logical changes. // - No large, noisy formatting commits are ever needed. Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies that a large, manual formatting commit is an anti-pattern and proposes the standard, automated solution (`gofmt` with CI/hooks), which would significantly improve repository health and developer workflow.	Medium
General	Use struct and JSON marshaling Refactor `handleOIDCDiscovery` to generate the JSON response using a struct and `encoding/json` instead of string concatenation for improved safety and readability. pkg/core/api/server.go [663-682] func (s APIServer) handleOIDCDiscovery(w http.ResponseWriter, r http.Request) { scheme := "http" if r.TLS != nil { scheme = "https" } host := r.Host issuer := scheme + "://" + host - jwks := issuer + "/auth/jwks.json" + jwksURL := issuer + "/auth/jwks.json" + + discoveryDoc := struct { + Issuer string `json:"issuer"` + JwksURI string `json:"jwks_uri"` + IDTokenSigningAlgs []string `json:"id_token_signing_alg_values_supported"` + ResponseTypesSupported []string `json:"response_types_supported"` + SubjectTypesSupported []string `json:"subject_types_supported"` + ClaimsSupported []string `json:"claims_supported"` + }{ + Issuer: issuer, + JwksURI: jwksURL, + IDTokenSigningAlgs: []string{"RS256"}, + ResponseTypesSupported: []string{"code", "token", "id_token"}, + SubjectTypesSupported: []string{"public"}, + ClaimsSupported: []string{"sub", "email", "roles", "exp", "iat"}, + } w.Header().Set("Content-Type", "application/json") - _, _ = w.Write([]byte("{\n" + - " \"issuer\": \"" + issuer + "\",\n" + - " \"jwks_uri\": \"" + jwks + "\",\n" + - " \"id_token_signing_alg_values_supported\": [\"RS256\"],\n" + - " \"response_types_supported\": [\"code\", \"token\", \"id_token\"],\n" + - " \"subject_types_supported\": [\"public\"],\n" + - " \"claims_supported\": [\"sub\", \"email\", \"roles\", \"exp\", \"iat\"]\n" + - "}")) + if err := json.NewEncoder(w).Encode(discoveryDoc); err != nil { + s.logger.Error().Err(err).Msg("failed to encode OIDC discovery document") + http.Error(w, "failed to generate OIDC discovery document", http.StatusInternalServerError) + } } Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that building JSON via string concatenation is poor practice and replaces it with idiomatic Go using a struct and `json.NewEncoder`, which improves code quality, readability, and robustness.	Medium
More

Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/1665#issuecomment-3344291787 Original created: 2025-09-28T21:49:11Z --- ## PR Code Suggestions ✨  Explore these optional code suggestions: <table><thead><tr><td><strong>Category</strong></td><td align=left><strong>Suggestion                                                                                                                                    </strong></td><td align=center><strong>Impact</strong></td></tr><tbody><tr><td rowspan=1>High-level</td> <td> <details><summary>Automate formatting instead of manual changes</summary> ___ **Instead of submitting large manual formatting changes, the project should adopt <br>an automated formatting tool like <code>gofmt</code>. This should be enforced via CI or <br>pre-commit hooks to maintain consistency without creating noisy commits.** ### Examples: <details> <summary> <a href="https://github.com/carverauto/serviceradar/pull/1665/files#diff-1bb99367fdd853c728b7cfbf5893a293f6d217144dfb5282cb8dd32e5261021eR1-R2034">pkg/core/api/server.go [1-2034]</a> </summary> ```go /* * Copyright 2025 Carver Automation Corporation. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. * You may obtain a copy of the License at * * http://www.apache.org/licenses/LICENSE-2.0 * * Unless required by applicable law or agreed to in writing, software ... (clipped 2024 lines) ``` </details> ### Solution Walkthrough: #### Before: ```go // Developer manually reformats a file // 1. Changes all space indentations to tabs. // 2. Adds blank lines for readability. // 3. Removes trailing whitespace. // A pull request is created containing thousands of lines // of purely stylistic changes, making code review difficult // and polluting the git history. // Example from pkg/core/api/server.go - s := &APIServer{ - pollers: make(map[string]*PollerStatus), - router: mux.NewRouter(), - } ``` #### After: ```go // 1. A formatting tool is configured for the project (e.g., in a pre-commit hook or CI pipeline). /* .github/workflows/lint.yml */ - name: Run gofmt run: if [ "$(gofmt -s -l . | wc -l)" -gt 0 ]; then exit 1; fi // 2. Developers make logical code changes. // 3. The tool automatically formats the code upon commit or fails the CI check if not formatted. // Result: // - Formatting is always consistent. // - PRs contain only meaningful, logical changes. // - No large, noisy formatting commits are ever needed. ``` <details><summary>Suggestion importance[1-10]: 8</summary> __ Why: The suggestion correctly identifies that a large, manual formatting commit is an anti-pattern and proposes the standard, automated solution (`gofmt` with CI/hooks), which would significantly improve repository health and developer workflow. </details></details></td><td align=center>Medium </td></tr><tr><td rowspan=1>General</td> <td> <details><summary>Use struct and JSON marshaling</summary> ___ **Refactor <code>handleOIDCDiscovery</code> to generate the JSON response using a struct and <br><code>encoding/json</code> instead of string concatenation for improved safety and <br>readability.** [pkg/core/api/server.go [663-682]](https://github.com/carverauto/serviceradar/pull/1665/files#diff-1bb99367fdd853c728b7cfbf5893a293f6d217144dfb5282cb8dd32e5261021eR663-R682) ```diff func (s *APIServer) handleOIDCDiscovery(w http.ResponseWriter, r *http.Request) { scheme := "http" if r.TLS != nil { scheme = "https" } host := r.Host issuer := scheme + "://" + host - jwks := issuer + "/auth/jwks.json" + jwksURL := issuer + "/auth/jwks.json" + + discoveryDoc := struct { + Issuer string `json:"issuer"` + JwksURI string `json:"jwks_uri"` + IDTokenSigningAlgs []string `json:"id_token_signing_alg_values_supported"` + ResponseTypesSupported []string `json:"response_types_supported"` + SubjectTypesSupported []string `json:"subject_types_supported"` + ClaimsSupported []string `json:"claims_supported"` + }{ + Issuer: issuer, + JwksURI: jwksURL, + IDTokenSigningAlgs: []string{"RS256"}, + ResponseTypesSupported: []string{"code", "token", "id_token"}, + SubjectTypesSupported: []string{"public"}, + ClaimsSupported: []string{"sub", "email", "roles", "exp", "iat"}, + } w.Header().Set("Content-Type", "application/json") - _, _ = w.Write([]byte("{\n" + - " \"issuer\": \"" + issuer + "\",\n" + - " \"jwks_uri\": \"" + jwks + "\",\n" + - " \"id_token_signing_alg_values_supported\": [\"RS256\"],\n" + - " \"response_types_supported\": [\"code\", \"token\", \"id_token\"],\n" + - " \"subject_types_supported\": [\"public\"],\n" + - " \"claims_supported\": [\"sub\", \"email\", \"roles\", \"exp\", \"iat\"]\n" + - "}")) + if err := json.NewEncoder(w).Encode(discoveryDoc); err != nil { + s.logger.Error().Err(err).Msg("failed to encode OIDC discovery document") + http.Error(w, "failed to generate OIDC discovery document", http.StatusInternalServerError) + } } ``` - [ ] **Apply / Chat**  <details><summary>Suggestion importance[1-10]: 7</summary> __ Why: The suggestion correctly identifies that building JSON via string concatenation is poor practice and replaces it with idiomatic Go using a struct and `json.NewEncoder`, which improves code quality, readability, and robustness. </details></details></td><td align=center>Medium </td></tr> <tr><td align="center" colspan="2"> - [ ] More  </td><td></td></tr></tbody></table>