@Catrope, @TrevorParscal, @Esanders, @Jdforrester-WMF: We are getting ready to start work on this. It would be great to get your input on the API design. Does the changeset representation make sense to you? Do you prefer replace semantics only?

How would this look with multiple sections inserted at a time. In terms of performance, we've already thrown away the majority of the data, so I'm not too worried about including an extra paragraph. Ultimately I think it would be better if the API was simple to implement.

@Esanders, agreed on simplicity and limited impact of including the original paragraph / section. Multiple inserted sections would just be multiple sections in the replacement content.

We discussed this on IRC. Outcomes:

• Use simple replace semantics for changesets. Simplicity is more important than a micro-optimization for the relatively rare append case.
• Determine a magic ID for replacing the entire body (for edits on empty content). Candidates: mw0, mw_body; Requirements:
  • Cannot be in the body, even if users supply it
  • Needs to be a valid JSON key

Quick update:

• As of last week, Parsoid now emits offsets of top-level sections keyed on id in data-parsoid. This lets us use cheap substring operations to get a top-level section in RESTBase, with the main cost being data-parsoid loading (~10ms) and JSON parsing (~16ms).
• An alternative for finer grained section / content retrieval (potentially useful for use cases like Labeled Section Transclusion (T100139)) would be to parse the HTML. Timings for Obama (1.1M):
  • SAX parse via libxmljs (node) and no-op handlers: 64ms
  • XML DOM parse via libxmljs (node): 16ms
    • XPATH match for ID (ex: dom.find('//*[@id = "mw123"]')) : 15ms
    • XPATH match for class (ex: dom.find("//*[contains(concat(' ', normalize-space(@class), ' '), ' interlanguage-link ')]"): 34ms
  • HTML5 DOM parse via Mozilla's html5ever: 32ms
    • full round-trip with serialization: 60ms
  • HTML5 DOM parse via domino (node): 220ms

Patch implementing the section retrieval portion at https://github.com/wikimedia/restbase/pull/250

@mobrovac also implemented a wikitext save end point internally. The main missing bit is html to wikitext conversion with individual modified sections:

{
  "mwXXOriginalId": "<p id='mwXXOriginalId'>Modified content</p><p>Possibly new content</p>",
  "mwXXOtherId": "<p>Some content</p><p id='mwXXOtherId'>Possibly unchanged original content for element</p>"
}

Fast (as in latency) implementation option:

• Serialize each modified section to wikitext. For this, we need to extract the original corresponding HTML and pass it to Parsoid, along with data-parsoid for the entire page.
• Use the wikitext offsets for the remaining sections (sorted by offset) to combine the original wikitext of the revision with the serialized sections, and return it.

Simpler implementation option:

• Replace each modified section in the original HTML (from storage), using the HTML section offset information
• serialize the modified full HTML, original full HTML and data-parsoid as for a regular /transform/html/to/wikitext request, and return the result

Created a PR for html->wikitext translation with sections: https://github.com/wikimedia/restbase/pull/270

In T94890#1410095, @Pchelolo wrote:

Created a PR for html->wikitext translation with sections: https://github.com/wikimedia/restbase/pull/270

The PR has been merged. Let's keep this ticket open until we actually deploy it.

The fix has been deployed. Closing.

A small utility function to simplify working with this section API in full-document edit applications like VE is available at https://gist.github.com/gwicke/f012b257e74223055065fe23a359bdc8.

If we're extending the schema, just implement the outline mode we proposed. It is completely trivial for the client and server to implement:

[
   { id: 'mwBBB' },
   { id: 'mwAAA' },
   { content: 'New' }
]

This can be computed in a single pass at convert time, by simply checking if a node has been flagged as modified. I have both server and client code written and tested end-to-end on my machine. Anything else is trying to fit a square peg in a round hole. We've already sunk far too much time into this.

Here is the small change to the RESTBase schema required to support it: https://github.com/wikimedia/restbase/pull/650/files

And the patch to the VE converter to skip over unmodified content: https://gerrit.wikimedia.org/r/#/c/303441/8/modules/ve-mw/dm/ve.dm.MWConverter.js

@Esanders: We are trying to keep our API surface small and orthogonal. Having two times two APIs (transform and save) for section editing is not very orthogonal, and complicates the API for all users.

To illustrate why small APIs matter, imagine a request from a user with a frequent need to bold an entire paragraph, and asking you to add a "bold paragraph" button. While it is clearly a win for the user to avoid the need to select the paragraph first, you might still decide not to add a second button, as the confusion for other users from having two buttons does not necessarily outweigh the relatively minor saving for the specific user.

So, if you want to make the case for a second API, then it would be helpful if you could document why the inconvenience of using a single API is so large as to outweigh the cost of adding a second API.

In T94890#2548482, @GWicke wrote:

@Esanders: We are trying to keep our API surface small and orthogonal. Having two times two APIs (transform and save) for section editing is not very orthogonal, and complicates the API for all users.

You don't have the two times two problem if you don't add two different endpoints for the two different formats. You could have one endpoint that takes either the existing replace format or Ed's outline format, provided they are easy to distinguish (array vs object is already a distinguishing property, but perhaps you'd like something more elegant).

You could have one endpoint that takes either the existing replace format or Ed's outline format, provided they are easy to distinguish (array vs object is already a distinguishing property, but perhaps you'd like something more elegant).

This doesn't avoid the problem of needing to document two different APIs, with different request formats. Trying to do this in a single entry point tends to be harder than in two separate ones. For example, swagger-ui can display example requests, but this only works well for a single request format.

Getting it to work with the documentation tool of the day is a relatively minor issue. I'm sure we can work around that.

@Esanders: Paths are not what is expensive here. The expensive part is documenting two different request formats with overlapping use cases, as well as when to use each of them.

That is not expensive. What's expensive is spending a whole week trying to shoe horn our use case into an exsiting API that was designed for something else - for the sake of having marginally simpler documentation. We've already sunk far too many person hours into this. Let's just merge our code and move on - VE (and I'm sure Services) have far more important things to work on.

@Esanders: Clearly we have different perspectives and priorities. I am trying to figure out which underlying problems you are trying to solve & how we can fit them into the larger API. You have a specific solution in mind, and are reluctant to describe the underlying problems, and consider alternative solutions. As a result, this takes longer than necessary.

We talked about doing this by changing the API so that the replace value is no longer a string, but instead a list, each element of which is either a string or an { id: 'mwXXX' } object, like this:

{
    mwABC: [ 'foo', { id: 'mwDEF' }, 'bar', ... ],
    mwBBB: ...,
}

In this example, the prior text for section 'mwABC' is to be replaced by 'foo' + ( the prior text for section 'mwDEF' ) + 'bar' .

This means an append can be expressed as

{ mwXXX: [ { id: 'mwXXX' }, 'text to be appended' ] }

A prepend can be expressed as

{ mwXXX: [ 'text to be prepended', { id: 'mwXXX' } ] }

A move of mwYYY to before mwXXX can be expressed as

{ mwXXX: [ { id: 'mwYYY' }, { id: 'mwXXX' } ], mwYYY: [] }

A move of mwYYY to after mwXXX can be expressed as

{ mwXXX: [ { id: 'mwXXX' }, { id: 'mwYYY' } ], mwYYY: [] }

A deletion of several sections must be expressed as blanking each such section:

{ mwAAA: [], mwBBB: [], mwCCC: [], ... }

Thank you, @dchan. That's an excellent summary.

Next steps:

Services will:

• Implement array syntax & reference support in section edit API.
  • Most of us are leaning towards deprecating & removing plain string support, so only arrays would be allowed for simplicity & generality. The API is marked as unstable, but we are not aware of any actual section API users, so this should not affect anybody. Input on whether to remove plain strings would be appreciated.
• Update [the sectionDiff utility function](https://gist.github.com/gwicke/f012b257e74223055065fe23a359bdc8) to emit this format, given
  • an array of original section ids, and
  • an array of new section ids / strings: [{id: 'a'}, 'foo', {id: 'c'}]

Once this is ready, the function can be hooked up in VisualEditor, and saves will only transmit changed content over the wire.

In T94890#2562739, @GWicke wrote:

• an array of new section ids / strings: [{id: 'a'}, 'foo', {id: 'c'}]

And to clarify, the IDs can be reordered, so [{id: 'b'}, {id: 'a'}] is valid.

To clarify - what happens if one of the sections from the old document is not present as a key in the section replacement object? Is it preserved or removed?

@Pchelolo, it is removed. The semantics are "replace original section xyz with this new content".

T143356 discusses how section editing could cleanly support separate data-mw retrieval / editing. Please have a look, especially at the last comments.

Concretely, with data-mw support section edit requests would look like this:

{
  mwABC: {
    html: ['Foo', { id: 'mwABC' }],
    "data-mw": { /* data-mw for new content, or empty object if all inline */ }
  }
}

@Esanders, @dchan: Any objections from your side against moving straight to the { html: [ ... ] } format, so that we can support separate data-mw as well?

I had a brief in-person chat about moving straight to { html: [ ... ] } with @Esanders, and he saw no issue with doing so from the VE perspective. Now waiting for the Parsoid team feedback on T143356 before we can tackle the public section API change.

BTW, as we're changing the API in the backwards incompatible way, let's also tackle T116336 and accept application/json instead of weird formData and json combinations

Here we go: https://github.com/wikimedia/restbase/pull/655

I'm working on the client-side code at https://github.com/gwicke/parsoid-dom-utils/blob/master/lib/section_diff.js.

We also decided to vary the section replacement syntax slightly, using an array again: T143356#2580945

Let us know if you have concerns about this change.

In T94890#2581012, @GWicke wrote:

I'm working on the client-side code at https://github.com/gwicke/parsoid-dom-utils/blob/master/lib/section_diff.js.

This is now ready.

The VE effort to speed up saves using this section edit API is tracked at T142320: Allow client to send back empty top-level sections if they are unchanged.

In T94890#2600697, @Esanders wrote:

Per IRL discussion - we either need VirtualRestService support for the JSON API, or we need the new API to support form data encoding.

PR for supporting both: https://github.com/wikimedia/hyperswitch/pull/59

Personally I don't think it makes sense to have just one of the APIs be JSON when the rest are form encoded, just because the main parameter is a JSON string. It means that identical parameters, e.g. scrub_wikitext, are passed in different ways between two endpoints, which I think will be confusing for future clients.

AFAICT the issue is with documentation/validation, but we shouldn't break our API because of the limitations of our current tools.

Change 308014 had a related patch set uploaded (by Ppchelko):
MultiHTTPClient: Support sending JSON POST requests.

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

Hm, my thinking is really moving back and forth on this.. On one hand it's weird to have JSON embedded inside form-encoded param. But on the other hand, when we introduce separate data-mw field to the other endpoints - we will eventually have the same problem anyway since data-mw is JSON and we're not going to change other endpoint accept types to JSON since they're used. @GWicke what do you think?

@Pchelolo, I commented on the more general question of JSON / form data interaction at T111748#2603225.

@GWicke So, having read that I would say that the best/fasted way to unblock VE Team is to merge my PR that would allow supplying both JSON and form-data and continue the conversation

@Pchelolo, if we go with a form-primary model, then I would propose to change the swagger spec to require a changes object containing the section diffs directly (without another changes entry inside the JSON). We wouldn't have a body parameter any more, so the hyperswitch coercion patch wouldn't be needed in its current form. JSON submission would still be supported, using the request format as documented right now.

Yesterday I've been trying a lot of options and I think that status quo plus PR https://github.com/wikimedia/hyperswitch/pull/59 is the best we can do right now without investing a lot of time in sagger-ui improvement.

With this we get validation support, more-or-less nice doc UI, support for both form-data and JSON.

When we introduce data-mw parameter to other transform endpoints we can seamlessly switch them to JSON without breaking existing clients because form-data will also be supported.

In T94890#2605417, @Pchelolo wrote:

Yesterday I've been trying a lot of options and I think that status quo plus PR https://github.com/wikimedia/hyperswitch/pull/59 is the best we can do right now without investing a lot of time in sagger-ui improvement.

With this we get validation support, more-or-less nice doc UI, support for both form-data and JSON.

When we introduce data-mw parameter to other transform endpoints we can seamlessly switch them to JSON without breaking existing clients because form-data will also be supported.

This has now been merged, and should go out next week.

@Esanders The PR fixing that is merged: https://github.com/wikimedia/restbase/pull/664 so the current RB master should work for your local testing. We will deploy it later this week as we accumulate more changes (or I can deploy it ASAP if you need)

Change 308014 abandoned by Ppchelko:
MultiHTTPClient: Support sending JSON POST requests.

Reason:
Not needed any more

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