Skip to content

feat: enhance session manager to support custom drivers#1005

Merged
hwbrzzl merged 5 commits intogoravel:masterfrom
merouanekhalili:make-session-driver-extendable
Apr 16, 2025
Merged

feat: enhance session manager to support custom drivers#1005
hwbrzzl merged 5 commits intogoravel:masterfrom
merouanekhalili:make-session-driver-extendable

Conversation

@merouanekhalili
Copy link
Contributor

@merouanekhalili merouanekhalili commented Apr 12, 2025

Refactor session manager architecture to allow for custom driver integration. Update file driver implementation to be compatible with these changes, making the session system more extensible and flexible.

📑 Description

Overview

This pull request introduces a significant refactor of the Session Manager to allow the use of custom session drivers, including Redis. The initial goal was to implement a Redis session driver directly; however, I discovered that the current implementation of the Session Manager did not support such integration. To overcome this limitation, I refactored both the Session Manager and its associated file driver, paving the way for adding custom session drivers.

Updated Session Configuration:

package config

import (
	"github.com/goravel/framework/contracts/session" // Required for Driver type in factory signature
	"github.com/goravel/framework/facades"
	fileriver "github.com/goravel/framework/session/driver" // Import the package containing the driver AND its factory
	"github.com/goravel/framework/support/path"
	"github.com/goravel/framework/support/str"
)

func init() {
	config := facades.Config()
	config.Add("session", map[string]any{
		// Default Session Driver
		//
		// This option controls the default session "driver" that will be used on
		// requests. By default, we will use the lightweight file session driver, but you
		// may specify any of the other wonderful drivers provided here.
		//
		// Supported: "file"
		"driver": config.Env("SESSION_DRIVER", "file"),

		// Session Lifetime
		//
		// Here you may specify the number of minutes that you wish the session
		// to be allowed to remain idle before it expires. If you want them
		// to immediately expire when the browser is closed, then you may
		// indicate that via the expire_on_close configuration option.
		"lifetime": config.Env("SESSION_LIFETIME", 120),

		"expire_on_close": config.Env("SESSION_EXPIRE_ON_CLOSE", false),

		// Session File Location
		//
		// When using the file session driver, we need a location where the
		// session files may be stored. A default has been set for you, but a
		// different location may be specified. This is only needed for file sessions.
		"files": path.Storage("framework/sessions"),

		// Session Garbage Collection Running Time Interval (in minutes)
		//
		// Here you may specify how many minutes you want the session to be allowed
		// to remain idle before its garbage collection routine runs. If you want
		// to avoid hitting the garbage collection routine, you may set this value to -1.
		"gc_interval": config.Env("SESSION_GC_INTERVAL", 30),

		// Session Cookie Name
		//
		// Here you may change the name of the cookie used to identify a session
		// in the application. The name specified here will get used every time
		// a new session cookie is created by the framework for every driver.
		"cookie": config.Env("SESSION_COOKIE", str.Of(config.GetString("app.name")).Snake().Lower().String()+"_session"),

		// Session Cookie Path
		//
		// The session cookie path determines the path for which the cookie will
		// be regarded as available.Typically, this will be the root path of
		// your application, but you are free to change this when necessary.
		"path": config.Env("SESSION_PATH", "/"),

		// Session Cookie Domain
		//
		// Here you may change the domain of the cookie used to identify a session
		// in your application.This will determine which domains the cookie is
		// available to in your application.A sensible default has been set.
		"domain": config.Env("SESSION_DOMAIN", ""),

		// HTTPS Only Cookies
		//
		// By setting this option to true, session cookies will only be sent back
		// to the server if the browser has an HTTPS connection. This will keep
		// the cookie from being sent to you if it cannot be done securely.
		"secure": config.Env("SESSION_SECURE", false),

		// HTTP Access Only
		//
		// Setting this to true will prevent JavaScript from accessing the value of
		// the cookie, and the cookie will only be accessible through the HTTP protocol.
		"http_only": config.Env("SESSION_HTTP_ONLY", true),

		// Same-Site Cookies
		//
		// This option determines how your cookies behave when cross-site requests
		// take place, and can be used to mitigate CSRF attacks.By default, we
		// will set this value to "lax" since this is a secure default value.
		"same_site": config.Env("SESSION_SAME_SITE", "lax"),

		// Session Driver Configurations
		//
		// This map defines the available session drivers and how to create them.
		// The Session Manager reads this map to register driver factories.
		"drivers": map[string]any{

			// Example: Redis Driver Configuration (User would add this)
			/*
				"redis": map[string]any{
					"driver":     "custom",
					"connection": "default"

					// 'via' points to the factory function for the Redis driver
					"via": func() (session.Driver, error) {
						return redisfacades.RedisSession("redis"), nil
					},
				},
			*/
		},
	})
}

Refactor session manager architecture to allow for custom driver integration.
Update file driver implementation to be compatible with these changes,
making the session system more extensible and flexible.
Copilot AI review requested due to automatic review settings April 12, 2025 22:25
@merouanekhalili merouanekhalili requested a review from a team as a code owner April 12, 2025 22:25
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

@krishankumar01
Copy link
Member

krishankumar01 commented Apr 13, 2025

It looks like no changes are needed for the file driver. We already support custom drivers: https://www.goravel.dev/the-basics/session.html#add-custom-session-drivers.

Copy link
Contributor

@hwbrzzl hwbrzzl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, a great PR. For the file driver, we don't need to add "via": fileriver.FileDriverFactory, in the configuration file, it's an internal driver. User can add drivers when using a custom driver. And CI failed.

@hwbrzzl
Copy link
Contributor

hwbrzzl commented Apr 13, 2025

It looks like no changes are needed for the file driver. We already support custom drivers: https://www.goravel.dev/the-basics/session.html#add-custom-session-drivers.

It seems only structure is optimized, logic is not changed.


filePath := f.getFilePath(id)

// 1. Check existence first (optimization)
Copy link
Member

@krishankumar01 krishankumar01 Apr 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's consider if comments are really necessary

Ref: https://refactoring.guru/smells/comments

Revert back the file driver to the original content
Make sure that the gc use UTC when checking ModTime
Follow goravel code style
@codecov
Copy link

codecov bot commented Apr 13, 2025

Codecov Report

Attention: Patch coverage is 70.58824% with 20 lines in your changes missing coverage. Please review.

Project coverage is 70.19%. Comparing base (4dc9b57) to head (9eb0b8e).
Report is 4 commits behind head on master.

Files with missing lines Patch % Lines
session/manager.go 70.14% 16 Missing and 4 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1005      +/-   ##
==========================================
- Coverage   70.23%   70.19%   -0.04%     
==========================================
  Files         169      169              
  Lines       11456    11498      +42     
==========================================
+ Hits         8046     8071      +25     
- Misses       3059     3073      +14     
- Partials      351      354       +3     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@merouanekhalili
Copy link
Contributor Author

Hi Team,

I reverted the file driver back to the original content as you suggested, I only updated the GC to convert info.ModTime to UTC before checking
I also made sure all suggestions are implemented

Thanks

@merouanekhalili
Copy link
Contributor Author

Hi Team,

Just for information I just pushed the redis driver for Session module :

https://github.com/merouanekhalili/session-redis-driver

Thanks

Comment on lines +185 to +188
err := m.Extend(name, factoryWrapper)
if err != nil {
color.Errorf("Failed to register driver '%s': %s\n", name, err)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
err := m.Extend(name, factoryWrapper)
if err != nil {
color.Errorf("Failed to register driver '%s': %s\n", name, err)
}
return m.Extend(name, factoryWrapper)

@hwbrzzl
Copy link
Contributor

hwbrzzl commented Apr 14, 2025

Hi Team,

Just for information I just pushed the redis driver for Session module :

https://github.com/merouanekhalili/session-redis-driver

Thanks

Amazing. Could you merge it into https://github.com/goravel/redis? We can set it as an official implementation.

Copy link
Contributor

@hwbrzzl hwbrzzl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, almost complete. 👍

Add file drive case  to registerConfiguredDrivers func
Copy link
Contributor

@hwbrzzl hwbrzzl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, perfect!

@hwbrzzl hwbrzzl merged commit a73eeea into goravel:master Apr 16, 2025
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants