Page MenuHomePhabricator

Refinement ideas for spec templates
Closed, ResolvedPublic

Description

Our request templating functionality has become a central and useful feature of RESTBase. In the first serious uses we have found some things that could be improved, resulting in #413 and #415.

Here are some ideas following up on those PRs.

Remove the special case escape handling of the uri property

Currently, the behavior of {var} differs between "normal" attributes and the uri attribute. Inside uri, a {var} substitution will magically result in percent encoding, while in all other attributes it won't. When no escaping is desired in a uri, we currently use {+ var } instead. While this works, the inconsistent behavior of the same syntax is confusing for users.

I think it would be nice to eliminate this inconsistency and special case handling. One option to do so is to support the {{var}} form for un-escaped substitution. This would let us use the same syntax with consistent semantics in all attributes. The migration would require us changing templated references in non-uri attributes to this new form.

A side benefit of this change would be a simplification of our code. swagger-router#29 already unifies the handling of complex URI attributes with those of other attributes, but there is still a special case for the URI attribute that results in it being excluded from the compiled general request template.

Less ugly namespacing

Currently, we prefix request-global objects with $., and global (request-independent) objects with $$.. While safe and convenient for the implementation, all those dollar prefixes can be fairly ugly, especially in nested expressions. Experienced Perl wranglers seem to occasionally experience PTSD attacks when encountering those dollar signs. It is also not always obvious for users whether some helper function is in $$. or $.. Having to keep track of those two namespaces complicates the mental model for users unnecessarily.

Related observations:

  • Compact access to attribute locals is important. We use this heavily especially in uri templates. These accesses normally do not require a dotted path. In attributes like headers, we often prefer the global form where the local form is supported.
  • Parameters and sub-requests can be named anything, so it would be nice to avoid conflicts between those parameters and globals.
  • Accesses to globals fall into two groups: 1) dotted paths, and 2) function calls. Both can be distinguished syntactically.
  • There are many references to $.request.*, so shortening this name might be worthwhile.

Strawman 1: "request"

  • Add the {{var}} syntax as described in "Remove the special case escape handling of the uri property".
  • Automatically treat dotted paths & function calls as references to a global namespace. Under the hood, treat dotted paths as references to the root model, and functions calls as references to the actual global namespace.
  • Namespace response objects to a new responses object.
  • Migration: Start supporting the new behavior. Deprecate the old syntax, and possibly log a warning on startup. After a while, disable support for the old behavior.
  • Add a global alias to the global namespace, which (as in JS) lets users explicitly access the global namespace. This is useful to reference global objects without dotted path notation (ex: global.options).

Strawman 2: "req"

Like Strawman 1, but:

  • Shorten request to req.

Syntax comparison, using an example from the graphoid PR

Current syntax:

x-request-handler:
  - get_mobileapps:
      request:
        method: get
        uri: {+ $$.options.host }/{domain}/v1/page/{route}/{title}
        headers:
          x-restbase-etag: '{$.request.body.items[0].rev}/{$.request.body.items[0].tid}'
  - store:
      request:
        method: put
        uri: /{domain}/sys/key_rev_value/mobileapps.{route}/{title}/{$.request.body.items[0].rev}/{$.request.body.items[0].tid}
        headers:
          content-type: application/json
        body:
          headers: '{$$.strip($.get_mobileapps.headers, ["content-length", "content-encoding"])}'
          body: '{$.get_mobileapps.body}'
      return:
        status: 200
        headers: '{$$.strip($.get_mobileapps.headers, ["content-length", "content-encoding"])}'
        body: '{$.get_mobileapps.body}'

Strawman 1 ("request"):

x-request-handler:
  - get_mobileapps:
      request:
        method: get
        uri: {{options.host}}/{domain}/v1/page/{route}/{title}
        headers:
          x-restbase-etag: '{{request.body.items[0].rev}}/{{request.body.items[0].tid}}'
  - store:
      request:
        method: put
        uri: /{domain}/sys/key_rev_value/mobileapps.{route}/{title}/{request.body.items[0].rev}/{request.body.items[0].tid}
        headers:
          content-type: application/json
        body:
          headers: '{{strip(responses.get_mobileapps.headers, ["content-length", "content-encoding"])}}'
          body: '{{responses.get_mobileapps.body}}'
      return:
        status: 200
        headers: '{{strip(responses.get_mobileapps.headers, ["content-length", "content-encoding"])}}'
        body: '{{responses.get_mobileapps.body}}'

Strawman 2 ("req"):

x-request-handler:
  - get_mobileapps:
      request:
        method: get
        uri: {{options.host}}/{domain}/v1/page/{route}/{title}
        headers:
          x-restbase-etag: '{{req.body.items[0].rev}}/{{req.body.items[0].tid}}'
  - store:
      request:
        method: put
        uri: /{domain}/sys/key_rev_value/mobileapps.{route}/{title}/{req.body.items[0].rev}/{req.body.items[0].tid}
        headers:
          content-type: application/json
        body:
          headers: '{{strip(responses.get_mobileapps.headers, ["content-length", "content-encoding"])}}'
          body: '{{responses.get_mobileapps.body}}'
      return:
        status: 200
        headers: '{{strip(responses.get_mobileapps.headers, ["content-length", "content-encoding"])}}'
        body: '{{responses.get_mobileapps.body}}'

Voting

Strawman 1 ("request"): @Eevans, @Yurik
Strawman 2 ("req"): @GWicke

Event Timeline

GWicke raised the priority of this task from to Needs Triage.
GWicke updated the task description. (Show Details)
GWicke added a project: RESTBase.
GWicke subscribed.
GWicke set Security to None.
GWicke added a project: Services.
GWicke edited subscribers, added: Services; removed: Aklapper.
GWicke updated the task description. (Show Details)

Support for un-prefixed global references is now implemented in https://github.com/wikimedia/swagger-router/pull/30, and other parts in https://github.com/wikimedia/restbase/pull/417. This patch is currently supporting both request and req, but if we decide to not support req that's two lines to remove.

hm... honestly I've not too much opinion here. Just whatever you guys think is friendly. When using a system like this that I don't know I appreciate two things:

  1. documentation with clear examples
  2. a way to automatically validate the spec to know whether or not it works. This one's important and, from what I understand, difficult. But important nonetheless for people like me who are just looking to copy paste some examples, test to make sure it works, test manually a little bit and move on to something else.

a way to automatically validate the spec to know whether or not it works

Beyond syntax checking correctness is indeed hard to guarantee statically. However, we do have x-ample tests that can make it fairly easy to run actual tests by declaring request / response pairs. They are also optionally picked up by our automated service monitoring scripts.

I have been thinking a bit about error handling for {{foo}} vs. {+foo}. Originally, I thought that it would be good to require {{foo}} to return a value that's not undefined, and throw an exception if that's not the case. Now, I'm no longer so sure that this would be a good trade-off between situations where we want things to be optional, vs. where we don't.

Required result + {+optional}:

# really want options.host to be defined here.
uri: '{{options.host}}/{domain}/'
headers:
  content-type: '{{content-type}}'
  cache-control: '{+cache-control}'
  x-optional: '{+req.body.option}'

Optional result + new {{!required}} / {!required} syntax:

# really want options.host to be defined here.
uri: '{{!options.host}}/{!domain}/'
headers:
  content-type: '{{!content-type}}'
  cache-control: '{{cache-control}}'
  x-optional: '{{req.body.option}}'

Required result + new {{?optional}} syntax:

# really want options.host to be defined here.
uri: '{{options.host}}/{domain}/'
headers:
  content-type: '{{content-type}}'
  cache-control: '{{?cache-control}}'
  x-optional: '{{?req.body.option}}'

The current implementation accepts undefined returns (and replaces undefined string segments with the empty string), but does not swallow exceptions.

I think that we've settled with our templating syntax and I wanna close this task. @GWicke what do you think

GWicke claimed this task.

@Pchelolo, agreed. There is still a PR discussing further refinements at https://github.com/wikimedia/swagger-router/pull/32, but the concrete changes discussed here are done.