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

Jump to bottom

avoid large amounts of notifications being added to the session #17736

Merged
merged 15 commits into from Aug 12, 2021
Merged

avoid large amounts of notifications being added to the session #17736

merged 15 commits into from Aug 12, 2021

Conversation

diosmosis
Copy link
Member

Description:

Fixes #17732

Changes:

  • impose limit on notification message size when logging to notifications
  • if in memory notification count exceeds max notification size in session, do not attempt to new ones it to the session
  • Detect when session was too large to read and provide warning to user.

Review

  • Functional review done
  • Potential edge cases thought about (behavior of the code with strange input, with strange internal state or possible interactions with other Matomo subsystems)
  • Usability review done (is anything maybe unclear or think about anything that would cause people to reach out to support)
  • Security review done see checklist
  • Code review done
  • Tests were added if useful/possible
  • Reviewed for breaking changes
  • Developer changelog updated if needed
  • Documentation added if needed
  • Existing documentation updated if needed

@diosmosis diosmosis added not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. Needs Review PRs that need a code review labels Jul 5, 2021
@diosmosis diosmosis added this to the 4.4.0 milestone Jul 5, 2021
sgiehl
sgiehl requested changes Jul 5, 2021
View reviewed changes
Copy link
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

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

Might it make sense to add some tests for that?

core/Notification/Manager.php Outdated Show resolved Hide resolved
@diosmosis
Copy link
Member Author

@sgiehl I added some relevant tests. I didn't add tests for all of DbTable.php or Notification\Manager.php, they were completely untested before. That might be good to do eventually.

@sgiehl
Copy link
Member

sgiehl commented Jul 7, 2021

@sgiehl I added some relevant tests. I didn't add tests for all of DbTable.php or Notification\Manager.php, they were completely untested before. That might be good to do eventually.

guess we could create a new issue for creating more/better tests for that

@diosmosis
Copy link
Member Author

@sgiehl created #17743 re adding more tests

@github-actions
Copy link
Contributor

This issue is in "needs review" but there has been no activity for 7 days. ping @matomo-org/core-reviewers

@github-actions github-actions bot added the Stale The label used by the Close Stale Issues action label Jul 15, 2021
@diosmosis diosmosis added the Do not close PRs with this label won't be marked as stale by the Close Stale Issues action label Jul 15, 2021
@github-actions github-actions bot removed the Stale The label used by the Close Stale Issues action label Jul 16, 2021
@diosmosis
Copy link
Member Author

@sgiehl @tsteur changed the session column type

sgiehl
sgiehl reviewed Jul 19, 2021
View reviewed changes
core/Version.php Outdated Show resolved Hide resolved
@diosmosis
Copy link
Member Author

@sgiehl updated

sgiehl
sgiehl requested changes Jul 19, 2021
View reviewed changes
core/Updates/4.4.0-rc3.php Outdated Show resolved Hide resolved
@sgiehl
Copy link
Member

sgiehl commented Jul 19, 2021

Should be good to merge if tests are passing again now

@tsteur
Copy link
Member

tsteur commented Jul 20, 2021

As it's quite a big PR / change let's maybe merge it after the Matomo 4.4 release (as there have been already two RC releases and the release is due soon)

@sgiehl sgiehl modified the milestones: 4.4.0, 4.5.0 Jul 22, 2021
@sgiehl sgiehl removed Do not close PRs with this label won't be marked as stale by the Close Stale Issues action Needs Review PRs that need a code review labels Jul 22, 2021
@sgiehl sgiehl modified the milestones: 4.6.0, 4.5.0 Aug 4, 2021
@sgiehl
Copy link
Member

sgiehl commented Aug 4, 2021

@diosmosis I've merged in the latest changes from 4.x-dev and updated the name of the update script as well as the version to 4.5.0-b1. Guess this should be good to merge then...

sgiehl
sgiehl reviewed Aug 10, 2021
View reviewed changes
tests/PHPUnit/Integration/Session/SaveHandler/DbTableTest.php Outdated
*
*/

namespace PHPUnit\Integration\Session\SaveHandler;
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't that be Piwik\Tests\Integration\...? Same for the other file

@diosmosis
Copy link
Member Author

@sgiehl applied the changes. The UI tests seem to be due to the version bump I think, the "new update available" widget is being displayed (w/o content).

@sgiehl
Copy link
Member

sgiehl commented Aug 11, 2021

@diosmosis we are already on 4.4.1, so the version and update scripts needs to be changed to 4.4.2-b1 or 4.5.0-b1

@diosmosis
Copy link
Member Author

diosmosis commented Aug 11, 2021 •
edited

I was sure I did that, maybe I didn't push... 😕

EDIT: Oh I see.

@diosmosis
Copy link
Member Author

diosmosis commented Aug 12, 2021 •
edited

@sgiehl build is passing (TwoFactorAuth failures unrelated)

@sgiehl sgiehl merged commit 9873cb7 into 4.x-dev Aug 12, 2021
@sgiehl sgiehl deleted the 17732-notification-log-size branch August 12, 2021 07:13
@Findus23 Findus23 mentioned this pull request Oct 8, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tsteur tsteur tsteur left review comments

@sgiehl sgiehl sgiehl approved these changes

Assignees
No one assigned
Labels
not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Milestone
4.5.0
Development

Successfully merging this pull request may close these issues.

Users can get logged out when many notifications are pending
3 participants
@diosmosis @sgiehl @tsteur