[#31421] - [4.0] Remove extra session query
- Closed
- 28 Nov 2020
- Medium
- Build: 4.0-dev
- # 31421
- Diff
- Denitz:Extra_session_SQL_query
User tests: Successful: Unsuccessful:
Pull Request for Issue #30225 .
Summary of Changes
If the session handler is database, Joomla\CMS\Session\MetadataManager::checkSessionRecordExists() performs an extra database query which is is useless because if we have active session at this stage, it means that the data was loaded from #__session table before and the session record exists.
We can check the session presence for:
- Non-database handler.
- Inactive session.
- New session (calling isNew() method results in getting session data, which also results in auto-writing the session record in the database for the new record, next it's updated via
updateSessionRecord()).
Testing Instructions
Use database session handler, load page multiple times, see that we always have 3rd query:
SELECT `session_id`
FROM `jos_session`
WHERE `session_id` = :session_id LIMIT 1
which is executed right after:
SELECT `data`
FROM `jos_session`
WHERE `session_id` = ?
Actual result BEFORE applying this Pull Request
The 3rd query always exists.
Expected result AFTER applying this Pull Request
The 3rd query from above is only executed once on a new session. You can test the new session creation and expiration by truncating the #__session table, or deleting cookies, or setting past time in #__session table.
Documentation Changes Required
No.
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | Libraries |
I have tested this item
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/31421.
| Status | Pending | ⇒ | Ready to Commit |
RTC
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/31421.
I think this need to be reviewed by someone experienced
This might look like a duplicate query, but from an engineering perspective, the current API is correct and a system engineer from the Joomla team should reject this pull request.
The changes in this pull request introduce a hidden dependency for this metadata manager class to function properly, the assumption that the session handler has promptly written a record to the database before this metadata manager can be properly used. This is the type of assumption that leads to broken code.
For background, the database session handler in Joomla 3 and earlier have this same fundamental problem in that it assumes that it can only perform UPDATE queries, which means it relies on another part of the system to run the INSERT query and if that doesn't happen for some reason then the session handler will critically fail. Luckily, someone fixed this fatal flaw in the 4.0 API.
Quy
- | Status | Ready to Commit | ⇒ | Pending |
@jfeedback The session is always started before the metadata manager runs the code (because the session ID is used by the manager), hence the data is written.
And 4.0 database session storage does INSERT in write(), am I wrong?
The comment from @jfeedback is correct from an architectural perspective. Honestly 99% of the time this code probably is fine - but there are going to be edge cases where you can get issues happening
| Status | Pending | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2020-11-28 13:01:31 |
| Closed_By | ⇒ | wilsonge | |
| Labels |
Added:
Information Required
?
|
||
I have tested this item✅ successfully on e1c1eac
Confirm: on second page load the SELECT session_id query is not present.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/31421.