knuckleswtf / scribe

Generate API documentation for humans from your Laravel codebase.✍
https://scribe.knuckles.wtf/laravel/
MIT License
1.58k stars 280 forks source link

Endpoint URL params for OpenAPI #804

Open tcox-enrollfirst opened 4 months ago

tcox-enrollfirst commented 4 months ago

Slight enhancement to the generated OpenAPI specification by simply moving URL parameters down to the endpoint level. This can improve some http clients' ability to import and render the openapi.yaml specification accurately. Generally, it's probably not a bad idea to define all parameters explicitly at the request level. Tests have been updated and are passing.

Example Routes

/**
 * Search Trucks
 * 
 */
Route::get('trucks', [TruckController::class, 'search'])->name('demo.search');

/**
 * View Truck
 * 
 * @urlParam id string Unique identifier for the truck
 */
Route::get('trucks/{id}', [TruckController::class, 'view'])->name('demo.view');

BEFORE

openapi.yaml

paths:
  /trucks: ...
  '/trucks/{id}':
    get:
      summary: 'View a Truck'
      operationId: getTrucksId
      description: 'View a specific Truck'
      parameters: []
      responses: ...
      tags: ...
      security: []
    parameters:
      -
        in: path
        name: id
        description: 'The ID of the truck.'
        example: TRK123
        required: true
        schema:
          type: string

A Client

image

AFTER

openapi.yaml

paths:
  /trucks: ...
  '/trucks/{id}':
    get:
      summary: 'View a Truck'
      operationId: getTrucksId
      description: 'View a specific Truck'
      parameters:
        -
          in: path
          name: id
          description: 'The ID of the truck.'
          example: TRK123
          required: true
          schema:
            type: string
      responses: ...
      tags: ...
      security: []

Same Client

image

shalvah commented 4 months ago

I think this is a bit of a controversial change. Functionality's probably the same, but this will unnecessarily increase the size of specs, no?

tcox-enrollfirst commented 4 months ago

It's not unnecessary if the results are more accurate. Rather be right multiple times than wrong the one time. Id rather my customers have a few extra lines in a small spec file than a smaller spec file that can be misinterpreted by importers. In a perfect world, all http clients would load the spec properly but unfortunately that's not the case. It's just a little more verbose, and there's nothing wrong with that.

shalvah commented 4 months ago

I'm inclined to merge this, but I wouldn't characterize it as "a little more" verbose. If you have a /things/{id} endpoint that can take GET, POST, and DELETE, you'll now have 3x URL parameters rather than 1. Multiply this by the number of variables in the URL (/things/{id}/other-things/{other_id} goes from 2 to 6 variables), and by the number of endpoints (a project that has 50 endpoints of this form will go from 100 variables to 300), and it could really bloat files. But in the first place, the generated specs weren't meant for human consumption, so I think this may be fine.

tcox-enrollfirst commented 4 months ago

I hear ya, it's definitely less efficient... Would you be more open to a config-driven solution?

Something like this?

// Generate an OpenAPI spec (v3.0.1) in addition to docs webpage.
// For 'static' docs, the collection will be generated to public/docs/openapi.yaml.
// For 'laravel' docs, it will be generated to storage/app/scribe/openapi.yaml.
// Setting `laravel.add_routes` to true (above) will also add a route for the spec.
'openapi' => [
    'enabled' => true,

    // Generated spec can list URL params at either the "path" or "method" level
    // - "path" lists URL params once for every method that share's the same path
    // - "method" lists URL params under every method within each path grouping
    'url_params' => 'path',

    'overrides' => [
        // 'info.version' => '2.0.0',
    ],
],
shalvah commented 3 months ago

Honestly, probably not. It makes sense, but I'm wary of adding more config items.