feat: improve startup and save/read failure logging#47
feat: improve startup and save/read failure logging#47anshuman9468 wants to merge 1 commit intodbpedia:masterfrom
Conversation
📝 WalkthroughWalkthroughThe changes augment two Scala files with comprehensive logging support via SLF4J and improve error handling. ApiImpl.scala introduces stricter parameter validation with a helper function and wraps operations with error recovery. GitClient.scala adds logging around Git operations and token retrieval. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip CodeRabbit can use OpenGrep to find security vulnerabilities and bugs across 17+ programming languages.OpenGrep is compatible with Semgrep configurations. Add an |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/main/scala/org/dbpedia/databus/GitClient.scala (1)
154-168: Consider sanitizing error message before logging.The error message
ecomes directly from the GitLab API response body, which could potentially contain sensitive information (tokens, credentials, or internal server details). Consider logging a generic failure message or sanitizing the response before logging.🛡️ Suggested approach
case Left(e) => - log.error(s"Failed to get access token from Gitlab: $e") + log.error("Failed to get access token from Gitlab") + log.debug(s"Token retrieval error details: $e") Failure(new RuntimeException(e))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/scala/org/dbpedia/databus/GitClient.scala` around lines 154 - 168, The accessToken block is logging the raw GitLab response body (variable e) which may contain sensitive data; update the error handling in the Try within accessToken (around authReq(rootUser, rootPass) and backend.send(req).body) to avoid logging the full response: log a generic error message (e.g., "Failed to get access token from GitLab") and if you need details log a sanitized/truncated version at debug level or attach a non-sensitive status, and change the Failure to include a non-sensitive message (e.g., new RuntimeException("Failed to retrieve access token")) while preserving original response only in a debug or internal variable after sanitization.src/main/scala/org/dbpedia/databus/ApiImpl.scala (1)
250-256: Consider moving logger to companion object scope.
LoggerFactory.getLogger(ApiImpl.getClass)is called each timegetRequiredParamis invoked, which creates a logger lookup on each call. While SLF4J typically caches loggers internally, it's more idiomatic to define the logger once at the object level.♻️ Suggested improvement
Add a logger at the
Configobject level:object Config { private val log = LoggerFactory.getLogger(ApiImpl.getClass) // ... }Then use it in
getRequiredParam:def getRequiredParam(name: String): String = { getParam(name).getOrElse({ val msg = s"Missing required configuration parameter: $name" - LoggerFactory.getLogger(ApiImpl.getClass).error(msg) + log.error(msg) throw new NoSuchElementException(msg) }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/scala/org/dbpedia/databus/ApiImpl.scala` around lines 250 - 256, Move the logger instantiation out of getRequiredParam to the companion/object scope so it is created once: add a private val (e.g. log) in the Config / ApiImpl companion object using LoggerFactory.getLogger(ApiImpl.getClass) and then replace the inline LoggerFactory.getLogger(...) call inside getRequiredParam with that private log variable; update any references accordingly so getRequiredParam uses the shared logger instance instead of creating a new one on each call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/main/scala/org/dbpedia/databus/ApiImpl.scala`:
- Around line 250-256: Move the logger instantiation out of getRequiredParam to
the companion/object scope so it is created once: add a private val (e.g. log)
in the Config / ApiImpl companion object using
LoggerFactory.getLogger(ApiImpl.getClass) and then replace the inline
LoggerFactory.getLogger(...) call inside getRequiredParam with that private log
variable; update any references accordingly so getRequiredParam uses the shared
logger instance instead of creating a new one on each call.
In `@src/main/scala/org/dbpedia/databus/GitClient.scala`:
- Around line 154-168: The accessToken block is logging the raw GitLab response
body (variable e) which may contain sensitive data; update the error handling in
the Try within accessToken (around authReq(rootUser, rootPass) and
backend.send(req).body) to avoid logging the full response: log a generic error
message (e.g., "Failed to get access token from GitLab") and if you need details
log a sanitized/truncated version at debug level or attach a non-sensitive
status, and change the Failure to include a non-sensitive message (e.g., new
RuntimeException("Failed to retrieve access token")) while preserving original
response only in a debug or internal variable after sanitization.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 942289a9-c1b0-4af8-9596-daba5b01e2a1
📒 Files selected for processing (2)
src/main/scala/org/dbpedia/databus/ApiImpl.scalasrc/main/scala/org/dbpedia/databus/GitClient.scala
Overview
This PR adds SLF4J logging to improve observability during startup and Git operations, making debugging faster and clearer.
Changes
LocalGitClientorRemoteGitlabHttpClientis used.getRequiredParamlogs and throws errors for missing required configs.Context
Fixes unclear failures caused by misconfiguration, invalid GitLab credentials, or Git operation issues.
Summary by CodeRabbit
Release Notes
Bug Fixes
Chores