Mechanism for storing and retrieving Parsoid HTML from JS and PHP #1179

hknustwmf · 2019-08-23T21:19:14Z

It may look like a lot of changes, but there are actually only a few. The key pieces are parsoid.js and the new test variant_config.js. The transform.js test battery is now executed twice -- once for each variant. The tests themselves did not change except for the headers.

Pchelolo · 2019-08-23T21:37:21Z

I haven't looked in any of the details, but I strongly disagree with the implementation approach taken here.

Touching parsoid.js in this manner was never intended and doing so creates so much more complexity... Here's what needs to be done in my opinion:

We need to mount sys/parsoid.js in project twice under 2 separate paths with 2 separate options - one for JS and one for PHP
Instead of mounting /v1/parsoid.yaml we need to write a little JS proxy module that would look at the cookie and decide which parsoid version (PHP vs JS) to use, call it, and add a Vary header in the end - that way all the code related to this transition will be confined to a single place and not spread around the codebase. This change will be removed after full transition anyway, so we want it to be as self-confined as possible
I don't understand the purpose of one more copy of the config. What we need to do is to have tests running for JS parsoid-only, for PHP parsoid-only and maybe one or two tests verifying that actual switching works. For the purpose of the first two, we can add support in preq to set additional headers based on an environment variable and add one more dimention in the travis testing matrix. For the second - we just write a couple of new tests in a separate suite.

d00rman · 2019-08-26T12:21:04Z

Touching parsoid.js in this manner was never intended and doing so creates so much more complexity... Here's what needs to be done in my opinion:

I agree. Too many places in parsoid.js have been touched just for the transition period. I agree with @Pchelolo on the course of action outlined below. Some comments follow.

We need to mount sys/parsoid.js in project twice under 2 separate paths with 2 separate options - one for JS and one for PHP

Instead of mounting /v1/parsoid.yaml we need to write a little JS proxy module that would look at the cookie and decide which parsoid version (PHP vs JS) to use, call it, and add a Vary header in the end - that way all the code related to this transition will be confined to a single place and not spread around the codebase. This change will be removed after full transition anyway, so we want it to be as self-confined as possible

I agree this is the way to go, but there are some caveats. These could be mounted onto /sys/parsoid/ and /sys/parsoidphp/. In the mount options, then, one would get the Parsoid/JS host supplied for parsoidHost, while the other would get the Parsoid/PHP host. Additionally, each would need an extra option passed to it, bucket_name (or similar) that instructs it which buckets to use (the values are parsoid and parsoidphp respectively).

I don't understand the purpose of one more copy of the config. What we need to do is to have tests running for JS parsoid-only, for PHP parsoid-only and maybe one or two tests verifying that actual switching works. For the purpose of the first two, we can add support in preq to set additional headers based on an environment variable and add one more dimention in the travis testing matrix. For the second - we just write a couple of new tests in a separate suite.

I think we can tackle the tests after the first two points have been completed (but still in this PR).

d00rman

Needs to be reworked as per above discussion.

hknustwmf · 2019-09-04T03:18:51Z

Needs to be reworked as per above discussion.

I added the proxy and a couple basic tests.

d00rman

Gave it a first pass, but looking much better now.

projects/sys/default.wmf.yaml

config.fullstack.test.yaml

sys/parsoid.js

sys/parsoid_proxy.js

projects/sys/default.wmf.yaml

sys/parsoid_proxy.js

projects/example.yaml

sys/parsoid_proxy.js

Pchelolo · 2019-09-05T18:01:29Z

sys/parsoid_proxy.js

+                .then((res) => {
+                    if (res) {
+                        res.headers = res.headers || {};
+                        res.headers[VARIANT_HDR_NAME] = rootReqHeaders[VARIANT_HDR_NAME];


I'm not quite sure this is a right behavior that wouldn't accidentally mask issues. We assume we've routed the request to correct Parsoid, but who knows? Why would we set the response header for parsed variant here??

It's in the ticket: "Furthermore, RESTBase must include this header in the response, ..." https://phabricator.wikimedia.org/T230791

Ideally, the variant header would be added by the Parsoid instances instead of Restbase

Yeah, that's what I meant. Or, the content-type would be different. Would you mind filing a ticket, something like 'Parsoid JS/PHP should indicate which Parsoid implementation the page was rendered with'?

Pchelolo · 2019-09-05T18:06:19Z

A couple of issues left, but you're close.

One thing I'm wondering about is testing. With the current patch we will be testing PHP parsoid compatibility with exactly 1 test. We need more. We need to have an env variable that would set a parsoid variant header in preq for all requests, (I believe we'd need to support this in preq, but maybe request already supports something like that. Needs to be checked). Then we need to add 2 versions of this variable to the test matrix in travis.yaml - that would allow us to run all the tests with both parsoid versions and be sure we're compatible with both.

hknustwmf

Doesn't look like request has support for what you want to do. Do you intend to modify the tests with the additional header field? The value would be whatever the env var says or JS as the default?

…id HTML from JS and PHP

Pchelolo

Doesn't look like request has support for what you want to do. Do you intend to modify the tests with the additional header field? The value would be whatever the env var says or JS as the default?

There's https://github.com/request/request#requestdefaultsoptions. We can add this to 'preq' as well - preq.defaults will return a wrapper that will modify the request options. Should be fairly easy to achieve. Then we can modify the tests to require some instance of preq from utils and that one would be a wrapper based on an env var.

I've realized we can't really add the header based on the env var in preq itself cause we use preq not only in test code but in the application code as well.

Pchelolo · 2019-10-03T20:48:57Z

This needs to be manually rebased.
Now that we have parsoid in beta accessible, we can pick up the work here again. See T231569 for reference regarding parsoid in beta cluster.

There's a bunch of things to be done to be able to run CI and local testing agains Parsoid-PHP. We can't access production wikis on beta PHP Parsoid, see T231933 so we need to convert all the tests to only talk to beta wikis. I think we can work on that in separate PRs.

hknustwmf · 2019-10-04T18:59:17Z

Based on the discussion with Giuseppe yesterday, let's also think about/create tasks to remove external dependencies from integration tests.

Pchelolo · 2019-10-04T19:06:17Z

Based on the discussion with Giuseppe yesterday, let's also think about/create tasks to remove external dependencies from integration tests.

Creating tasks makes sense, but actually blocking this on them is a bit too ambitious in my opinion.

hknustwmf · 2019-10-04T19:17:58Z

It was meant as a suggestion not a blocker

d00rman · 2019-10-13T09:11:08Z

Superseded by #1207

hknustwmf requested a review from d00rman August 23, 2019 21:26

d00rman added the new feature label Aug 26, 2019

d00rman suggested changes Aug 26, 2019

View reviewed changes

d00rman changed the title ~~T230791 - Have a Mechanism for Storing and Retrieving Parsoid HTML from JS and PHP~~ Mechanism for storing and retrieving Parsoid HTML from JS and PHP Aug 26, 2019

hknustwmf force-pushed the T230791 branch from 4142764 to 511f2ed Compare September 4, 2019 02:15

d00rman suggested changes Sep 4, 2019

View reviewed changes

hknustwmf force-pushed the T230791 branch from 511f2ed to d055b63 Compare September 4, 2019 13:44

Pchelolo reviewed Sep 4, 2019

View reviewed changes

sys/parsoid_proxy.js Outdated Show resolved Hide resolved

hknustwmf force-pushed the T230791 branch 2 times, most recently from 5d61671 to 9f86111 Compare September 4, 2019 14:42