Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Redis Session driver #621

Merged
merged 28 commits into from
May 22, 2022

Conversation

circulon
Copy link
Contributor

@circulon circulon commented May 8, 2022

This is an initial implementation of a driver for Redis backed sessions

@josephmancuso @girardinsamuel
Interested in your opinions on how I have implemented this

Tests are not built yet.... coming soon

@girardinsamuel girardinsamuel changed the title redis session driver Add Redis Session driver May 10, 2022
Copy link
Contributor

@girardinsamuel girardinsamuel left a comment

Choose a reason for hiding this comment

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

For now the architecture looks good !

@girardinsamuel
Copy link
Contributor

Hi !
Looks like linting needs to be fixed on this PR 😉

@circulon
Copy link
Contributor Author

@girardinsamuel @josephmancuso

OK I think this is completed now.
Added tests and reworked config and general compatibility with rxisting session methods

Copy link
Member

@josephmancuso josephmancuso left a comment

Choose a reason for hiding this comment

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

Looking good! I gave it a code review. Just some things I saw and some comments to make the code more similar to our other features

src/masonite/drivers/session/RedisDriver.py Outdated Show resolved Hide resolved
src/masonite/sessions/Session.py Outdated Show resolved Hide resolved
tests/integrations/config/session.py Outdated Show resolved Hide resolved
@girardinsamuel girardinsamuel added the Next Minor Non-breaking change that can go into the next minor version label May 17, 2022
src/masonite/drivers/session/RedisDriver.py Outdated Show resolved Hide resolved
tests/integrations/config/session.py Outdated Show resolved Hide resolved
tests/features/session/test_cookie_session.py Show resolved Hide resolved
@girardinsamuel
Copy link
Contributor

girardinsamuel commented May 20, 2022

Hi @circulon, thank you again for your contribution.
One thing I missed in the first review are the type hints. We are progressively type hinting the whole Masonite codebase (It should be completed for Masonite 5). As the Cookie session driver is type hinted, could you do the same for Redis session driver ? You can check how it's done in masonite/drivers/session/CookieDriver.py. 👍

Note: i have seen it's partially done, so it should just be adding some missing type hints on some methods.

@circulon
Copy link
Contributor Author

Hi @circulon, thank you again for your contribution. One thing I missed in the first review are the type hints. We are progressively type hinting the whole Masonite codebase (It should be completed for Masonite 5). As the Cookie session driver is type hinted, could you do the same for Redis session driver ? You can check how it's done in masonite/drivers/session/CookieDriver.py. 👍

Note: i have seen it's partially done, so it should just be adding some missing type hints on some methods.

Hi @girardinsamuel
I appreciate the thanks ;)

Yeah my project requires Redis for the cache and I use a thitf party auth system so internal lookups to validate the user token are just not feasible.
So I decided to put the user object in the session as many other frameworks do. Turns out having tokens in the user object exceeds the cookie size limit ;(

As the Redis Cache driver was already built it was just a matter of reworking that driver to fit the way Sessions work.
So there is a lot of reused code from the cache driver as you might have noticed.

Regarding Type hinting I have added most of them. There are a couple of methods like "set" that I'm not exactly sure of the return type so any pointers would be appreciated.

Copy link
Contributor

@girardinsamuel girardinsamuel left a comment

Choose a reason for hiding this comment

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

Here are missing type hints to help you.
For some return types you have to look at redis methods return type to know what is the type.

tests/integrations/config/session.py Show resolved Hide resolved
src/masonite/drivers/session/RedisDriver.py Outdated Show resolved Hide resolved
src/masonite/drivers/session/RedisDriver.py Outdated Show resolved Hide resolved
src/masonite/drivers/session/RedisDriver.py Outdated Show resolved Hide resolved
src/masonite/drivers/session/RedisDriver.py Outdated Show resolved Hide resolved
src/masonite/drivers/session/RedisDriver.py Outdated Show resolved Hide resolved
src/masonite/drivers/session/RedisDriver.py Outdated Show resolved Hide resolved
Copy link
Contributor

@girardinsamuel girardinsamuel left a comment

Choose a reason for hiding this comment

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

Final review ! Looks good to me.
@josephmancuso I guess we can merge this now.

Copy link
Member

@josephmancuso josephmancuso left a comment

Choose a reason for hiding this comment

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

Final code review looks good

@josephmancuso josephmancuso merged commit 77c7739 into MasoniteFramework:4.0 May 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Next Minor Non-breaking change that can go into the next minor version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants