Logging 2 by jesperlarsson1910 · Pull Request #26 · ithsjava25/project-jpa-mypod-d

jesperlarsson1910 · 2026-01-14T21:43:09Z

Summary by CodeRabbit

New Features
- Added persistent, database-backed application logging and broader runtime logging across components.
Bug Fixes
- More informative logging and improved error handling and input validation across API, persistence, and UI paths.
Chores
- Build configuration and logging setup updated; general formatting and cleanup applied.
Tests
- Test setup extended to initialize additional repository components for broader coverage.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

Signed-off-by: Jesper Larsson <jesper.larsson@iths.se>

# Conflicts: # src/main/java/org/example/ItunesPlayList.java

coderabbitai · 2026-01-14T21:43:20Z

Warning

Rate limit exceeded

@jesperlarsson1910 has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 10 minutes and 1 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between c72d3ac and 4c4edc0.

📒 Files selected for processing (4)

pom.xml
src/main/java/org/example/entity/LogEntry.java
src/main/java/org/example/logging/LoggingConnection.java
src/main/resources/log4j2.xml

📝 Walkthrough

Walkthrough

Adds Log4j2 and commons-dbcp2 to the build, configures a Log4j2 Console + JDBC appender, introduces SLF4J loggers across services and repositories, adds a LoggingConnectionFactory and LogEntry JPA entity, and includes minor interface, formatting, and test adjustments.

Changes

Cohort / File(s)	Summary
Build & Logging config `pom.xml`, `src/main/resources/log4j2.xml`	Adds Log4j dependencies and commons-dbcp2 to pom; adds Log4j2 config with Console and JDBC (app_logs) appenders.
DB Connection helper `src/main/java/org/example/logging/LoggingConnectionFactory.java`	New public class exposing `public static Connection getConnection()` with hardcoded URL/credentials used by JDBC appender.
Persistence entity for logs `src/main/java/org/example/entity/LogEntry.java`	New JPA entity `LogEntry` mapped to `app_logs` with id, level, message, errorDetails, timestamp and getters.
Services / UI with logging `src/main/java/org/example/DatabaseInitializer.java`, `src/main/java/org/example/ItunesApiClient.java`, `src/main/java/org/example/MyPod.java`, `src/main/java/org/example/ItunesPlayList.java`	Add SLF4J loggers and logging calls in error/info/debug branches; minor GUI field finalization and `onUpdateCallback` addition.
Repository logging & validation `src/main/java/org/example/repo/PlaylistRepositoryImpl.java`, `src/main/java/org/example/repo/SongRepositoryImpl.java`	Add SLF4J loggers, log on null/invalid inputs, standardize messages and log before throwing or returning empty lists.
Entity & interface cleanup `src/main/java/org/example/entity/DBObject.java`, `src/main/java/org/example/entity/Playlist.java`, `src/main/java/org/example/entity/Song.java`, `src/main/java/org/example/entity/Album.java`	Remove explicit `public` modifiers from `DBObject` methods, make `Playlist.songs` final, and apply minor whitespace/formatting edits.
Misc config & tests `src/main/java/org/example/PersistenceManager.java`, `src/main/resources/ipod_style.css`, `src/test/java/org/example/RepoTest.java`	Remove unused imports/extra brace in PersistenceManager, reformat CSS (no behavior change), initialize `playlistRepo` in test setup and minor formatting tweaks.

Sequence Diagram

sequenceDiagram
    participant App as Application (services/repos)
    participant SLF4J as SLF4J API
    participant L4J as Log4j2 Core
    participant JDBC as Log4j2 JDBC Appender
    participant Conn as LoggingConnectionFactory
    participant DB as Database (app_logs)
    participant Console as Console Appender

    App->>SLF4J: logger.error/info/debug(event)
    SLF4J->>L4J: forward LogEvent
    L4J->>Console: format & write to stdout
    L4J->>JDBC: prepare DB insert
    JDBC->>Conn: getConnection()
    Conn->>DB: open JDBC connection
    JDBC->>DB: INSERT INTO app_logs(...)
    DB-->>JDBC: ack
    L4J-->>SLF4J: event handled

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~30 minutes

Possibly related PRs

Feature/unittests #17 — Overlaps modifications to DatabaseInitializer and PlaylistRepositoryImpl.
Implements methods from SongRepository #7 — Related edits to SongRepositoryImpl (query handling and logging).
Menuitembindings #19 — Related changes to the DBObject interface and consuming entities.

Poem

🐰 I hopped through code with ears held high,

I stitched soft logs that now explain why,
From SLF4J whispers to JDBC’s song,
App_logs collect each line all day long,
A carrot-cheer — small logs, proud and strong!

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 15.69% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check	❓ Inconclusive	The title 'Logging 2' is vague and does not clearly convey the specific nature or scope of the changes made in this pull request.	Use a more descriptive title that summarizes the main objective, such as 'Add SLF4J logging integration with Log4j2 JDBC appender' or 'Implement comprehensive logging with database persistence'.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 7

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/main/java/org/example/PersistenceManager.java (1)
10-12: Hardcoded credentials pose a security risk.

While these appear to be local development credentials, hardcoding database connection details (URL, username, password) is a security concern and makes configuration inflexible across environments. Consider externalizing these to environment variables or a configuration file.
String url = System.getenv("DB_URL");
String user = System.getenv("DB_USER");
String pass = System.getenv("DB_PASS");

🤖 Fix all issues with AI agents

In `@src/main/java/org/example/logging/LoggingConnectionFactory.java`:
- Around line 9-11: The constants URL, USER, and PASSWORD in
LoggingConnectionFactory are hardcoded credentials; change them to be read from
external configuration (e.g., environment variables or system properties)
instead of literals. Replace the static PASSWORD/USER/URL usage with lookups
like System.getenv("DB_PASSWORD") or System.getProperty("db.password") (and
similar keys for USER/URL), provide sensible defaults or fail-fast behavior if
missing, and update any code that references URL/USER/PASSWORD to use the new
retrieval methods so credentials are not stored in source.

In `@src/main/java/org/example/MyPod.java`:
- Around line 807-809: The catch block in MyPod.java currently calls
logger.error("Kunde inte ladda data: {}", e.getMessage()) which drops the
exception stack trace; update the catch to pass the exception object to the
logger (e.g., logger.error("Kunde inte ladda data: {}", e.getMessage(), e) or
logger.error("Kunde inte ladda data", e)) so the full stack trace is preserved
when logging the error in the MyPod class catch block.
- Around line 778-779: In the catch block in MyPod (the catch(Exception e)
handling preview playback), change the logger.error call to include the
exception object so the full stack trace is preserved; pass the exception
variable e as the throwable argument to logger.error (instead of only
e.getMessage()) so the log records both the message and stack trace.
- Line 580: The log call uses e.getMessage() which drops the stack trace; change
the logger.error invocation in MyPod (the failing refresh playlists log) to pass
the Throwable as the last parameter so the stack trace is preserved, e.g.
replace the current logger.error("Failed to refresh playlists: {}",
e.getMessage()); with a call that includes the exception object as the last
argument (either logger.error("Failed to refresh playlists", e); or
logger.error("Failed to refresh playlists: {}", e.getMessage(), e);).

In `@src/main/java/org/example/repo/PlaylistRepositoryImpl.java`:
- Around line 90-93: The log message contains a typo—`insongInPlaylist`—but the
method is named isSongInPlaylist; update the logger call in the null-check
inside isSongInPlaylist to reference the correct method name (e.g.,
"isSongInPlaylist: playlist or song is null") so the message matches the actual
method and aids debugging.
- Around line 78-81: The log message in PlaylistRepositoryImpl incorrectly
references the method name "findsongsInPlaylist" (lowercase 's'); update the
logger.error call inside findSongsInPlaylist to use the correct method name
"findSongsInPlaylist" so logs match the actual method, e.g., change the message
string passed to logger.error to "findSongsInPlaylist: playlist is null" and
leave the IllegalArgumentException as-is.

In `@src/main/resources/log4j2.xml`:
- Around line 15-20: The Root logger is set to "error" which will suppress
WARN/INFO/DEBUG and the JDBC appender (DBAppender) could break logging if DB is
unreachable; change the Root logger's level to a less restrictive default (e.g.,
"info" or "warn") or make the root level configurable via a property, and update
the DBAppender/JDBC appender configuration to be resilient by adding
ignoreExceptions="true" and/or wrapping it with a Failover appender that points
to the Console appender as a fallback so logging continues if the DBAppender
fails.

🧹 Nitpick comments (7)

src/main/java/org/example/entity/LogEntry.java (1)
6-25: Entity lacks accessors - add getters if logs need to be queried.

The entity is correctly mapped to app_logs and column names are properly quoted for MySQL reserved words. However, if you plan to query logs via JPA (not just write via the JDBC appender), you'll need getter methods. The JDBC appender writes directly via SQL and doesn't use this entity.
♻️ Optional: Add getters for query access
public Long getId() { return id; }
public String getLevel() { return level; }
public String getMessage() { return message; }
public String getErrorDetails() { return errorDetails; }
public LocalDateTime getTimestamp() { return timestamp; }
pom.xml (1)
84-108: Consider using a property for the Log4j version to simplify maintenance.

Log4j 2.25.3 is the latest official release and all dependencies are correctly aligned. To avoid repetition across the four Log4j dependencies and simplify future version updates, define a version property:
♻️ Suggested property definition

Add to <properties>:
<log4j.version>2.25.3</log4j.version>
Then use ${log4j.version} in all Log4j dependencies.
src/main/java/org/example/repo/SongRepositoryImpl.java (1)
54-57: Consider using DEBUG level for expected null/blank inputs.

These null/blank checks appear to be expected code paths rather than exceptional situations. Using INFO level will generate noise in production logs. DEBUG would be more appropriate for tracing purposes.

Additionally, List.of() is more idiomatic for returning an immutable empty list.
Suggested change for one of the methods
     public List<Song> findByArtist(Artist artist) {
         if (artist == null) {
-            logger.info("findByArtist: artist is null");
-            return new ArrayList<>();
+            logger.debug("findByArtist: artist is null");
+            return List.of();
         }
Also applies to: 76-79, 98-101
src/main/java/org/example/ItunesApiClient.java (2)
66-68: Consider using WARN or DEBUG for null artistName.

A null artistName from the API is a data quality issue, not an application error. Using ERROR level suggests something is broken in the application. Since the code gracefully skips these records, WARN (to highlight data issues) or DEBUG (if this is expected) would be more appropriate.
Suggested change
             if (song.artistName() == null) {
-                logger.error("searchSongs: artistName is null");
+                logger.warn("searchSongs: skipping result with null artistName");
                 continue;
             }
55-57: Consider DEBUG level for "no results" scenario.

An empty result set from a search is expected application behavior, not something that typically warrants INFO-level logging. Using DEBUG would reduce log noise in production.
src/main/java/org/example/logging/LoggingConnectionFactory.java (1)
14-18: Explicit driver loading is unnecessary for modern JDBC.

Since JDBC 4.0 (Java 6+), drivers register themselves via ServiceLoader. The Class.forName() call can be removed.
Simplified implementation
     public static Connection getConnection() throws SQLException {
-        try {
-            Class.forName("com.mysql.cj.jdbc.Driver"); // Ensure driver is loaded
-        } catch (ClassNotFoundException e) {
-            throw new SQLException("MySQL JDBC Driver not found", e);
-        }
         return DriverManager.getConnection(URL, USER, PASSWORD);
     }
src/main/java/org/example/MyPod.java (1)

43-44: Logger field placement is unconventional.

The logger is declared at line 94 after instance fields. Convention is to place static final logger fields at the top of the class, right after the class declaration or after static constants. This improves readability and consistency.

Also applies to: 94-95

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c602ad8 and dbf293a.

📒 Files selected for processing (17)

pom.xml
src/main/java/org/example/DatabaseInitializer.java
src/main/java/org/example/ItunesApiClient.java
src/main/java/org/example/ItunesPlayList.java
src/main/java/org/example/MyPod.java
src/main/java/org/example/PersistenceManager.java
src/main/java/org/example/entity/Album.java
src/main/java/org/example/entity/DBObject.java
src/main/java/org/example/entity/LogEntry.java
src/main/java/org/example/entity/Playlist.java
src/main/java/org/example/entity/Song.java
src/main/java/org/example/logging/LoggingConnectionFactory.java
src/main/java/org/example/repo/PlaylistRepositoryImpl.java
src/main/java/org/example/repo/SongRepositoryImpl.java
src/main/resources/ipod_style.css
src/main/resources/log4j2.xml
src/test/java/org/example/RepoTest.java

🧰 Additional context used

🪛 Biome (2.1.2)

src/main/resources/ipod_style.css

[error] 56-56: Unknown property is not allowed.

See CSS Specifications and browser specific properties for more details.
To resolve this issue, replace the unknown property with a valid CSS property.