Skip to content

Make API OPTIONS report actual controller capabilities - #4645

Open
ammad-elev8ai wants to merge 2 commits into
phpipam:developfrom
Elev8AIuk:feature/api-options-dynamic-controller-list
Open

ammad-elev8ai wants to merge 2 commits into
phpipam:developfrom
Elev8AIuk:feature/api-options-dynamic-controller-list

Conversation

@ammad-elev8ai

Copy link
Copy Markdown

Part of #4642 (context/overview for a set of 4 related API fixes).

Gap

OPTIONS /api/{app_id}/ is supposed to describe what the API supports, but the
controllers list is missing entries for functionality that's fully working and
routable - most notably customers (a working tools/ sub-resource) and circuits (a
working top-level controller):

curl -s -k -X OPTIONS https://host/api/my_app/
{
  "code": 200, "success": true,
  "data": {
    "permissions": "Write",
    "controllers": [
      {"rel": "sections", "href": "/api/my_app/sections/"},
      {"rel": "subnets", "href": "/api/my_app/subnets/"},
      {"rel": "folders", "href": "/api/my_app/folders/"},
      {"rel": "addresses", "href": "/api/my_app/addresses/"},
      {"rel": "vlans", "href": "/api/my_app/vlan/"},
      {"rel": "vrfs", "href": "/api/my_app/vrf/"},
      {"rel": "nameservers", "href": "/api/my_app/tools/nameservers/"},
      {"rel": "scanAgents", "href": "/api/my_app/tools/scanagents/"},
      {"rel": "locations", "href": "/api/my_app/tools/locations/"},
      {"rel": "racks", "href": "/api/my_app/tools/racks/"},
      {"rel": "nat", "href": "/api/my_app/tools/nat/"},
      {"rel": "tools", "href": "/api/my_app/tools/"}
    ]
  }
}

No customers, no circuits - despite both being usable today. This ambiguity is also
visible in #4560, where a user couldn't tell circuits had a controller at all.

Root cause

Tools_controller::OPTIONS() (the handler used for both OPTIONS /api/{app}/ and
OPTIONS /api/{app}/tools/..., since index.php forces controller=Tools whenever an
OPTIONS request has none) builds controllers from a hand-maintained static array that's
disconnected from $this->subcontrollers, the actual registry of what's dispatchable under
tools/. It had already drifted.

Fix

Generate the tools/* portion of the list from $this->subcontrollers directly, so it
can't silently drift out of sync again, and add the missing top-level circuits entry
(and circuitsLogical, once #4646 also lands - see note below). Per-controller
OPTIONS() methods (e.g. Circuits::OPTIONS(), which already correctly reports
verb-level capabilities for its own routes) are untouched - this is specifically about the
root/tools discovery list being wrong, not a broader OPTIONS redesign.

Related to #4560 (contributed to the confusion about whether a circuits controller
existed).

Tested with

Verified OPTIONS /api/{app_id}/ now lists customers and circuits, and every
tools/* sub-resource entry matches what Tools_controller actually dispatches
(including the URL-segment quirks, e.g. ipTags → tools/tags/, vrf → tools/vrfs/).

Tested against PHP 8.2, 8.3, and 8.4 - lint clean and passing on each, no new PHP errors.

Suggested merge order: independent, can merge in any order. This branch already adds
a circuitsLogical entry to the list in anticipation of #4646 (CircuitsLogical
controller) - harmless if this merges first (the href just won't resolve to anything
until #4646 also lands), but ideally merge together or #4646 first.

The root/tools OPTIONS response listed a hand-maintained, static set
of controllers that had already drifted from what's actually
routable: "customers" was a fully working tools sub-resource
(Tools_controller::define_tools_controllers()) but missing from the
list, and the "circuits" controller wasn't listed at all.

Generate the tools/* portion of the list from the real subcontroller
registry instead of a hand-written array, so it can't drift again,
and add the top-level "circuits" entry.
Small follow-up now that the CircuitsLogical controller exists (see
the feature/api-circuitslogical-controller branch/PR). This commit
has a soft dependency on that one landing - the href is just
documentation, so merging this first won't break anything, but the
entry won't be genuinely accurate until both are in.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant