REST: create specs.v1 module
Open, MediumPublic
Actions

Assigned To

None

Authored By

	BPirkle
	Wed, Feb 4, 10:15 PM

Description

Overview

The specs.v0 module is used to provide OpenAPI specs, which are useful to callers on their own, and which also are used by the REST Sandbox. This module has been in use in production for over a year and is currently available on all wikis.

There are two known issues with specs.v0, both (ironically) related to its own OpenAPI spec:

its OpenAPI spec has errors
its OpenAPI spec has omissions and needs improvement. Notably some of the description strings are either missing or are placeholders

Create a specs.v1 module that resolves these issues, and begin using it.

Completion Criteria

The OpenAPI spec for the specs.v1 module is complete and useful, including helpful description strings
The specs.v1 module's OpenAPI spec loads in the REST Sandbox without error
Integration tests exist for the specs.v1 module
The specs.v1 module is enabled on all wikis
All REST Sandbox urls currently pointed at the specs.v0 module are instead pointed at the specs.v1 module
The specs.v1 module is available in the REST Sandbox (so that its own endpoints are viewable/testable)
The specs.v0 module is removed from the REST Sandbox

NOTE: we will not immediately deprecate or remove the specs.v0 module. It will remain callable by anyone currently using it. A separate task should be created for its deprecation/removal process. We should consider whether externally-facing communications should be part of that.

NOTE: the recently-merged ModuleManager allows enabling MW Core modules and publishing them to the REST Sandbox without config changes. This may be useful, but care should be taken to not list both the specs.v0 and specs.v1 modules in the REST Sandbox at the same time (it wouldn't cause errors, but it would be confusing).

Regarding integration tests, most of our endpoints have a snippet similar to the following:

openApiSpec = JSON.parse( text );
chai.use( chaiResponseValidator( openApiSpec ) );

This validates that the actual endpoint behavior matches the endpoint's OpenAPI spec. The current specs.v0 module integration tests do not have this.. We should try to include it in the specs.v1 integration tests. (There's also nothing wrong with spot-checking response structure, as the specs.v0 integration tests currently do.)

Event Timeline

BPirkle created this task.Wed, Feb 4, 10:15 PM

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptWed, Feb 4, 10:15 PM

AGhirelli-WMF triaged this task as Medium priority.Thu, Feb 5, 2:55 PM

AGhirelli-WMF moved this task from Incoming (Needs Triage) to Backlog (Triaged and Ready) on the MW-Interfaces-Team board.

Some details regarding the OpenAPI spec errors in specs.v0:

the error is in the response body schema for the /discovery endpoint
that endpoint returns, among other things, a link to the discovery-1.0.json file, which exists with the MW core codebase at docs/rest/discovery-1.0.json, and which is a JSON Schema (v6) description of the /discovery endpoint response schema
response schemas for MW REST handlers are generally placed into .json files in the includes/Rest/Handler/Schema directory. These files contain json snippets suitable for direct inclusion in OpenAPI specs
these response schema files are referenced, and included in the OpenAPI spec by the Handler-derived class overriding Handler::getResponseBodySchemaFileName()
in a choice that is so meta that it makes my head hurt, DiscoveryHandler instead returns the discovery-1.0.json file as its schema from this function.
this would have been a great and clever choice, except that the discovery-1.0.json file is a full, valid, standalone JSON Schema, not a snippet suitable for inclusion in an OpenAPI spec. Trying to directly use it in the OpenAPI spec causes the errors we see in the REST Sandbox

Instead, create an appropriate .json file defining the /discovery endpoint response, place it in the includes/Rest/Handler/Schema directory, and reference that in DiscoverHandler::getResponseBodySchemaFileName(). In other words, make this handler work like all the others.

The original idea was neat, but just doesn't work the way our system is set up. Instead of what I said above, we could do something more in line with the spirit of that approach. Instead of just returning the contents of the discovery-1.0.json file, we could parse it and return the correct portions, using discovery-1.0.json as our source of truth for the endpoint response. That's what it is supposed to be for right? This would involve overriding Handler::generateResponseSpec() instead of Handler::getResponseBodySchemaFileName().

Why wouldn't we do that? Aside from the extra performance it of parsing that json inline with endpoint execution (which is probably negligible), it means that we've deferred the definition of the endpoint contract to the endpoint response. Therefore, any future changes to the endpoint response necessarily change its contract. In other words, we'd be coupling what should be two independent concerns together. By keeping them separate, we can (and should) validate via integration tests that the structure of the actual response matches the structure defined in the OpenAPI spec. This provides a protection against accidentally changing the response format and breaking callers. By coupling those, such a test would no longer have independent validity.

So, cool idea, but we should go a different direction.

BPirkle updated the task description. (Show Details)Thu, Feb 5, 3:45 PM

REST: create specs.v1 moduleOpen, MediumPublicActions

Description

Overview

Completion Criteria

Event Timeline

REST: create specs.v1 module
Open, MediumPublic
Actions