Revert the order of page_restrictions rev key #718

Pchelolo · 2016-11-18T21:49:10Z

Final polish needed to enable the restrictions table. Still need to reload the data to the new table and backfill the redirects, but overall we're good to go. All unit tests pass (sqlite depends on wikimedia/restbase-mod-table-sqlite#37)

Done a quite extensive testing pass in vagrant, all looks good. Vagrant depends on https://gerrit.wikimedia.org/r/#/c/322358/

Bug: https://phabricator.wikimedia.org/T149965

gwicke · 2016-11-22T03:36:14Z

sys/page_revisions.js

@@ -32,7 +32,7 @@ const tableName = 'title_revisions';
 * @type {string}
 * @const
 */
-const restrictionsTableName = 'restrictions';
+const restrictionsTableName = 'page_restrictions';


A better name in any case.

@gwicke Ye, we've needed to rename it anyways..

gwicke · 2016-11-22T03:38:53Z

sys/parsoid.js

@@ -318,7 +318,8 @@ class ParsoidService {
                            // This revision is actually a redirect. Pass redirect target
                            // to caller, and let it rewrite the location header.
                            resp.status = 302;
-                            resp.headers.location = encodeURIComponent(redirectTarget);
+                            // Filter will take care of uri-encoding
+                            resp.headers.location = redirectTarget;


This strikes me as a bit brittle. Is this mainly meant as a performance optimization, or is there another reason to defer encoding?

@gwicke To be able to share the same codepath within the filter

Could the decoding be done once in the filter & the result be stored in a local variable? The performance impact should be pretty small, and in exchange we'd avoid introducing an exception to the "regular HTTP headers" principle & assumptions about which filters are active.

A that's possible. But that would mean doing encode-decode-encode cycle instead of just one encode

Yeah, but a) redirects are rare, and b) encoding & decoding a small string is fairly cheap. IMHO, consistency & lack of coupling is more important.

gwicke · 2016-11-22T03:40:05Z

sys/page_revisions.js

@@ -261,7 +262,8 @@ class PRS {
    storePageDeletion(hyper, req, revision) {
        return this.storeRestrictions(hyper, req, {
            title: req.params.title,
-            rev: req.params.revision || revision.rev,
+            // We're storing page_deletions with magic 0 rev just to update the static column


Perhaps also worth mentioning that 0 is chosen so that it will always match for any query for less-or-equal some revision greater than 0.

gwicke · 2016-11-22T03:42:59Z

lib/access_check_filter.js

@@ -64,6 +66,9 @@ module.exports = (hyper, req, next, options, specInfo) => {
                }

                return contentPromise.then((content) => redirect(content, location, options));
+            } else if (content.status === 302) {


Is the intention here to override stored redirects?

We return 302 when content's not in storage && it's a redirect. For ?redirect=false and some other conditions we don't what 302 but want 200 instead

To me it looks as if this condition would fire when content was in storage, but the restriction table did not have the redirect. Is that intended?

@gwicke nono. We're only setting 302 in parsoid.js when the content was not in storage because in that case restriction table update would be concurrent with the content request because we've just parsed out the redirect.

When we're taking the content from the storage we don't set 302 and rely on the filter to get it from the restrictions table

I see -- that's pretty subtle. Do you think an explaining comment here could be helpful to remind ourselves about how this interacts with parsoid.js?

I think that in the perfect world we wouldn't have such a weird hard dependency at all, but I couldn't think of a way to get rid of it. I'd add a comment

gwicke · 2016-11-22T03:45:41Z

LGTM overall, just a couple of questions in line.

Pchelolo · 2016-11-22T19:48:27Z

@gwicke Updated to address your comments

Pchelolo · 2017-09-29T10:41:49Z

I've rebased the PR on top of the latest master. The tests are currently failing, see the Phablet ticket for an explanation why are they failing and a discussion on what to do with that.

Pchelolo · 2017-09-29T12:13:43Z

I've adapted the patch to the new storage (we don't need to supply a tacky artificial fixed _tid any more!!) and made the tests pass by making them sequential. The tests need more attention, see discussion on the ticket.

Also this one already starts using the new filter to demonstrate that the tests are passing. However, we need to revert that part before deploying because we don't have the restrictions filled in and also we wanna make a proxy to run both filters in parallel in the beginning and check that the results are correct. Before building the proxy filter I will do some manual correctness testing though.

Pchelolo · 2017-09-29T13:31:15Z

I did manually test this locally and in vagrant for various scenarios of page creation/renaming/deleting/redirecting etc and it seems to work correct for all the cases I'm managed to imagine.

TODO: Create a proxy that will run both filters and compare the results.

gwicke · 2017-10-02T21:16:33Z

lib/access_check_filter.js

@@ -64,6 +67,12 @@ module.exports = (hyper, req, next, options, specInfo) => {
                }

                return contentPromise.then(content => redirect(content, location, options));
+            } else if (content.status === 302) {
+                // Parsoid.js emits a redirect when the new content is parsed because
+                // there could be a race condition with updating a restriction table.


Is this about the time window between a new restriction being created (ex: revision suppressed), and the update for this being processed in ChangeProp?

gwicke · 2017-10-02T21:20:45Z

👍

Change-Id: I067887abf493a6fbb51ba3530443ee26c6c484c5

Change-Id: I2c0db7395a5e553e4e5307d004831b98722b2b3d

Pchelolo · 2017-11-08T12:13:48Z

Finally ready!

Change-Id: I84e22519126bf9677ed9e17ec1d52a32227a0c20

d00rman

LGTM in general, one in-lined question. Also, it would be best if we created the Cass3 tables before we merge this, so as not to shoot ourselves in the foot.

d00rman · 2017-11-08T13:14:20Z

lib/access_check_filter.js

+            } else if (content.status === 302) {
+                // Parsoid.js emits a redirect when the new content is parsed because
+                // there could be a race condition with updating a restriction table.
+                // If the client doesn't want a redirect - remove it.


How can we know for sure that the client doesn't want a redirect here simply based on the status code (which implies otherwise) ?

This is inside an else-if clause, if client wanted a redirect or we wanted to provide it - we wouldn've going into the if clause and never even evaluated this condition comparing the status to 302

gwicke reviewed Nov 22, 2016

View reviewed changes

Pchelolo force-pushed the revert_restriction_table branch from 3f17420 to 5b0d6fd Compare November 22, 2016 19:41

Pchelolo mentioned this pull request Nov 22, 2016

Added a script to reload data from old restrictions table to the new one wikimedia/restbase-mod-table-cassandra#198

Closed

Pchelolo force-pushed the revert_restriction_table branch from 5b0d6fd to 80e50c7 Compare September 29, 2017 10:23

gwicke reviewed Oct 2, 2017

View reviewed changes

Pchelolo mentioned this pull request Oct 5, 2017

Remove /revision/{revision} endpoint #884

Merged

Pchelolo added 4 commits November 8, 2017 13:49

Revert the order of page_restrictions rev key

a5e7a61

Decreased coupling between the filter and parsoid.js

781cf10

Rebase on master

1c0b7ca

Change-Id: I067887abf493a6fbb51ba3530443ee26c6c484c5

Adapted the restriction table to new storage

88e706a

Change-Id: I2c0db7395a5e553e4e5307d004831b98722b2b3d

Pchelolo force-pushed the revert_restriction_table branch from 7cf2766 to 88e706a Compare November 8, 2017 12:13

Rebased and reverted to old filter

f2a1f1d

Change-Id: I84e22519126bf9677ed9e17ec1d52a32227a0c20

d00rman reviewed Nov 8, 2017

View reviewed changes

d00rman added enhancement new feature and removed new feature labels Nov 9, 2017

d00rman merged commit e17288c into wikimedia:master Nov 9, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revert the order of page_restrictions rev key #718

Revert the order of page_restrictions rev key #718

Pchelolo commented Nov 18, 2016 •

edited

gwicke Nov 22, 2016

Pchelolo Nov 22, 2016

gwicke Nov 22, 2016

Pchelolo Nov 22, 2016

gwicke Nov 22, 2016

Pchelolo Nov 22, 2016

gwicke Nov 22, 2016

gwicke Nov 22, 2016

gwicke Oct 2, 2017

gwicke Nov 22, 2016

Pchelolo Nov 22, 2016

gwicke Nov 22, 2016

Pchelolo Nov 22, 2016

Pchelolo Nov 22, 2016

gwicke Nov 22, 2016

Pchelolo Nov 22, 2016 •

edited

gwicke commented Nov 22, 2016

Pchelolo commented Nov 22, 2016

Pchelolo commented Sep 29, 2017

Pchelolo commented Sep 29, 2017 •

edited

Pchelolo commented Sep 29, 2017 •

edited

gwicke Oct 2, 2017

Pchelolo Oct 3, 2017

gwicke commented Oct 2, 2017

Pchelolo commented Nov 8, 2017

d00rman left a comment

d00rman Nov 8, 2017

Pchelolo Nov 8, 2017

Revert the order of page_restrictions rev key #718

Revert the order of page_restrictions rev key #718

Conversation

Pchelolo commented Nov 18, 2016 • 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

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

Choose a reason for hiding this comment

Pchelolo Nov 22, 2016 • edited

Choose a reason for hiding this comment

gwicke commented Nov 22, 2016

Pchelolo commented Nov 22, 2016

Pchelolo commented Sep 29, 2017

Pchelolo commented Sep 29, 2017 • edited

Pchelolo commented Sep 29, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gwicke commented Oct 2, 2017

Pchelolo commented Nov 8, 2017

d00rman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Pchelolo commented Nov 18, 2016 •

edited

Pchelolo Nov 22, 2016 •

edited

Pchelolo commented Sep 29, 2017 •

edited

Pchelolo commented Sep 29, 2017 •

edited