Generate a detailed and accurate OpenAPI spec from the backend - #2432
Draft
correct-horse-battery-bench wants to merge 13 commits into
Conversation
Extract the APIv2 OpenAPI spec generation out of the inline route closure into a dedicated library (src/inc/apiv2/openapi/), add ApiRegistry as the single source of truth for APIv2 class registration, and make a set of targeted corrections so the emitted spec matches what the API returns. Spec accuracy fixes: - 3.1 compliant output, typed enums, records/dicts with real values - nullable/required corrected (chunkId, objectId, userId) - task status enum sourced from the new DTaskStatus define - getUserPermission helper references GlobalPermissionGroup, not RightGroup - relation schemas only emitted for relationship routes Adds an offline spec generator (ci/tools/generate-openapi.php) plus unit tests and JSON fixtures covering the generated spec.
Drop the separate /api/v2/openapi-compliant.json route and run the spec through SpecSanitizer in the existing openapi.json handler instead, so there is only one spec endpoint and it returns the corrected document. The offline generator loses its --compliant flag and always sanitizes, keeping its output identical to the endpoint.
SpecSanitizer stripped Slim placeholder constraints with a regex whose
body could not cross a closing brace, so the quantifier braces of the
tus upload route "{id:[0-9]{14}-[0-9a-f]{32}}" cut the match short and
left the remainder in the path.
Later phases read the already cleaned path, so the single mangled key
"/api/v2/helper/importFile/{id}-[0-9a-f]{32}}" also produced:
- operationIds like "deleteImportFileById}-[0-9a-f]{32", which failed
the Redocly operation-operationId-url-safe rule
- a bogus required path parameter named "32", matched out of the
leftover "{32}" by the missing path parameter fix
Replace the regex with a helper that locates the brace closing a
placeholder by counting depth, so the balanced braces of a constraint
are skipped. Redocly now reports the generated spec as valid.
Refines the spec generator so it describes more of the real API surface and matches the runtime output more closely: - Add per-model Count, PatchMultiple, DeleteMultiple and SingleResponse schemas plus the getGlobalConfig/getCompletedCount/getCracksPerDay helper paths, and clean the importFile path template. - Emit collection pagination links as self/first/last/next/prev to match the runtime, and give single-resource documents only a self link. - Pin the JSON:API string id cast (ResourceObjectIdTest) and refresh the FeatureTypeMapper/StaticFragments/HelperApiPathBuilder fragments. - Add a Spectral ruleset and wire it into the openapi-lint workflow; mark the generated spec fixtures as linguist-generated. - Regenerate the phpunit spec fixtures and update the generator tests.
JSON:API requires resource ids to be strings. The resource id and the relationship linkage were already cast, but foreign-key attributes (agent.userId, chunk.taskId, ...) were still emitted as integers, which is inconsistent and risks precision loss for bigint keys in JavaScript clients. Add a "reference" marker, derived from the existing model-column "relation" annotations, that flows through all three layers so the runtime output, the generated spec and the input validation stay in agreement: - generator.php: emit "reference" => <Target> in getFeatures - FeatureTypeMapper: type a reference as string (nullable -> [string, null]) - AbstractBaseAPI::db2json: serialize the value as a string id (null kept) - AbstractBaseAPI::validateData: accept a numeric string (or int) on input The column type stays int, so DB coercion, range checks and filter/sort keep working; only the JSON representation changes. Complete the missing relation annotations on Pretask.crackerBinaryTypeId and TaskWrapperDisplay.taskId/hashTypeId, and update the ConfigAPI attribute override so configSectionId is a string too. The polymorphic NotificationSetting.objectId and the usePreprocessor flag aliased as preprocessorId are intentionally left as integers. Regenerate the models and spec fixtures; add ReferenceIdTest covering the spec type, runtime serialization and input validation.
The spec was only reachable from a running server (GET /api/v2/openapi.json) or by regenerating it locally, which makes it hard to review what the API actually looks like and to diff it across changes. Add openapi.json, produced by `php ci/tools/generate-openapi.php --pretty` (170 paths, 286 schemas), and mark it linguist-generated so it is collapsed in code review like the fixture snapshots. Also finish the JSON:API string id change in the search-hashes helper example, which still showed integer id and hashlistId values, so the committed spec matches the committed source.
Three places where the APIv2 answered something JSON:API does not describe, which forced the generated spec to document non-compliant behaviour and the lint ruleset to switch rules off: Atomic operations instead of collection PATCH/DELETE JSON:API describes no PATCH or DELETE on a collection, so patchMultiple and deleteMultiple are replaced by POST <collection>/operations, the atomic operations extension (https://jsonapi.org/ext/atomic/). The endpoint is only routed for collections that can be modified, requires the ext="https://jsonapi.org/ext/atomic" media type parameter (415 without it), checks the permission of the method each operation maps to, applies the operations in order inside one transaction and reports one result each. The single-object create and update paths are factored into createResource and updateResource so both entry points validate identically; the ConfigAPI updateObjects override drops out with the old bulk route. Grouping several *Utils calls into one unit of work needs nested transactions, which plain PDO refuses. NestedTransactionPDO maps a nested begin/commit/rollBack onto savepoints and behaves like PDO at depth zero. JSON:API error documents ErrorHandler::errorResponse rendered RFC 7807 problem documents as application/problem+json. It now answers an "errors" document under application/vnd.api+json, with the status as the string JSON:API requires. Content negotiation ContentNegotiationMiddleware answers 415 when the request Content-Type is the JSON:API media type modified by a parameter other than ext/profile or naming an unimplemented extension, and 406 when no accepted instance of the media type is usable. Other media types (application/json, TUS uploads) pass through untouched. Also: cursor pagination moves from "ext" to "profile" in the jsonapi header, where JSON:API 1.1 keeps profile URIs. Generator and lint follow: the spec documents the operations endpoint with its request and result schemas per model, the error responses as JSON:API error documents and the 406/415 replies, and the Spectral ruleset re-scopes the rules that were switched off before, so no rule is disabled any more. Update the python integration tests to the new error format (error_title) and the atomic operations endpoint (patch_many/delete_many), add test_atomic_operations.py, and cover the new pieces with AtomicOperationsTest, NestedTransactionPDOTest and ContentNegotiationMiddlewareTest. Regenerate the spec fixtures and openapi.json.
…izer Title and version were built by SpecBuilder, while description, contact and license were filled in afterwards by SpecSanitizer. The two halves of the same info object lived in different classes, and the raw spec (which the builder tests and the fixtures compare against) was missing three of the five fields. Build the whole info object in SpecBuilder, so both spec variants identify the API and its license the same way. The sanitizer no longer touches info at all. Move the coverage accordingly: the two SpecSanitizerTest cases are replaced by one FullSpecTest case asserting the info of both variants. Regenerate the fixtures, which now carry the full info object; openapi.json is unchanged, since the sanitizer produced the same fields before.
The generator keyed its paths by the raw Slim pattern, constraints and all
("/api/v2/ui/configs/{id:[0-9]+}/{relation:configSection}"), and SpecSanitizer
rewrote them into path templates as one of its phases. Everything in between
therefore worked on strings that are not valid OpenAPI paths: StaticFragments
had to spell out a regex constraint to hit the right path item, and the raw
spec the builder tests and fixtures assert against was never a valid document.
Translate the pattern into a path template in RouteIntrospector, where the
route is resolved, and carry both on RouteTarget. The path template is what the
spec is keyed by; the raw pattern stays available because a constraint encodes
more than a validation rule, namely the relation name of a relationship route,
which ModelApiPathBuilder still reads from it.
Consequently the sanitizer loses its path cleaning phase (the remaining phases
are renumbered) and StaticFragments addresses the importFile paths by template.
The relationship routes of one model collapse into a single templated path item
now, which is what the sanitized spec always contained, so openapi.json is
unchanged; the fixtures show the collapse because they snapshot the raw spec.
RouteIntrospectorTest covers the translation, including the balanced braces of
a quantifier inside a constraint, which the two removed sanitizer cases used to
cover.
TaskAPI and PreTaskAPI each spelled out the same nine-entry $base array and merged type and alias into it per aggregate, which buries the two values that actually differ per feature and invites the next model to copy the block again. Add AbstractBaseAPI::aggregateFeature(), which fills in the defaults that hold for every computed property (read-only, no primary key, no dba mapping) and takes the type, the alias and whatever a feature has to say beyond that, e.g. the choices of an enum. Pure refactoring, the declared features are unchanged, so neither the fixtures nor openapi.json move.
Only TaskAPI and PreTaskAPI declared their computed attributes. The other six models offer aggregates through getAggregateFieldsets() without declaring them in getAggregateFeatures(), so a client asking for e.g. `aggregate[agent]=crackingTime` got back an attribute the document never mentions, and no generated client could reach it. Declare the aggregates of AgentAPI, AgentAssignmentAPI, SupertaskAPI, TaskWrapperDisplayAPI and ConfigAPI, plus the "cracked" field of TaskAPI that was offered but not declared. Two of them needed the generator to catch up: Config: the value boundaries of an item depend on its config type, so the members of the object differ per item and no feature type describes it. A feature may now carry an 'openapi_schema' that FeatureTypeMapper takes verbatim instead of looking the type up. Config again: it replaces its attributes through getOpenAPIAttributesSchemaOverride(), and the override used to be taken as the final word. Since aggregateData() appends its fields to whatever the override describes, the aggregates are now merged into it, into every branch of a oneOf. FullSpecTest walks every registered model and asserts that each offered fieldset entry is declared, is reachable under the same alias and appears in the response schema as an optional property, so the next aggregate cannot be added without documenting it.
The API applies the JSON:API "fields" parameter to the primary data and to every included resource (AbstractBaseAPI::obj2Resource), but the spec never mentioned it, so no generated client could ask for a narrowed resource. The "aggregate" parameter had the opposite problem: it was documented on collection reads only, although getOneResource applies it just as well, which left it unreachable on a single read, a create and an update. Both parameters shape the resource an operation returns rather than selecting which resources it returns, so they are built together in makeResourceShapeParameters() and attached wherever a resource comes back: the collection read, the single read, POST and PATCH. The aggregate block moves out of the collection branch into makeAggregateParameter() unchanged. The "fields" parameter names the attributes of its own type and the types reachable through "include", and warns that a narrowed resource no longer carries every attribute the response schema lists as required. Relationship routes answer with the related resource, so they are left out, as with "include". FullSpecTest checks for each operation that carries "fields" that the advertised type is the type the operation returns and that the example names attributes that response actually has.
The API orders a collection by the JSON:API "sort" parameter (AbstractBaseAPI::makeOrderFilterTemplates) without the spec saying so, so clients had to discover both the parameter and its accepted keys by reading the server. Document it on every collection read, next to filter and include, listing the attributes the collected resource has. Two details of the implementation shape the list: the primary key is addressed as "id" rather than under its own alias, and the parser accepts [_a-zA-Z.]+, so an attribute carrying a digit is not offered. The description names the "-" prefix for descending order, the "<relationship>.<attribute>" form for an included resource, and the primary key appended as a tie-breaker that keeps pagination stable. FullSpecTest asserts the parameter appears on collection reads only and that every advertised key is an attribute of the returned resource which the parser accepts.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
--- DRAFT ---
Extract the APIv2 OpenAPI spec generation out of the inline route closure into a dedicated library (src/inc/apiv2/openapi/), add ApiRegistry as the single source of truth for APIv2 class registration, and make a set of targeted corrections so the emitted spec matches what the API returns.
Spec accuracy fixes:
Adds an offline spec generator (ci/tools/generate-openapi.php) plus unit tests and JSON fixtures covering the generated spec.