Page MenuHomePhabricator

[RFC] Consider some tweaks to the section transform API
Closed, DuplicatePublic

Description

Currently we are using a mix of JSON and post parameters in the section transform API. An example request would send a mix of form data and JSON like this:

sections: '{"mwAg":"<h2>First Section replaced</h2><h2>Appended Section</h2>"}'

This is a messy approach for validation and processing.

I think we should instead move to a JSON focused spec, using a JSON schema to describe the request body. We could still support POST-based requests using the mapping schema described in T111748.

The upside of moving to a JSON spec is the avoidance of special cases, and clean validation support. The downside is the loss of per-key forms in the documentation, although arguably a single JSON form field might actually be less confusing than the current mix of plain-string and JSON form fields.

Event Timeline

GWicke raised the priority of this task from to Needs Triage.
GWicke updated the task description. (Show Details)
GWicke subscribed.
GWicke set Security to None.
GWicke added a project: RESTBase-API.
GWicke updated the task description. (Show Details)
GWicke updated the task description. (Show Details)
GWicke removed a subscriber: Aklapper.

I support moving to JSON, which is already implicitly supported in RESTBase anyway. As for the validation, since the sections stanza is an object with arbitrary keys, all we can validate here is that if the sections stanza is present, it is indeed an object with one or more keys. But that should be enough to reject ill-formatted requests.

Actually, since sectionIDs are not completely arbitrary, but follow a specified pattern, we could do even more:

schema:
  type: 'object'
  properties:
    sections:
      type: 'object'
      patternProperties:
        '^mw\w+$':
          type: 'string'
      additionalProperties: false
  additionalProperties: false
  required: 
    - sections

However, the validation error message, produced by the schema validator (which we don't control) could be a bit confusing in case section ID doesn't match a pattern:

data.body.sections['asdcmwAg'] should NOT have additional properties

Anyway, all of this would be possible only when we deprecate and remove form-data support.

GWicke triaged this task as Medium priority.Nov 12 2015, 4:06 AM

I'm all for that, but shouldn't we make an announcement, log users etc. prior to the change?

For this API, losing the POST parameter form fields in the swagger docs shouldn't be an issue. This could be an issue for the more general T111748, though. In particular, HTML transform forms currently provide a text entry field, which is useful for testing & quick ad-hoc conversions.

@GWicke ye, but html transform endpoint doesn't need changes since the value of the POST parameter there is string, not JSON object.