Page MenuHomePhabricator
Create Task
Maniphest T89411

Parsoid v2 API selser might not be working correctly
Closed, ResolvedPublic
Actions

Description

Request:
{
  "method": "post",
  "uri": "http://parsoid-lb.eqiad.wikimedia.org/v2/en.wikipedia.test.local/wt/User%3AGWicke%2F_restbase_test/646859921",
  "headers": {
    "content-type": "application/json"
  },
  "body": {
    "original": {
      "html": {
        "headers": {
          "content-type": "text/html;profile=mediawiki.org/specs/html/1.0.0"
        },
        "body": "<!DOCTYPE html>\n<html prefix=\"dc: http://purl.org/dc/terms/ mw: http://mediawiki.org/rdf/\" about=\"http://en.wikipedia.org/wiki/Special:Redirect/revision/646859921\"><head prefix=\"mwr: http://en.wikipedia.org/wiki/Special:Redirect/\"><meta property=\"mw:articleNamespace\" content=\"2\"/><link rel=\"dc:replaces\" resource=\"mwr:revision/0\"/><meta property=\"dc:modified\" content=\"2015-02-12T22:30:30.000Z\"/><meta about=\"mwr:user/11429869\" property=\"dc:title\" content=\"GWicke\"/><link rel=\"dc:contributor\" resource=\"mwr:user/11429869\"/><meta property=\"mw:revisionSHA1\" content=\"6417e5e59b2975e65eebb5104ea572913a61db7e\"/><meta property=\"dc:description\" content=\"selser test page\"/><meta property=\"mw:parsoidVersion\" content=\"0\"/><link rel=\"dc:isVersionOf\" href=\"//en.wikipedia.org/wiki/User%3AGWicke/_restbase_test\"/><title>User:GWicke/_restbase_test</title><base href=\"//en.wikipedia.org/wiki/\"/><link rel=\"stylesheet\" href=\"//en.wikipedia.org/w/load.php?modules=mediawiki.legacy.commonPrint,shared|mediawiki.skinning.elements|mediawiki.skinning.content|mediawiki.skinning.interface|skins.vector.styles|site|mediawiki.skinning.content.parsoid&amp;only=styles&amp;skin=vector\"/></head><body id=\"mwAA\" lang=\"en\" class=\"mw-content-ltr sitedir-ltr ltr mw-body mw-body-content mediawiki\" dir=\"ltr\"><div id=\"bar\">Selser test</div></body></html>"
      },
      "data-parsoid": {
        "headers": {
          "content-type": "application/json;profile=mediawiki.org/specs/data-parsoid/0.0.1"
        },
        "body": {
          "counter": 0,
          "ids": {
            "mwAA": {
              "dsr": [
                0,
                23,
                0,
                0
              ]
            },
            "bar": {
              "stx": "html",
              "autoInsertedEnd": true,
              "dsr": [
                0,
                23,
                12,
                0
              ]
            }
          }
        }
      },
      "revid": "646859921"
    },
    "html": "<!DOCTYPE html>\n<html prefix=\"dc: http://purl.org/dc/terms/ mw: http://mediawiki.org/rdf/\" about=\"http://en.wikipedia.org/wiki/Special:Redirect/revision/646859921\"><head prefix=\"mwr: http://en.wikipedia.org/wiki/Special:Redirect/\"><meta property=\"mw:articleNamespace\" content=\"2\"/><link rel=\"dc:replaces\" resource=\"mwr:revision/0\"/><meta property=\"dc:modified\" content=\"2015-02-12T22:30:30.000Z\"/><meta about=\"mwr:user/11429869\" property=\"dc:title\" content=\"GWicke\"/><link rel=\"dc:contributor\" resource=\"mwr:user/11429869\"/><meta property=\"mw:revisionSHA1\" content=\"6417e5e59b2975e65eebb5104ea572913a61db7e\"/><meta property=\"dc:description\" content=\"selser test page\"/><meta property=\"mw:parsoidVersion\" content=\"0\"/><link rel=\"dc:isVersionOf\" href=\"//en.wikipedia.org/wiki/User%3AGWicke/_restbase_test\"/><title>User:GWicke/_restbase_test</title><base href=\"//en.wikipedia.org/wiki/\"/><link rel=\"stylesheet\" href=\"//en.wikipedia.org/w/load.php?modules=mediawiki.legacy.commonPrint,shared|mediawiki.skinning.elements|mediawiki.skinning.content|mediawiki.skinning.interface|skins.vector.styles|site|mediawiki.skinning.content.parsoid&amp;only=styles&amp;skin=vector\"/></head><body id=\"mwAA\" lang=\"en\" class=\"mw-content-ltr sitedir-ltr ltr mw-body mw-body-content mediawiki\" dir=\"ltr\"><div id=\"bar\">Selser test</div></body></html>"
  }
}

Expected:
"<div id=bar>Selser test"

Result:
"<div id=\"bar\">Selser test</div>"

Parsoid clearly fell back to non-selective serialization. The original HTML & the modified HTML are identical strings.

Is there anything wrong in the request, or is this an issue in Parsoid? At first sight it seems to match the test. Although, that test looks like it doesn't actually test selser, as it is not passing an oldid.

Details

SubjectRepoBranchLines +/-
Add a v2 api test for fetching wikitext when serializingmediawiki/services/parsoidmaster+61 -9
Fetch wikitext if original dom is passed in using v2 apimediawiki/services/parsoidmaster+2 -1
Fetch wikitext if an original DOM is supplied in selser modemediawiki/services/parsoidmaster+12 -1
Fix selser in the v2 APImediawiki/services/parsoidmaster+243 -97
Customize query in gerrit

Related Objects
Search...

View Standalone Graph
This task is connected to more than 200 other tasks. Only direct parents and subtasks are shown here. Use View Standalone Graph to show more of the graph.
StatusSubtypeAssignedTask
· · ·
Resolved• GWickeT75955 RESTBase / Parsoid integration - waiting for parsoid deploy
ResolvedArlolraT89411 Parsoid v2 API selser might not be working correctly
· · ·

Event Timeline

• GWicke created this task.Feb 12 2015, 11:19 PM
• GWicke raised the priority of this task from to Needs Triage.
• GWicke updated the task description. (Show Details)
• GWicke added projects: Parsoid, RESTBase.
• GWicke added subscribers: • GWicke, Arlolra.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptFeb 12 2015, 11:19 PM
• GWicke triaged this task as High priority.Feb 12 2015, 11:20 PM
• GWicke updated the task description. (Show Details)
• GWicke set Security to None.
• GWicke updated the task description. (Show Details)
• GWicke removed a subscriber: Aklapper.
• GWicke updated the task description. (Show Details)Feb 12 2015, 11:29 PM
• GWicke updated the task description. (Show Details)
• GWicke updated the task description. (Show Details)Feb 12 2015, 11:35 PM
• GWicke updated the task description. (Show Details)
• GWicke mentioned this in T75955: RESTBase / Parsoid integration - waiting for parsoid deploy.Feb 12 2015, 11:49 PM
• GWicke added a parent task: T75955: RESTBase / Parsoid integration - waiting for parsoid deploy.
Arlolra claimed this task.Feb 13 2015, 1:00 AM
• GWicke mentioned this in T88319: Replicate Parsoid v1 API functionality in RESTBase.Feb 13 2015, 1:05 AM
Arlolra added a comment.Feb 13 2015, 1:29 AM

@GWicke: You aren't supplying the wikitext so presumably parsoid-lb.eqiad.wikimedia.org is making a TemplateRequest to en.wikipedia.test.local for the revision source and that's probably unreachable. Without the source, selser falls back to non-selective serialization.

Re: the test you mention. That's a good point. Some of those v2 API tests aren't actually enforcing anything. I was just using them to document the expected API. With the move to spec'ing the API out in Swagger, I should harden them up. I'll add this normalization test for selser.

• GWicke added a comment.Feb 13 2015, 1:34 AM

Ahhhh, now I see that broken domain prefix too. My fault, sorry. Let me see if fixing that gets it working.

• GWicke added a comment.Feb 13 2015, 2:26 AM

With https://github.com/gwicke/restbase/commit/481e19401a1cc10ebf335a168cb39f59ca439bee, it still does not seem to work:

{
  "uri": "http://parsoid-lb.eqiad.wikimedia.org/v2/en.wikipedia.org/wt/User%3AGWicke%2F_restbase_test/646859921",
  "headers": {
    "content-type": "application/json"
  },
  "body": {
    "original": {
      "html": {
        "headers": {
          "content-type": "text/html;profile=mediawiki.org/specs/html/1.0.0"
        },
        "body": "<!DOCTYPE html>\n<html prefix=\"dc: http://purl.org/dc/terms/ mw: http://mediawiki.org/rdf/\" about=\"http://en.wikipedia.org/wiki/Special:Redirect/revision/646859921\"><head prefix=\"mwr: http://en.wikipedia.org/wiki/Special:Redirect/\"><meta property=\"mw:articleNamespace\" content=\"2\"/><link rel=\"dc:replaces\" resource=\"mwr:revision/0\"/><meta property=\"dc:modified\" content=\"2015-02-12T22:30:30.000Z\"/><meta about=\"mwr:user/11429869\" property=\"dc:title\" content=\"GWicke\"/><link rel=\"dc:contributor\" resource=\"mwr:user/11429869\"/><meta property=\"mw:revisionSHA1\" content=\"6417e5e59b2975e65eebb5104ea572913a61db7e\"/><meta property=\"dc:description\" content=\"selser test page\"/><meta property=\"mw:parsoidVersion\" content=\"0\"/><link rel=\"dc:isVersionOf\" href=\"//en.wikipedia.org/wiki/User%3AGWicke/_restbase_test\"/><title>User:GWicke/_restbase_test</title><base href=\"//en.wikipedia.org/wiki/\"/><link rel=\"stylesheet\" href=\"//en.wikipedia.org/w/load.php?modules=mediawiki.legacy.commonPrint,shared|mediawiki.skinning.elements|mediawiki.skinning.content|mediawiki.skinning.interface|skins.vector.styles|site|mediawiki.skinning.content.parsoid&amp;only=styles&amp;skin=vector\"/></head><body id=\"mwAA\" lang=\"en\" class=\"mw-content-ltr sitedir-ltr ltr mw-body mw-body-content mediawiki\" dir=\"ltr\"><div id=\"bar\">Selser test</div></body></html>"
      },
      "data-parsoid": {
        "headers": {
          "content-type": "application/json;profile=mediawiki.org/specs/data-parsoid/0.0.1"
        },
        "body": {
          "counter": 0,
          "ids": {
            "mwAA": {
              "dsr": [
                0,
                23,
                0,
                0
              ]
            },
            "bar": {
              "stx": "html",
              "autoInsertedEnd": true,
              "dsr": [
                0,
                23,
                12,
                0
              ]
            }
          }
        }
      },
      "revid": "646859921"
    },
    "html": "<!DOCTYPE html>\n<html prefix=\"dc: http://purl.org/dc/terms/ mw: http://mediawiki.org/rdf/\" about=\"http://en.wikipedia.org/wiki/Special:Redirect/revision/646859921\"><head prefix=\"mwr: http://en.wikipedia.org/wiki/Special:Redirect/\"><meta property=\"mw:articleNamespace\" content=\"2\"/><link rel=\"dc:replaces\" resource=\"mwr:revision/0\"/><meta property=\"dc:modified\" content=\"2015-02-12T22:30:30.000Z\"/><meta about=\"mwr:user/11429869\" property=\"dc:title\" content=\"GWicke\"/><link rel=\"dc:contributor\" resource=\"mwr:user/11429869\"/><meta property=\"mw:revisionSHA1\" content=\"6417e5e59b2975e65eebb5104ea572913a61db7e\"/><meta property=\"dc:description\" content=\"selser test page\"/><meta property=\"mw:parsoidVersion\" content=\"0\"/><link rel=\"dc:isVersionOf\" href=\"//en.wikipedia.org/wiki/User%3AGWicke/_restbase_test\"/><title>User:GWicke/_restbase_test</title><base href=\"//en.wikipedia.org/wiki/\"/><link rel=\"stylesheet\" href=\"//en.wikipedia.org/w/load.php?modules=mediawiki.legacy.commonPrint,shared|mediawiki.skinning.elements|mediawiki.skinning.content|mediawiki.skinning.interface|skins.vector.styles|site|mediawiki.skinning.content.parsoid&amp;only=styles&amp;skin=vector\"/></head><body id=\"mwAA\" lang=\"en\" class=\"mw-content-ltr sitedir-ltr ltr mw-body mw-body-content mediawiki\" dir=\"ltr\"><div id=\"bar\">Selser test</div></body></html>"
  }
}

Expected:
"<div id=bar>Selser test"

Result:
"<div id=\"bar\">Selser test</div>"

Is there another issue with the request?

gerritbot added a project: Patch-For-Review.Feb 13 2015, 3:45 AM
gerritbot subscribed.

Change 190410 had a related patch set uploaded (by Arlolra):
Fix selser in the v2 API

https://gerrit.wikimedia.org/r/190410

Patch-For-Review

Arlolra added a comment.Feb 13 2015, 3:54 AM

GWicke: There was a bug in there (see the attached patch) but I would have expected the current result to be:

<div id=\"bar\">Selser test

because you've got { "stx": "html", "autoInsertedEnd": true } in there, which the non-selective serializer should respect.

Arlolra added a comment.Feb 13 2015, 3:58 AM

Oh, wait, nevermind. I was looking at the original data-parsoid. Not the new one, which doesn't exist. In that case, selser will assume they're all new nodes (no data-parsoid on them) and just serialize as such.

Arlolra added a comment.Feb 13 2015, 4:10 AM

I suspect you'e about to say that data-parsoid won't change from the original and should be applied to the new html as well. And that this test that accepts a pagebundle doesn't seem right:

https://github.com/wikimedia/parsoid/blob/master/tests/mocha/api.js#L293-L320

But how do you distinguish between when to apply the original to the new, and when the new has the data-parsoid inlined?

• GWicke added a comment.Feb 13 2015, 4:14 AM

But how do you distinguish between when to apply the original to the new, and when the new has the data-parsoid inlined?

Can you expand on this a bit? Is your concern somebody adding ids to elements that then pull in data-parsoid that originally belonged to another node?

Arlolra added a comment.Feb 13 2015, 4:27 AM

Can you expand on this a bit? Is your concern somebody adding ids to elements that then pull in data-parsoid that originally belonged to another node?

Ha. And I'm also concerned that someone requested a pagebundle in the first place in that scenario.

Moving on.

Should we require that data-parsoid always be part of original in the post then?

• GWicke added a comment.EditedFeb 13 2015, 4:32 AM
In T89411#1036036, @Arlolra wrote:

Should we require that data-parsoid always be part of original in the post then?

I would say that if there is an original in the request, then data-parsoid should be used from it. Do you see a use case for supporting inline data-parsoid beyond a transition period at all? If there isn't one, then it would basically boil down to what you say.

Arlolra added a comment.Feb 13 2015, 4:43 AM

I'm not sure about long-term inline data-parsoid support.

My question was about this test,
https://github.com/wikimedia/parsoid/blob/master/tests/mocha/api.js#L293-L320

Should we allow a post like that? Or should data-parsoid always be child of original?

• GWicke added a comment.Feb 13 2015, 4:52 AM
In T89411#1036045, @Arlolra wrote:

Should we allow a post like that? Or should data-parsoid always be child of original?

Ah, now I get it. I don't really see a use case for passing in a modified data-parsoid, except maybe parsoids playing ping-pong? IMHO we also shouldn't encourage clients that aren't storage services like parsoid to work directly with pagebundles. It just encourages people to mess with private information when they shouldn't.

Arlolra added a comment.Feb 13 2015, 5:00 AM

Sounds good. I'll update the patch or follow up with another to fix this.

• GWicke added a comment.Feb 13 2015, 5:29 AM

@Arlolra: Awesome! Thank you (as always) for your swift and thorough response.

gerritbot added a comment.Feb 13 2015, 9:08 PM

Change 190410 merged by jenkins-bot:
Fix selser in the v2 API

https://gerrit.wikimedia.org/r/190410

Arlolra closed this task as Resolved.Feb 13 2015, 9:13 PM

Guess you're just waiting on the Monday deploy now :)

• GWicke added a comment.EditedFeb 13 2015, 11:02 PM

When testing against the latest patch (b2c16b3628) I'm now getting something that looks like it's still not using selser:

"<div id=\"bar\">Selser test"

Those quotes shouldn't be there if selser was used.

• GWicke added a comment.EditedFeb 13 2015, 11:46 PM

It turns out that I need to also set fetchWT to true in the config to enable wikitext fetching for selser. The default value is off, and the documentation states:

/**
 * @property {boolean} fetchWT
 * When transforming from html to wt, fetch the original wikitext before.
 * Intended for use in round-trip testing.
 */
 ParsoidConfig.prototype.fetchWT = false;

This flag is not part of the default config file.

It is also not set in our production config.

How is this going to work in production?

gerritbot added a comment.Feb 14 2015, 12:00 AM

Change 190601 had a related patch set uploaded (by GWicke):
Fetch wikitext if an original DOM is supplied in selser mode

https://gerrit.wikimedia.org/r/190601

Patch-For-Review

gerritbot added a comment.Feb 14 2015, 3:08 AM

Change 190632 had a related patch set uploaded (by GWicke):
Fetch wikitext if original dom is passed in using v2 api

https://gerrit.wikimedia.org/r/190632

Patch-For-Review

gerritbot added a comment.Feb 14 2015, 3:10 AM

Change 190601 abandoned by GWicke:
Fetch wikitext if an original DOM is supplied in selser mode

Reason:
Better solution in https://gerrit.wikimedia.org/r/#/c/190632/

https://gerrit.wikimedia.org/r/190601

• GWicke reopened this task as Open.Feb 14 2015, 6:09 PM

Re-opened to reflect that the patch is not yet merged & deployed. Will close once that's the case.

gerritbot added a comment.Feb 14 2015, 6:21 PM

Change 190632 merged by jenkins-bot:
Fetch wikitext if original dom is passed in using v2 api

https://gerrit.wikimedia.org/r/190632

• GWicke renamed this task from Parsoid v2 API selser might not be working correctly to Parsoid v2 API selser might not be working correctly: fix in master, to be deployed.Feb 14 2015, 8:34 PM
ssastry renamed this task from Parsoid v2 API selser might not be working correctly: fix in master, to be deployed to Parsoid v2 API selser might not be working correctly.Feb 16 2015, 9:20 PM
ssastry closed this task as Resolved.
ssastry removed a project: Patch-For-Review.
gerritbot added a comment.Feb 19 2015, 7:33 PM

Change 191674 had a related patch set uploaded (by Arlolra):
Add a v2 api test for fetching wikitext when serializing

https://gerrit.wikimedia.org/r/191674

Patch-For-Review

gerritbot added a comment.Feb 19 2015, 9:31 PM

Change 191674 merged by jenkins-bot:
Add a v2 api test for fetching wikitext when serializing

https://gerrit.wikimedia.org/r/191674

Arlolra mentioned this in rGPARb2c16b3628d5: Fix selser in the v2 API.Apr 8 2015, 4:28 PM
• GWicke mentioned this in rGPARf5b5478188d0: Fetch wikitext if original dom is passed in using v2 api.Apr 8 2015, 4:28 PM
ssastry mentioned this in rGPAR4d4621990f80: Add a v2 api test for fetching wikitext when serializing.Apr 8 2015, 4:28 PM
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL