fix: should not print error when session doesn't exist

[hwbrzzl]

📑 Description

Summary by CodeRabbit

• New Features

  • Enhanced robustness of session handling by preventing unnecessary processing of empty values.

• Bug Fixes

  • Adjusted expected outcomes in tests to reflect the new behavior of the Read method, which now returns no error for non-existent sessions.
  • Removed specific session not found error handling, simplifying error management.

• Tests

  • Updated assertions in the test suite to expect no errors when reading from destroyed or garbage-collected files.

✅ Checks

• Added test cases for my code

[coderabbitai]

Walkthrough

The pull request introduces modifications across four files: session/driver/file.go, session/driver/file_test.go, session/session.go, and errors/list.go. The primary changes involve simplifying error handling in the Read method of the File struct, updating corresponding test cases to expect no errors in scenarios that previously returned errors, enhancing the readFromHandler method in the Session struct to prevent unmarshalling of empty values, and removing the SessionNotFound error variable from the errors package.

Changes

File	Change Summary
session/driver/file.go	- Removed error handling in Read method - Now returns nil error instead of errors.SessionNotFound - Removed errors package import
session/driver/file_test.go	- Updated test assertions in TestDestroy, TestGc, and TestRead - Changed error expectations from f.NotNil(err) to f.Nil(err)
session/session.go	- Added conditional check in readFromHandler method - Prevents unmarshalling of empty values - Maintains existing error logging behavior
errors/list.go	- Removed variable SessionNotFound

Sequence Diagram

sequenceDiagram
    participant Client
    participant Session
    participant FileDriver
    
    Client->>Session: Read session data
    Session->>FileDriver: Invoke Read method
    FileDriver-->>Session: Return data (or empty string)
    Session->>Session: Check if value is not empty
    alt Value is not empty
        Session->>Session: Unmarshal data
    else Value is empty
        Session->>Session: Skip unmarshalling
    end
    Session-->>Client: Return processed data or nil

Loading

---

📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9619463 and bb8f850.

📒 Files selected for processing (1)

• errors/list.go (0 hunks)

💤 Files with no reviewable changes (1)

• errors/list.go

---

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?

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>, please review it.
  • Generate unit testing code for this file.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai generate unit testing code for this file.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
  • @coderabbitai read src/utils.ts and generate unit testing code.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
  • @coderabbitai help me debug CodeRabbit configuration file.

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)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

Other keywords and placeholders

• Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
• Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
• Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• Please see the configuration documentation for more information.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.96%. Comparing base (8afbbef) to head (bb8f850).
Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #768      +/-   ##
==========================================
- Coverage   69.98%   69.96%   -0.03%     
==========================================
  Files         213      213              
  Lines       18118    18119       +1     
==========================================
- Hits        12680    12677       -3     
- Misses       4748     4750       +2     
- Partials      690      692       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (2)

session/driver/file_test.go (1)

42-42: Update test descriptions to reflect new behavior

The test assertions have been updated to expect nil errors, but the test function names and comments don't clearly indicate this new behavior. Consider adding comments to explicitly document that non-existent sessions should return empty string without error.

Consider adding comments like:

 func (f *FileTestSuite) TestDestroy() {
+  // Reading a destroyed session should return empty string without error
   ...
 }

 func (f *FileTestSuite) TestGc() {
+  // Reading an expired session should return empty string without error
   ...
 }

 func (f *FileTestSuite) TestRead() {
+  // Reading a non-existent session should return empty string without error
   ...
 }

Also applies to: 61-62, 91-92

session/session.go (1)

247-253: Consider consolidating error handling

While the empty string check prevents unnecessary JSON unmarshaling, we're still logging errors that might be expected. Consider:

1. Distinguishing between expected and unexpected errors
2. Only logging unexpected errors
3. Using structured logging for better error tracking

 func (s *Session) readFromHandler() map[string]any {
   value, err := s.driver.Read(s.GetID())
-  if err != nil {
+  if err != nil && !errors.Is(err, ErrSessionNotFound) {
     color.Errorln(err)
     return nil
   }
   var data map[string]any
   if value != "" {
     if err := s.json.Unmarshal([]byte(value), &data); err != nil {
-      color.Errorln(err)
+      color.Errorln("Failed to unmarshal session data:", err)
       return nil
     }
   }

   return data
 }

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9dddb62 and 9619463.

📒 Files selected for processing (3)

• session/driver/file.go (1 hunks)
• session/driver/file_test.go (3 hunks)
• session/session.go (1 hunks)

🔇 Additional comments (1)

session/driver/file.go (1)

82-82: Consider following Go idioms for "not found" cases

While removing the error helps achieve the goal of not printing errors for non-existent sessions, returning (nil, nil) for "not found" cases doesn't follow common Go idioms. Consider either:

1. Following the standard Go pattern of returning ("", ErrNotFound) but handling this specific error differently in the session package, or
2. Adding documentation to explicitly state that this is intentional behavior

Let's check how other session drivers handle this case:

[devhaozi]

Q: Does error errors.SessionNotFound need to be deleted?

[hwbrzzl]

Good question, we can remove it.

[github-actions]

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.

Benchmark suite	Current: bb8f850	Previous: 8afbbef	Ratio
Benchmark_Fatal	2e-7 ns/op 0 B/op 0 allocs/op	1e-7 ns/op 0 B/op 0 allocs/op	2
Benchmark_Fatal - ns/op	2e-7 ns/op	1e-7 ns/op	2

This comment was automatically generated by workflow using github-action-benchmark.

[devhaozi]

LGTM