NMS-20263: Improve shipped OpenAPI/Swagger Docs - #8817
Open
marshallmassengill wants to merge 7 commits into
Open
NMS-20263: Improve shipped OpenAPI/Swagger Docs#8817marshallmassengill wants to merge 7 commits into
marshallmassengill wants to merge 7 commits into
Conversation
Seven modules carry JAX-RS resources or their DTOs but had no swagger-annotations dependency, so their operations could not be annotated at all. Scope is provided: the jar already ships in the webapp's WEB-INF/lib.
… and examples Every one of the 356 v2 operations now carries a summary, a unique operationId, a request schema and example where it reads a body, and a schema and example per reachable status code. Each was exercised against a running instance and the annotations describe what it did, including where that differs from what the code appears to intend. Operations with a summary go from 93 to 355, response contents with a schema from 65 of 610 to 682 of 693, and response examples from none to 653. Generator-assigned operationIds such as createSpecific_14 drop from 213 to 5. Sixty-seven operations carried duplicate parameter declarations, which is invalid OpenAPI; none do now. The generic collection operations come from AbstractDaoRestServiceWithDTO, which is generic, so a single annotation there would give every DAO-backed resource the same operationId and no per-family entity schema. Each concrete resource instead declares delegating overrides that call super and carry only swagger annotations. No runtime logic changes. Handlers that assemble an ad-hoc Map have no type to reference, so this adds documentation-only classes describing those shapes. The bsm response DTOs that carry a custom @JsonSerialize are documented by example alone: swagger-core 2.1.12 introspects such serializers through a Jackson method removed after 2.12, which aborts document generation.
… and examples All 310 v1 operations now carry a summary, a unique operationId, and a schema and example per reachable status code, each verified against a running instance. Operations with a summary go from 32 to 310, response contents with a schema from 273 of 494 to 606 of 606, and response examples from none to 516. Where behaviour and intent disagree, the annotations record the behaviour: v1 error bodies are text/plain even under a JSON @produces, form-encoded keys are bean property names rather than the hyphenated names the read endpoints emit, several fields are XML attributes and are dropped if sent as elements, dates are epoch milliseconds in JSON and ISO-8601 in XML from the same field, and a node may be addressed either by database id or by foreignSource:foreignId. The /config family is documented for the first time; several of its resources carried no annotations at all. Its writes re-marshal the whole target file, which is noted on the tag because it discards comments in the shipped etc XML. The /measurements component schemas named properties the API rejects: those model classes carry Jackson 1 annotations and JAXB names that the generator cannot see, and three @XmlValue properties were missing from the schema entirely, leaving no way to express an expression or a filter parameter value. A request assembled purely from the corrected schema now succeeds.
marshallmassengill
requested review from
cgorantla,
christianpape,
dino2gnt,
indigo423,
mershad-manesh and
synqotik
August 26, 2026 12:34
…bserved behaviour
Reviewer feedback was that the wording was too much and should state only what
is true. Every description now holds facts a reader can check against a running
instance. Advice, unmeasured performance claims, guesses at intent, appeals to
the reader and disclaimer-style negatives are gone, and the long tag overviews
are shorter. URL trees, parameter lists and enumerated values are untouched.
Rendered prose drops from 128969 to 118763 characters in v2 and from 156608 to
142234 in v1, roughly 24600 characters. Operation count, operationIds,
summaries, request and response schemas and every example are unchanged.
Four descriptions were wrong rather than merely wordy. All nine enlinkd DTO
pairs described both the create and the last-poll timestamp as a poll
timestamp. EventConfEventEditRequest said both members were optional while its
operation documents a 500 for a body carrying only `enabled`. The v1 event
publish description treated `uei` as required, contradicting the documented
empty-body 202. Nineteen literal newline escapes inside text blocks in the
heatmap descriptions rendered as a line break mid-sentence.
Seven places where a description and its response set disagreed are reconciled
against the instance rather than by deleting the prose. `GET /nodes/service-types`
answers 500 for an XML Accept, `PUT` and `DELETE /ipinterfaces/{id}` answer 400
for an ambiguous address, `POST /ksc` answers 415, `POST /events` does consume
`application/atom+xml`, and the three `/acks` reads answer 403 for a caller
holding none of the roles the filter chain requires, as the alarm reads already
documented. The status business-service list serializes as `businessservices`,
so the example that showed `business-services` was corrected; the sibling
`/business-services` list does emit the hyphenated key. Bridge elements are one
per discovered VLAN, with a single null-vlan entry when the device has no VLAN
table, not one per bridge base address.
…undles The swagger annotations have runtime retention, so adding them to classes in bundle-packaged modules made bnd emit a mandatory Import-Package for io.swagger.v3.oas.annotations and its subpackages. Sentinel does not ship that bundle, so its feature resolution failed on org.opennms.features.measurements.api and every sentinel smoke test timed out waiting for a health check. The annotations are only read at build time by opennms-openapi-docs, which works off the compile classpath, so an optional import is enough. This matches what opennms-config-model and features/events/api already do.
Every operation's documented status codes, error bodies and examples were re-verified against the handlers, the shared helpers and the DAOs, with read-only live probes where the code path was ambiguous. Fixes the claims that did not survive: - v1 error bodies: ~75 4xx/5xx responses across seven services declared application/json where OnmsRestService's helpers emit text/plain - v2 Minion and MonitoringLocation JSON/XML PUT: an unknown id is not a 404, saveOrUpdate inserts a new row and answers 204 - unknown query parameters in v1 node/interface/ifservices resources fail with 500 'Unknown entity: null' rather than being ignored; the v1 minions filter 400 is likewise really a 500 - situations: unknown alarm ids fail with 500 rather than being dropped, the _s behaviour claim was wrong in both directions, the 403 examples now quote SecurityHelper's real messages, and the update/memo/journal/ ticket operations document their reachable 403 - unreachable responses corrected: v1 whoami 204 (really 404), category availability 404 (rewrapped as 500), eventconf delete 400 (mapped to 500), web-assets fallback 404 (broken list mutation, 500) - wrong facts: KSC timespan list missing twelve valid values, the three datacollection child filters fall back to sorting by name rather than createdTime, wrong schema classes on the v1 trapd and datacollection config resources, kebab-case JSON keys on the snmp config resource, a malformed JSON example in GraphRestService, totalCount:null examples contradicted by JaxbListWrapper's count fallback, and ten datacollection operations missing their real 500 response - the v1 trapd operationId no longer collides with the v2 one The Jackson 2 @JsonProperty annotations added to the measurements model for the OpenAPI generator are now named explicitly so a Jackson 2 consumer would serialize the real wire name 'value', and the new com.fasterxml.jackson.annotation import on that bundle is optional, in line with the swagger imports. Also drops two unused imports and an unrelated license-header reflow.
Pointing the v1 trapd response at org.opennms.netmgt.config.trapd.TrapdConfiguration broke document generation: swagger-core introspects the class with Jackson, which rejects its conflicting array and List setters for snmpv3User. Document the operation by example only, as the BSM responses with custom serializers already are. The v2 TrapdConfigDto is not a substitute here because the v1 response carries the XSD attribute names.
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.
After some user feedback in mattermost it's pretty apparent we need to add response/request examples and schemas to pretty much all of our API docs. This gets that done in bulk so it's a hefty update but it does not touch any runtime logic.
In theory, every operation in both v1 and v2 APIs now carries a summary, a unique operationId, a request schema and example where it reads a body, and a schema and example for each status code it can actually return. Each was exercised against a running instance, so the annotations describe observed behavior rather than intent.
I leveraged multiple claude agents to do this in one go so I would treat this as a bulk operation on the premise that some documentation that could be wrong is better than a complete void of documentation.
It definitely will need further refinement and review. I've spot checked a handful of these and they do seem accurate enough that someone can more easily sort out how the API works and it's a meaningful improvement over what we have today.
This one is a "I'd claim assisted but that feels like cheating" by Anthropic Claude Opus 5 (x10).
External References