parse-community / parse-server

Parse Server for Node.js / Express
https://parseplatform.org
Apache License 2.0
20.7k stars 4.76k forks source link

fix: Parse Server option `extendSessionOnUse` not working for session lengths < 24 hours #9113

Closed vivekjoshi556 closed 1 month ago

vivekjoshi556 commented 2 months ago

Pull Request

This PR helps with dynamically extending session based on session Length.

Issue

Closes: #8981

Approach

I followed a similar approach to what @mman suggested here.

Tasks

parse-github-assistant[bot] commented 2 months ago

Thanks for opening this pull request!

vivekjoshi556 commented 2 months ago

Hello @mman, You mentioned that you would beta test. Please look at the solution and let me know what you think.

codecov[bot] commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 94.15%. Comparing base (f1469c6) to head (57f8283). Report is 13 commits behind head on alpha.

:exclamation: Current head 57f8283 differs from pull request most recent head c99c48b

Please upload reports for the commit c99c48b to get more accurate results.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## alpha #9113 +/- ## ========================================== + Coverage 94.13% 94.15% +0.01% ========================================== Files 186 186 Lines 14687 14726 +39 ========================================== + Hits 13826 13865 +39 Misses 861 861 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

mman commented 2 months ago

Thanks a lot @vivekjoshi556, I will take a look first thing next week, now OOO.

vivekjoshi556 commented 2 months ago

Hey @mman. Just wanted to check if you got some time to look at the PR.

mman commented 2 months ago

@vivekjoshi556 The code looks nice and clean, the tests as well. Will try to deploy to my stage env later today.

vivekjoshi556 commented 1 month ago

Hey @mman Just wanted to check in again.

@mtrezza if you have inputs as well, please let me know.

There is a minor issue with linting I'll fix it in the next push, anything else let me know.

mtrezza commented 1 month ago

This fix just shifts the problem downwards from 24 hours to 1 minute. Because for a session length of <1 minute, the problem in https://github.com/parse-community/parse-server/issues/8981 persists.

I wonder whether we need such a complex logic for this. If I understand https://github.com/parse-community/parse-server/issues/8981#issuecomment-1976147677 correctly, the purpose of this logic is to find a middle ground between:

Wouldn't it be simpler to extend the session when the remaining session token validity time reached < 50% of the session length?

For example:

Pros:

And could you please document this new behavior in the Parse Server option extendSessionOnUse?

mtrezza commented 1 month ago

@vivekjoshi556 Just a friendly ping here; if you could simplify the logic and add the functionality description to the option docs (this repo, not the docs repo), then we could go ahead and merge.

vivekjoshi556 commented 1 month ago

So sorry just got a bit swamped with work. Give me some time

vivekjoshi556 commented 1 month ago

@mtrezza I have made the mentioned changes. Please let me know if anything else needs to be done.

parseplatformorg commented 1 month ago

🎉 This change has been released in version 7.1.0-alpha.9

mman commented 1 month ago

@vivekjoshi556 Excellent, very clean code, putting into production :)

parseplatformorg commented 5 days ago

🎉 This change has been released in version 7.1.0-beta.1

parseplatformorg commented 5 days ago

🎉 This change has been released in version 7.1.0