fix(osgen)!: unshadow generic-erased response fields and reduce the type surface (#1034)

`_id` was unreachable from a typed search response. SearchResultHits embedded
SearchHitsMetadata, which declares json:"hits", and redeclared hits at depth 0.
encoding/json resolves a duplicate tag at differing depths in favor of the
shallower field, so the embedded slice was never populated: the winner held only
_source, against the shadowed type's full SearchHit envelope. resp.Hits.Hits[0].ID
returned nothing, and neither did _index, _score, _routing, the
optimistic-concurrency pair _seq_no/_primary_term, nor the sort cursor for
search_after pagination. Re-parsing SearchResp.RawBody() was the only escape.

The cause is the spec instantiating generic types (HitsMetadata[TDocument],
GetResult[TDocument]) as an allOf whose second member narrows one property to the
type argument. osgen has no generics, flattens TDocument to json.RawMessage, and
emitted the narrowing as a sibling field that shadowed the base. A narrowing that
substitutes nothing a Go type can express is now dropped when the allOf base
already declares the property, and a schema left contributing nothing beyond its
base resolves to the base instead of registering a wrapper. One that names a
concrete schema is left alone, which preserves the intended cases: bucket
aggregations narrowing buckets from the erased TBucket, and
SearchResultJSONValue.suggest, whose union adds a completion branch the base
lacks. Response bodies are exempt, since an operation's Resp inlines the fields of
whatever its ref resolves to.

Search hits were the loudest case, not the only one. GetResult[TDocument] is the
same erasure, so _source was also shadowed on Doc.Get, MGet, and Explain
responses, where GetResp redeclared _source over the copy it already inherited
from GetResultBase. Duplicate tags across an embed boundary drop from 30 to 22
over the generated surface, and the 22 that remain are the deliberate ones: 21
bucket aggregations plus SearchResultJSONValue.suggest. The eight cleared are
SearchResultHits.hits, SearchHitsMetadataJSONValue.hits,
SearchResultJSONValue.hits, SearchHitsMetadataHitsItem._source, GetResp._source,
GetResult._source, ExplainRespBodyGet._source, and one bucket narrowing that
collapsed with its wrapper.

Fixing that exposed how much of the generated surface existed only as an artifact
of the same erasure, so this also collapses it: opensearchapi goes from 2,379
exported generated types to 2,139. 34 of 66 identity wrappers are gone, 148
per-union branch error types become one UnionBranchError, a $ref'd union is named
after its own schema and emitted once rather than once per referencing field
(212 unions to 122, with 91 distinct shapes surviving where 87 did before), and
type names over 60 characters drop from 74 to 16.

osgen also now honors the OpenAPI discriminator, which it had ignored entirely.
Eight schemas declare one and every branch of all eight resolves distinctly, so
146 subtypes the payload names outright were being decoded by a JSON token-class
heuristic. Six unions gain a Type(), discriminant constants, and an UnmarshalJSON
that reads one property and decodes exactly that branch; a value naming no branch
is an error rather than a silent mis-decode. Accessors on a request-selected union
stop returning a zero value with a nil error when the payload cannot be that
branch, which a decode-error check cannot detect because encoding/json ignores
unknown keys.

Two generation guards keep the class of bug from returning. The first pins
duplicate JSON tags across an embed boundary, which is the exact shape that caused
this: go vet's structtag analyzer only compares tags within a single struct, and
golangci-lint excludes generated files, so nothing caught it. Its allowlist is
seeded with the 22 sites that exist today, all deliberate. The second reads
version extensions written beside a $ref, which kin-openapi keeps on the reference
rather than the resolved schema; 141 annotations were being dropped, and the same
values feed the version filter, so those fields could not be excluded by
-min-version or -max-version. The generic-substitution traversals are now bounded
by $ref cycle detection rather than a seven-level depth cap that was never a
termination guarantee.

BREAKING CHANGE: every union branch accessor returns (T, error) instead of T, so
v := u.Branch() becomes v, err := u.Branch().

BREAKING CHANGE: the 148 per-union branch error types are replaced by one
UnionBranchError carrying Union, Branch, Got, and an Err reachable through
errors.As.

BREAKING CHANGE: 135 generated union types are removed and 45 added, and branch
accessors drop the group prefix the union name already carries. See
opensearchapi/UPGRADING_V4_TO_V5.md for the rename table.

BREAKING CHANGE: apirev.Field.IsPointer is now a method and the surface JSON drops
the isPointer key. This lives under cmd/osapilint/internal/, so it is not
importable by consumers.

Co-authored-by: Sean Chittenden <sean.chittenden@crowdstrike.com>
Signed-off-by: Sean Chittenden <sean.chittenden@crowdstrike.com>
Signed-off-by: Ryan Yuan <ryan.yuan@crowdstrike.com>

Author: ryanyuan

.codecov.yml

@@ -1,7 +1,7 @@
 # https://docs.codecov.com/docs/pull-request-comments
 comment:
   layout: "diff, flags, files"
-  behavior: default
+  behavior: new
   require_changes: false  # if true: only post the comment if coverage changes
   require_base: false        # [true :: must have a base report to post]
   require_head: true       # [true :: must have a head report to post]

CHANGELOG.md

@@ -24,6 +24,10 @@
     - [Latency Profiles](#latency-profiles)
     - [Inspecting and Clearing Latency](#inspecting-and-clearing-latency)
   - [Code Generation](#code-generation)
+    - [Partial-failure error generation](#partial-failure-error-generation)
+    - [Version-Scoped Generation](#version-scoped-generation)
+    - [Contributing Spec Fixes Upstream](#contributing-spec-fixes-upstream)
+    - [Generation Guards](#generation-guards)
   - [Demo](#demo)
   - [Verification Matrix](#verification-matrix)
     - [Individual Verifications](#individual-verifications)
@@ -493,6 +497,44 @@ Version flags accept an optional operator prefix (`>=`, `>`, `<=`, `<`). When om
 
 When items are excluded by version filtering, breadcrumb comments are left in the generated code explaining the reason (e.g., `// cat.masterPath: deprecated in OpenSearch 2.0.0 (treated as removed).`). Breadcrumb visibility is configurable per category via `-version-breadcrumb-*` flags.
 
+### Contributing Spec Fixes Upstream
+
+Most generated code defects are not generator bugs. The spec is the input, so a missing doc comment, a wrong type, or an absent version annotation usually has to be fixed in [`opensearch-api-specification`](https://github.com/opensearch-project/opensearch-api-specification) to benefit every language client.
+
+Missing doc comments are the common case. Generated types and fields carry the `description` from their schema, and the generator emits 3,060 of the 3,187 property descriptions the spec provides; the remainder simply have none upstream. To see the gaps:
+
+```
+make report-missing-descriptions
+```
+
+This generates into a temporary directory and prints a report to stderr, so the checked-in generated files are untouched. Output is grouped into types, struct fields, and string-enum members, with the spec component key in brackets:
+
+```
+  - SearchProcessorExecutionDetail [_core.search___ProcessorExecutionDetail]
+  - ScrollResp.ProcessorResults json:"processor_results" [_core.search___SearchResponse]
+
+SUMMARY: 1274 types, 3471 fields, 20 enum members; 4765 total
+```
+
+The bracketed key is what to search for upstream. A gap is reported at both the reference site and the `$ref` target, so adding a description to one shared schema often resolves many lines at once.
+
+> **Editing `opensearch-openapi.yaml` locally.** The vendored spec may be edited for correctness (a wrong type, a missing required field), but changes are visible to every client generated from it, so renames and cosmetic edits belong upstream rather than here. Send a corresponding PR to `opensearch-api-specification` for anything kept locally.
+
+### Generation Guards
+
+Two checks run before any file is written, so a regression aborts generation instead of landing in the tree. Each pins its permitted set in a reviewed, checked-in allowlist:
+
+| Guard              | Allowlist                            | What it catches                                                       |
+| ------------------ | ------------------------------------ | --------------------------------------------------------------------- |
+| `json.RawMessage`  | `cmd/osgen/rawmessage_allowlist.txt` | A type the generator could not resolve, widening the raw-JSON surface |
+| Duplicate JSON tag | `cmd/osgen/tagshadow_allowlist.txt`  | A struct redeclaring a JSON tag its embedded type already carries     |
+
+The duplicate-tag guard covers a defect nothing else catches. `encoding/json` resolves a duplicate tag at differing depths in favor of the shallower field, so an outer redeclaration wins and the embedded declaration is never populated -- which is how the per-hit search envelope (`_id`, `_seq_no`, `sort`) became unreachable. `go vet`'s `structtag` analyzer only checks duplicates within one struct, and `golangci-lint` relaxes generated files.
+
+When a guard fails, read the offender it names and decide whether the change is intended. If it is, add the entry with `-update-tagshadow-allowlist` (or `-update-raw-message-allowlist`) and review the resulting allowlist diff as part of the change: adding an entry asserts the shadow is deliberate.
+
+> **`make regen` deletes generated files before writing.** An aborted generation therefore leaves the tree empty. Recover with `git checkout -- opensearchapi/ plugins/ internal/`.
+
 See [`cmd/osgen/README.md`](cmd/osgen/README.md) for the full flag reference and subcommand details.
 
 ## Lint

DEVELOPER_GUIDE.md

@@ -283,6 +283,25 @@ gen-api-update-rawlist: fetch-opensearch-spec  ## Regenerate API files and refre
 		-remove-deprecated=$(GEN_REMOVE_DEPRECATED) \
 		-update-raw-message-allowlist
 
+report-missing-descriptions: fetch-opensearch-spec  ## List generated identifiers whose OpenAPI schema has no description (upstream spec gaps)
+	@printf "\033[2m-> Reporting generated identifiers with no OpenAPI description...\033[0m\n"
+	@# Generation writes into a temp dir so the checked-in generated files stay
+	@# untouched; only the stderr report matters here. OSGEN_SKIP_GIT_CHECK lets
+	@# osgen write outside the working tree. Both allowlist checks are downgraded
+	@# to warnings so unrelated allowlist drift cannot abort before the report.
+	@tmp=$$(mktemp -d) && trap 'rm -rf "$$tmp"' EXIT && \
+	cd $(REPO_ROOT)/cmd/osgen && OSGEN_SKIP_GIT_CHECK=1 go run . api \
+		-spec $(OPENAPI_SPEC) \
+		-out "$$tmp/opensearchapi" \
+		-pkg opensearchapi \
+		-plugins-out "$$tmp/plugins" \
+		-min-version=$(GEN_MIN_VERSION) \
+		-max-version=$(GEN_MAX_VERSION) \
+		-remove-deprecated=$(GEN_REMOVE_DEPRECATED) \
+		-allow-unlisted-raw-message \
+		-allow-unlisted-tagshadow \
+		-report-missing-descriptions
+
 gen: gen-paths gen-api  ## Regenerate all code from OpenAPI spec (run gen-paths and gen-api in parallel with `make -j gen`)
 
 regen: clean-gen gen  ## Clean generated files then regenerate from spec

Makefile

@@ -120,8 +120,12 @@ func example() error {
 	}
 	fmt.Printf("Task completed: action=%s\n", taskResp.Task.Action)
 
-	// Read the BulkByScroll status.
-	status := taskResp.Task.Status.BulkByScrollTaskStatus()
+	// Read the BulkByScroll status. The accessor errors when the task's status
+	// decoded as some other branch of the union.
+	status, err := taskResp.Task.Status.BulkByScrollTaskStatus()
+	if err != nil {
+		return err
+	}
 
 	fmt.Printf("Total: %d\n", status.Total)
 	if status.Created != nil {

_samples/usage-tasks.go

@@ -31,14 +31,26 @@
 // map.
 package apirev
 
+import "strings"
+
 // Field is one exported struct field as seen by the type checker.
 type Field struct {
-	Name      string `json:"name"`
-	Type      string `json:"type"`      // types.Type.String()
-	IsPointer bool   `json:"isPointer"` // true if the field's type is a pointer
-	JSONTag   string `json:"jsonTag,omitempty"`
+	Name    string `json:"name"`
+	Type    string `json:"type"` // types.Type.String()
+	JSONTag string `json:"jsonTag,omitempty"`
 }
 
+// IsPointer reports whether the field's type is a pointer. Pointer-ness is
+// derived from Type rather than stored in the surface, because Type is
+// types.Type.String() and a pointer type always renders with a leading "*" -
+// storing a separate flag duplicated information the surface already carried.
+//
+// The prefix test is required: a non-leading "*" shows up in slice-of-pointer
+// types ("[]*net/url.URL"), func types ("func(*net/http.Request)"), and inline
+// anonymous struct types whose embedded tag text mentions a pointer. None of
+// those are pointers, so a substring test would misreport them.
+func (f Field) IsPointer() bool { return strings.HasPrefix(f.Type, "*") }
+
 // Struct is one exported struct type, qualified by its package import path so
 // same-named types in different packages (e.g. opensearch.Config vs
 // opensearchtransport.Config) never collide.

cmd/osapilint/internal/apirev/apirev.go

@@ -233,7 +233,7 @@ func diffFields(sFrom, sTo Struct, dispByFrom map[string]FieldDisposition) []Fie
 	for _, fFrom := range sFrom.Fields {
 		fTo, still := toByName[fFrom.Name]
 		switch {
-		case still && fTo.IsPointer && !fFrom.IsPointer:
+		case still && fTo.IsPointer() && !fFrom.IsPointer():
 			changes = append(changes, FieldChange{Kind: KindPointerWrap, From: fFrom.Name, NewType: fTo.Type})
 		case still && incompatibleTypeChange(fFrom.Type, fTo.Type):
 			// Field kept its name but its type changed in a way that breaks

cmd/osapilint/internal/apirev/delta.go

@@ -74,9 +74,10 @@ func ExtractFromDir(dir, version string, patterns ...string) (*Snapshot, error)
 }
 
 // extractStruct records every exported field of st, flattening embedded structs
-// so a field that moved into an embedded base type (e.g. v5's GetResultBase) is
-// still seen as present rather than mis-read as removed. Unexported fields are
-// skipped: the rewriter only rewrites fields a caller can name in a literal.
+// so a field that moved into an embedded base type (e.g. v5's
+// CommonAggregationsAggregateBase) is still seen as present rather than mis-read
+// as removed. Unexported fields are skipped: the rewriter only rewrites fields a
+// caller can name in a literal.
 func extractStruct(pkgPath, name string, st *types.Struct) Struct {
 	s := Struct{PkgPath: pkgPath, Name: name}
 	s.Fields = flattenFields(st, map[string]bool{})
@@ -112,12 +113,10 @@ func flattenFields(st *types.Struct, seen map[string]bool) []Field {
 		if !f.Exported() {
 			continue
 		}
-		_, isPtr := f.Type().(*types.Pointer)
 		out = append(out, Field{
-			Name:      f.Name(),
-			Type:      f.Type().String(),
-			IsPointer: isPtr,
-			JSONTag:   reflect.StructTag(st.Tag(i)).Get("json"),
+			Name:    f.Name(),
+			Type:    f.Type().String(),
+			JSONTag: reflect.StructTag(st.Tag(i)).Get("json"),
 		})
 	}
 	return out

cmd/osapilint/internal/apirev/extract.go

@@ -65,7 +65,7 @@ func TestArgDetailV2toV3AgainstSurfaces(t *testing.T) {
 		require.Truef(t, ok, "struct %s.%s not found in surface", pkg, structName)
 		m := make(map[string]bool, len(st.Fields))
 		for _, f := range st.Fields {
-			m[f.Name] = f.IsPointer
+			m[f.Name] = f.IsPointer()
 		}
 		return m
 	}