Remove cache data from the symfony events #30793

sharidas · 2018-03-15T16:13:33Z

It was a mistake to include cache data.
This change removes the cache data from the
symfony event. And hence the listener of
the event would avail update and oldvalue
from the events.

Signed-off-by: Sujith H sharidasan@owncloud.com

Description

Remove cache data from the symfony event dispatcher.

Related Issue

Motivation and Context

Remove cache data from the symfony event dispatcher. Its not ideal to expose too much information.

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

codecov · 2018-03-15T17:18:07Z

Codecov Report

Merging #30793 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master   #30793      +/-   ##
============================================
+ Coverage     61.98%   61.98%   +<.01%     
- Complexity    18369    18536     +167     
============================================
  Files          1157     1157              
  Lines         69080    69088       +8     
  Branches       1242     1242              
============================================
+ Hits          42818    42826       +8     
  Misses        25890    25890              
  Partials        372      372

Flag	Coverage Δ	Complexity Δ
#javascript	`52.33% <ø> (ø)`	`0 <ø> (ø)`	⬇️
#phpunit	`63.08% <100%> (ø)`	`18536 <7> (+167)`	⬆️

Impacted Files	Coverage Δ	Complexity Δ
lib/private/AppConfig.php	`98.26% <100%> (+0.04%)`	`29 <7> (+2)`	⬆️
lib/private/Config.php	`89.9% <100%> (+0.48%)`	`43 <0> (+1)`	⬆️
lib/public/Events/EventEmitterTrait.php	`100% <100%> (ø)`	`8 <0> (+1)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7273799...410d503. Read the comment docs.

PVince81 · 2018-03-16T13:02:04Z

lib/private/AppConfig.php

-			'before' => ['key' => $key, 'value' => $value, 'app' => $app, 'appcache' => isset($this->cache[$app]) ? $this->cache[$app] : null],
-			'after' => ['key' => $key, 'value' => $value, 'app' => $app, 'appcache' => isset($this->cache[$app]) ? $this->cache[$app] : null]
+			'before' => ['key' => $key, 'value' => $value, 'app' => $app],
+			'after' => ['key' => $key, 'value' => $value, 'app' => $app, 'update' => &$update, 'oldvalue' => &$oldValue]


why the & sign here ?

Because when the update value is updated https://github.com/owncloud/core/pull/30793/files#diff-cdb763d0bc05b09a392296fde32add1dR209 and & is not used, then the updated true is not used. Hence the update value will not be reflected in the variable update. Same for oldValue.

the & sign is ok in the use() block but why here in the "after" block ? also you didn't set a & in the other hooks in this location

The problem I felt was https://github.com/owncloud/core/blob/master/lib/public/Events/EventEmitterTrait.php#L46. The arguments array which has both before and after arrays, is passed before any change is made. So if we don't use & then no update will be reflected. This is one of the reason I find the other hooks I haven't used &.

as far as I understand how PHP works, if you add a & on this line (the one my comment points to), it means that the array consumer will be able to modify the values. Ok, now I see that the array consumer is actually the method defined just before that. So it works as expected but feels a bit convoluted and confusing. I don't expect anyone who decides to add a hook to understand when and when not to add &.

Anyway, since this event emitter pattern has already been reviewed and approved before, let's keep the pattern as is.

Hmmmmm wait, but https://github.com/owncloud/core/blob/master/lib/public/Events/EventEmitterTrait.php#L46 isn't even using $update directly. It's just passing the array as is to the GenericEvent. The GenericEvent arguments must be modified using setArgument('update', ...) and not to PHP references.

Have you tried what happens if you remove the & ? Does it stop working ?

follow up here https://github.com/owncloud/core/pull/30793/files#r175119030

PVince81 · 2018-03-16T15:05:08Z

lib/private/AppConfig.php

@@ -193,12 +200,18 @@ public function setValue($app, $key, $value) {

 			$changedRow = (bool) $sql->execute();

+			if (isset($this->cache[$app][$key])) {
+				$oldValue = $this->cache[$app][$key];


ohhh, so this line retroactively modifies the value of $oldValue as referenced in the after array... yeah, that's very convoluted and confusing.

any chance to read the $oldValue earlier ? or have some of the code outside of the callback ?

other possible approaches:

have the callback receive the "before" and "after" arrays as additional function arguments, so it can simply modify the values there when needed. the modification operation will be easier to read and understand than magic updating through reference defined elsewhere

or

don't use the emittingCall wrapper and send dispatcher events directly before and after the operation

Its better to have approach one. And it worked. Had a basic test with file creation + folder creation, rename, deletion and restore operations. It didn't caused any warning or error in the log file. And the logs were populated. But we may have to update other places where emittingCall is made. So that we can have callback receive 'after' array.

any chance to read the $oldValue earlier ? or have some of the code outside of the callback ?

yes I updated the PR by moving this logic outside emittingCall.

PVince81 · 2018-03-19T09:18:29Z

lib/private/AppConfig.php

+		$oldValue = null;
+
+		if (isset($this->cache[$app][$key])) {
+			$oldValue = $this->cache[$app][$key];


assuming there is a guarantee that the cache is always already populated at this point

I have updated for and verified cache has keys $app and $key set. If set then update the oldvalue and update var respectively.

it's not about whether to check programmatically

are there any real life scenarios where the cache isn't initialized when we reached this point ? because that could make the event mistakenly think that a key did not exist and have "$update=false" despite the key existing in DB but not being in cache

please verify the logic that populates the cache and see whether it's compatible with the expectation here

Got it. There is a method loadConfigValues() which can help us in such scenarios if configLoaded is set to false before calling loadConfigValues. That would at least make sure that the data fetched from DB is consistent with the cache. But there is a drawback. It would cause performance, I believe. For every setValue call made, it would fetch the data from the DB ( since the configLoaded is set to false).

Whenever the OC_APP::getAppVersions() is called the cache is populated. Another way I see the cache update happens is when the apps are getting enabled.

So basically whenever the call

$appConfig = \OC::$server->getAppConfig();

with any of the get or delete method is called in AppConfig the cache remains updated. Guess chances are bit low that such a scenario would be hit.

ok, let's go the safe route then, how about approach 1 from #30793 (comment) ?

can you elaborate ? all I see is "and" replaced with "&&"

ok I see it now... hmmm

this also means that any time you use "after" you are forced to define it as argument in the callback ?

in the case of other emitters, aren't they also using "after" and not specifying the receiving argument ?

if that works then I'm fine with this change

The receiving argument is not mandatory. That is we can have either

$this->emittingCall(function() use(&$a, &$b) {}, ['before' => [], 'after' => []], 'foo', 'update');

or

$this->emittingCall(function($afterArray) use (&$a, &$b) {}, ['before => [], 'after' => []], 'foo', 'read');

Adding a unit test for the same in for EventEmitterTrait.

It was a mistake to include cache data. This change removes the cache data from the symfony event. And hence the listener of the event would avail update and oldvalue from the events. Signed-off-by: Sujith H <sharidasan@owncloud.com>

PVince81

👍

PVince81 · 2018-03-27T13:34:50Z

@sharidas please backport

sharidas · 2018-03-27T13:40:23Z

Backport PR: #30937

lock · 2019-07-31T15:02:14Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

sharidas added the 2 - Developing label Mar 15, 2018

sharidas added this to the development milestone Mar 15, 2018

sharidas self-assigned this Mar 15, 2018

sharidas requested a review from PVince81 March 16, 2018 07:06

sharidas added 3 - To Review and removed 2 - Developing labels Mar 16, 2018

PVince81 reviewed Mar 16, 2018

View reviewed changes

sharidas force-pushed the remove-cache-from-symfony-event branch 2 times, most recently from a238ba1 to 0b44401 Compare March 19, 2018 08:19

PVince81 reviewed Mar 19, 2018

View reviewed changes

sharidas force-pushed the remove-cache-from-symfony-event branch 3 times, most recently from 2348475 to 1f3ef66 Compare March 19, 2018 13:46

Remove cache data from the symfony events

410d503

It was a mistake to include cache data. This change removes the cache data from the symfony event. And hence the listener of the event would avail update and oldvalue from the events. Signed-off-by: Sujith H <sharidasan@owncloud.com>

sharidas force-pushed the remove-cache-from-symfony-event branch from 1f3ef66 to 410d503 Compare March 19, 2018 14:45

ownclouders added the status/STALE label Mar 27, 2018

sharidas removed the status/STALE label Mar 27, 2018

PVince81 approved these changes Mar 27, 2018

View reviewed changes

PVince81 merged commit f9dd178 into master Mar 27, 2018

PVince81 deleted the remove-cache-from-symfony-event branch March 27, 2018 13:34

sharidas added the backport-request label Mar 27, 2018

sharidas mentioned this pull request Mar 27, 2018

[stable10] Remove cache data from the symfony events #30937

Merged

9 tasks

PVince81 removed the backport-request label Mar 28, 2018

lock bot locked as resolved and limited conversation to collaborators Jul 31, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove cache data from the symfony events #30793

Remove cache data from the symfony events #30793

sharidas commented Mar 15, 2018

codecov bot commented Mar 15, 2018 •

edited

PVince81 Mar 16, 2018

sharidas Mar 16, 2018

PVince81 Mar 16, 2018

sharidas Mar 16, 2018 •

edited

PVince81 Mar 16, 2018

PVince81 Mar 16, 2018

PVince81 Mar 16, 2018

PVince81 Mar 16, 2018

PVince81 Mar 16, 2018

sharidas Mar 19, 2018 •

edited

sharidas Mar 19, 2018

PVince81 Mar 19, 2018

sharidas Mar 19, 2018

PVince81 Mar 19, 2018

sharidas Mar 19, 2018 •

edited

PVince81 Mar 19, 2018

PVince81 Mar 19, 2018

PVince81 Mar 19, 2018

PVince81 Mar 19, 2018

sharidas Mar 19, 2018

sharidas Mar 19, 2018

PVince81 left a comment

PVince81 commented Mar 27, 2018

sharidas commented Mar 27, 2018

lock bot commented Jul 31, 2019

Remove cache data from the symfony events #30793

Remove cache data from the symfony events #30793

Conversation

sharidas commented Mar 15, 2018

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

codecov bot commented Mar 15, 2018 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharidas Mar 16, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharidas Mar 19, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharidas Mar 19, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PVince81 left a comment

Choose a reason for hiding this comment

PVince81 commented Mar 27, 2018

sharidas commented Mar 27, 2018

lock bot commented Jul 31, 2019

codecov bot commented Mar 15, 2018 •

edited

sharidas Mar 16, 2018 •

edited

sharidas Mar 19, 2018 •

edited

sharidas Mar 19, 2018 •

edited