store segment hash in DB #17408

flamisz · 2021-03-30T02:32:26Z

Description:

fixes #17301

Review

flamisz · 2021-03-30T02:42:41Z

Hi @tsteur and @diosmosis,

this is very WIP, but I'd like to know if I go in the right direction.

A couple of questions I have:

should I check if the hash column exists or the updater will solve it anyway (I mean it can not happen that the code updated but DB not?)
when we create the update file? whenever we need a migration? so this PR should contain a new update file?
how we fill the hash column for the existing segments in the db? does that go into the update file?
@tsteur mentioned in the issue, ideally we can remove the urlencodes from this PR: https://github.com/matomo-org/matomo/pull/17029/files. can we @diosmosis?

sorry I have too many questions, I just'd like to understand more and do properly. thanks

diosmosis · 2021-03-30T02:52:41Z

should I check if the hash column exists or the updater will solve it anyway (I mean it can not happen that the code updated but DB not?)

It shouldn't happen that the code is updated but DB is not updated. If it does happen, then it's because of a bug in the update process, or because of user error. So you shouldn't have to check if the added column exists. (There are exceptions in this because sometimes during the one click update, some new code will execute after it is written to the hard disk, while the old database hasn't been updated, but this shouldn't be one of those cases.)

when we create the update file? whenever we need a migration? so this PR should contain a new update file?

Yes 👍 and updates are tied to a version, so first you'd bump the version in Version.php (setting to a new beta or rc based on what's in there already).

how we fill the hash column for the existing segments in the db? does that go into the update file?

Yes 👍 it should be initialized to urlencode($segment['definition']) in the update file.

@tsteur mentioned in the issue, ideally we can remove the urlencodes from this PR: https://github.com/matomo-org/matomo/pull/17029/files. can we @diosmosis?

I think it depends on how this is implemented. To get rid of those urlencodes, in the Segment constructor you'd have to search through every stored segment in the db for a definition that matches, then if found, use the hash in the database.

sorry I have too many questions, I just'd like to understand more and do properly. thanks

from what matt & thomas said, questions are to be encouraged 👍

diosmosis · 2021-03-30T02:54:03Z

I guess we don't have docs for writing updates/migrations, I'll put it on my list to try and write them (unless someone gets to them first).

flamisz · 2021-03-31T02:17:24Z

@tsteur and @diosmosis I'm wondering what else we need in this PR?
Can we get rid of those urlencode calls from the other PR? I checked that part of the code, and most of the time it gets the hash in the end of the process from the Segment, and now the getSegmentHash method does its best to find it (without anything, with urlencode and with urldecode as well).

diosmosis · 2021-03-31T02:52:52Z

core/Updates/4.3.0-b2.php

+        $segmentTable = Common::prefixTable('segment');
+        $segments = Db::fetchAll('SELECT idsegment, definition from ' . $segmentTable);
+        foreach ($segments as $segment) {
+            $hash = md5(urlencode($segment['definition']));


This should be md5($segment['definition']). Apologies, I know I said it needs to be urlencoded, I was referring to Segment construction. The Segment::getSegmentHash() function decodes the definition before md5-ing, so:

$segment = new Segment(urlencode($segment['definition'])); $hash = $segment->getHash();

is the same as

md5(urldecode(urlencode($segment['definition'])))

So if not going through Segment, calling md5 directly works (but only for definitions in the segment table).

@diosmosis and @tsteur there is an issue with this one.
If we have a segment currently in a db like this actions%3E%3D2, current Matomo version will use the hash of the urlencoded version of it, so when we create the hash here:

$hash = md5($segment['definition']);

it won't be the same as we have in the archive_numeric tables.
One solution would be using $hash = md5(urldecode($segment['definition'])); in the update file.
We don't support double encoded segments in the db, do we? Only in URL/API?

The segment condition value is encoded multiple times, not the segment condition operator.

The hash is computed overall with the fully urlencoded segment, because that is what is sent in the segment parameter. The stored segments are created by sending a definition= parameter, which does not have special treatment and gets urldecoded once automatically. So there is a discrepancy with what is in the stored segment table and what is sent as a query parameter (segment=urlencode(pageUrl==urlencode(urlencode(segmentValue))). The hash in the table has to match the hash computed w/ the fully encoded segment. The tests should cover this.

The hash that gets put in the table needs to be the same hash that is created in this test: https://github.com/matomo-org/matomo/blob/4.x-dev/plugins/SegmentEditor/tests/System/ApiTest.php#L21 when run on 4.x-dev.

thanks, @diosmosis, much more clear now.

core/Segment.php

tsteur

@flamisz looks like that should work 👍 left some minor comments

core/Segment.php

flamisz · 2021-04-11T21:06:14Z

Hi @diosmosis, can you please review it again (hopefully last time) and we can merge it then?

diosmosis · 2021-04-11T23:46:05Z

When the tests pass, I'll take another look 👍

plugins/SegmentEditor/tests/System/ApiTest.php

tests/PHPUnit/Framework/TestCase/SystemTestCase.php

This reverts commit 7e1d370.

…g/matomo into 17301-store-segment-hash-in-db

tests/PHPUnit/Integration/Segment/SegmentsCacheTest.php

tsteur · 2021-04-18T22:08:38Z

@flamisz @diosmosis was wondering if there's much left in this issue to do? Asking as we'd like to have this issue included in the next release

diosmosis · 2021-04-18T22:19:05Z

@tsteur @flamisz merged it

flamisz self-assigned this Mar 30, 2021

flamisz added the not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. label Mar 30, 2021

flamisz added this to the 4.3.0 milestone Mar 30, 2021

flamisz changed the title ~~get segment hash~~ store segment hash in DB Mar 30, 2021

flamisz marked this pull request as ready for review March 31, 2021 02:13

flamisz added the Needs Review PRs that need a code review label Mar 31, 2021

diosmosis reviewed Mar 31, 2021

View reviewed changes

flamisz removed the Needs Review PRs that need a code review label Apr 5, 2021

flamisz commented Apr 6, 2021

View reviewed changes

core/Segment.php Show resolved Hide resolved

flamisz added the Needs Review PRs that need a code review label Apr 6, 2021

tsteur requested changes Apr 8, 2021

View reviewed changes

core/Segment.php Outdated Show resolved Hide resolved

core/Segment.php Outdated Show resolved Hide resolved

core/Segment.php Outdated Show resolved Hide resolved

flamisz added Needs Review PRs that need a code review and removed Needs Review PRs that need a code review labels Apr 9, 2021

flamisz force-pushed the 17301-store-segment-hash-in-db branch 2 times, most recently from a63912e to 2a1db98 Compare April 13, 2021 02:52

tsteur reviewed Apr 13, 2021

View reviewed changes

plugins/SegmentEditor/tests/System/ApiTest.php Outdated Show resolved Hide resolved

tsteur reviewed Apr 13, 2021

View reviewed changes

tests/PHPUnit/Framework/TestCase/SystemTestCase.php Outdated Show resolved Hide resolved

flamisz added 7 commits April 15, 2021 10:48

get segment hash

425a66c

convert tab indentations to spaces

31ef8ce

Create 4.3.0-b2.php

550d329

update tests and update file

f8653c4

bump version

38f90f2

Update 4.3.0-b3.php

40ddf6a

Update ApiTest.php

0203f23

flamisz and others added 19 commits April 15, 2021 16:19

Update BlobReportLimitingTest.php

c25e605

Update SystemTestCase.php

775067b

Update BlobReportLimitingTest.php

82249bf

Update SystemTestCase.php

2ac815e

Update phpunit.xml.dist

a396d87

modify mem limit for travis

009e1a6

Update .travis.yml

8c1da56

revert travis.yml

4722abd

Update SystemTestCase.php

f5e7a87

try test without cache

92df5b1

test witch cache and gc_disabled

d652a88

Revert "test witch cache and gc_disabled"

1652714

This reverts commit 7e1d370.

test witch cache and gc_disabled

a4d91b0

use other model method

4702d2a

refactor test

a7a7565

add more tests for segment caches

96df46b

revert phpunit.xml

0096c73

Merge branch '17301-store-segment-hash-in-db' of github.com:matomo-or…

f38dbc0

…g/matomo into 17301-store-segment-hash-in-db

revert phpunit.xml

ba176ac

diosmosis reviewed Apr 15, 2021

View reviewed changes

tests/PHPUnit/Integration/Segment/SegmentsCacheTest.php Outdated Show resolved Hide resolved

add more tests

2e6b72e

diosmosis merged commit 7b1b36c into 4.x-dev Apr 18, 2021

diosmosis deleted the 17301-store-segment-hash-in-db branch April 18, 2021 22:18

tsteur restored the 17301-store-segment-hash-in-db branch April 19, 2021 20:59

tsteur deleted the 17301-store-segment-hash-in-db branch April 19, 2021 20:59

tsteur restored the 17301-store-segment-hash-in-db branch April 19, 2021 20:59

tsteur mentioned this pull request Apr 19, 2021

Store the segment hash in the DB table #17301

Closed

diosmosis mentioned this pull request May 18, 2021

Disappearance of segment archive data during 3.0 => 4.0 update #17138

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

store segment hash in DB #17408

store segment hash in DB #17408

flamisz commented Mar 30, 2021

flamisz commented Mar 30, 2021

diosmosis commented Mar 30, 2021 •

edited

diosmosis commented Mar 30, 2021

flamisz commented Mar 31, 2021

diosmosis Mar 31, 2021

flamisz Mar 31, 2021

diosmosis Mar 31, 2021

diosmosis Apr 1, 2021

flamisz Apr 1, 2021

tsteur left a comment

flamisz commented Apr 11, 2021

diosmosis commented Apr 11, 2021

tsteur commented Apr 18, 2021

diosmosis commented Apr 18, 2021

store segment hash in DB #17408

store segment hash in DB #17408

Conversation

flamisz commented Mar 30, 2021

Description:

Review

flamisz commented Mar 30, 2021

diosmosis commented Mar 30, 2021 • edited

diosmosis commented Mar 30, 2021

flamisz commented Mar 31, 2021

diosmosis Mar 31, 2021

Choose a reason for hiding this comment

flamisz Mar 31, 2021

Choose a reason for hiding this comment

diosmosis Mar 31, 2021

Choose a reason for hiding this comment

diosmosis Apr 1, 2021

Choose a reason for hiding this comment

flamisz Apr 1, 2021

Choose a reason for hiding this comment

tsteur left a comment

Choose a reason for hiding this comment

flamisz commented Apr 11, 2021

diosmosis commented Apr 11, 2021

tsteur commented Apr 18, 2021

diosmosis commented Apr 18, 2021

diosmosis commented Mar 30, 2021 •

edited