feat: bake openapi schemas into endpoint annotations#1315
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
📊 Coverage ReportOverall Coverage: 92% Diff: origin/main...HEAD
Summary
Line-by-lineView line-by-line diff coveragelibs/server/kiln_server/utils/agent_checks/dump_annotations.pyLines 51-59 51
52 def _ref_name(ref: str) -> str | None:
53 """Return the component-schema name for a local $ref, else None."""
54 if not isinstance(ref, str) or not ref.startswith(_COMPONENT_REF_PREFIX):
! 55 return None
56 return ref[len(_COMPONENT_REF_PREFIX) :]
57
58
59 def _inline_schema(Lines 81-89 81 if "$ref" in node:
82 ref = node["$ref"]
83 name = _ref_name(ref)
84 if name is None:
! 85 raise _SchemaResolutionError(f"Unsupported $ref: {ref!r}")
86 if name in resolving:
87 deferred_defs.setdefault(name, {})
88 return {"$ref": f"#/$defs/{name}"}
89 if name not in components_schemas:Lines 111-119 111 inlined, defs_map = _inline_schema(schema, components_schemas)
112 if not defs_map:
113 return inlined
114 if not isinstance(inlined, dict):
! 115 raise _SchemaResolutionError("Cannot attach $defs to non-object schema root")
116 assert "$defs" not in inlined, "Unexpected pre-existing $defs in component schema"
117 return {**inlined, "$defs": defs_map}
118 Lines 151-163 151 "skipping request body schema"
152 )
153 return None
154 if json_entry.media_type_schema is None:
! 155 logger.warning(
156 f"application/json requestBody has no schema on {method.upper()} {path}; "
157 "skipping request body schema"
158 )
! 159 return None
160 raw_schema = raw_operation["requestBody"]["content"]["application/json"]["schema"]
161 inlined = _inline_with_defs(raw_schema, components_schemas)
162 return {
163 "required": request_body.required,Lines 196-212 196 # Operation-level parameters override path-item ones with the same (name, in).
197 merged: dict[tuple[str, ParameterLocation], dict] = {}
198 for typed, raw in zip(typed_params, raw_params):
199 if isinstance(typed, Reference):
! 200 logger.warning(
201 f"Parameter $ref not supported on {method.upper()} {path} "
202 f"(parameter {typed.ref!r}); skipping"
203 )
! 204 continue
205 if typed.param_in not in _SUPPORTED_LOCATIONS:
206 continue
207 if not isinstance(raw, dict):
! 208 continue
209 merged[(typed.name, typed.param_in)] = raw
210
211 for (name, param_in), raw in merged.items():
212 location = param_in.valueLines 283-295 283 components_schemas,
284 method,
285 path,
286 )
! 287 except _SchemaResolutionError as e:
! 288 logger.warning(
289 f"Failed to extract parameters on {method.upper()} {path}: {e}"
290 )
! 291 parameters = {"path": {}, "query": {}}
292
293 filename = normalize_endpoint_filename(method, path)
294 filepath = os.path.join(target_folder, filename)
295 with open(filepath, "w", encoding="utf-8") as f:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enhances API endpoint annotations by adding request_body and parameters metadata derived from the OpenAPI specification. It implements a schema inlining process in dump_annotations.py that supports recursive models via local $defs and provides updated documentation and tests. The review identifies bugs in parameter merging for $ref objects and reference resolution within discriminator.mapping, while also suggesting an optimization for resolving cyclic models.
| merged: dict[tuple[str, str], dict] = {} | ||
| for param in path_item.get("parameters", []) or []: | ||
| if not isinstance(param, dict): | ||
| continue | ||
| name = param.get("name") | ||
| location = param.get("in") | ||
| if isinstance(name, str) and isinstance(location, str): | ||
| merged[(name, location)] = param | ||
| for param in operation.get("parameters", []) or []: | ||
| if not isinstance(param, dict): | ||
| continue | ||
| name = param.get("name") | ||
| location = param.get("in") | ||
| if isinstance(name, str) and isinstance(location, str): | ||
| merged[(name, location)] = param |
There was a problem hiding this comment.
The current logic for merging parameters from path_item and operation silently ignores parameters defined as $ref (e.g., {"$ref": "#/components/parameters/MyParam"}). This is because the loop checks for name and in keys, which are absent in a $ref object. Consequently, if an operation overrides a path-level parameter using a $ref, the path-level parameter will incorrectly persist in the merged dictionary. Additionally, the warning for unsupported parameter $ref on line 187 is unreachable because such objects never make it into the merged map.
| if "$ref" in node: | ||
| ref = node["$ref"] | ||
| name = _ref_name(ref) | ||
| if name is None: | ||
| raise _SchemaResolutionError(f"Unsupported $ref: {ref!r}") | ||
| if name in resolving: | ||
| deferred_defs.setdefault(name, {}) | ||
| return {"$ref": f"#/$defs/{name}"} | ||
| if name not in components_schemas: | ||
| raise _SchemaResolutionError( | ||
| f"$ref {ref!r} not found in components.schemas" | ||
| ) | ||
| resolving.add(name) | ||
| try: | ||
| inlined = recurse(components_schemas[name]) | ||
| finally: | ||
| resolving.discard(name) | ||
| if name in deferred_defs: | ||
| deferred_defs[name] = inlined | ||
| return {"$ref": f"#/$defs/{name}"} | ||
| return inlined |
There was a problem hiding this comment.
The recurse function in _inline_schema only resolves $ref keys within dictionaries. However, OpenAPI's discriminator.mapping uses a dictionary where the values are strings representing refs (e.g., "mapping": {"type_a": "#/components/schemas/TypeA"}). These strings will not be resolved or updated by the current logic, leading to broken references in the generated annotation files if a discriminator is present.
| if "$ref" in node: | ||
| ref = node["$ref"] | ||
| name = _ref_name(ref) | ||
| if name is None: | ||
| raise _SchemaResolutionError(f"Unsupported $ref: {ref!r}") | ||
| if name in resolving: | ||
| deferred_defs.setdefault(name, {}) | ||
| return {"$ref": f"#/$defs/{name}"} | ||
| if name not in components_schemas: | ||
| raise _SchemaResolutionError( | ||
| f"$ref {ref!r} not found in components.schemas" | ||
| ) | ||
| resolving.add(name) | ||
| try: | ||
| inlined = recurse(components_schemas[name]) | ||
| finally: | ||
| resolving.discard(name) | ||
| if name in deferred_defs: | ||
| deferred_defs[name] = inlined | ||
| return {"$ref": f"#/$defs/{name}"} | ||
| return inlined |
There was a problem hiding this comment.
To improve efficiency and avoid redundant processing, recurse should check if a model name is already present in deferred_defs before attempting to resolve it again. If it is present and has a non-empty value, it means the model has already been identified as cyclic and fully resolved, so the local ref can be returned immediately.
if "$ref" in node:
ref = node["$ref"]
name = _ref_name(ref)
if name is None:
raise _SchemaResolutionError(f"Unsupported $ref: {ref!r}")
if name in resolving:
deferred_defs.setdefault(name, {})
return {"$ref": f"#/$defs/{name}"}
if name in deferred_defs and deferred_defs[name]:
return {"$ref": f"#/$defs/{name}"}
if name not in components_schemas:
raise _SchemaResolutionError(
f"$ref {ref!r} not found in components.schemas"
)
resolving.add(name)
try:
inlined = recurse(components_schemas[name])
finally:
resolving.discard(name)
if name in deferred_defs:
deferred_defs[name] = inlined
return {"$ref": f"#/$defs/{name}"}
return inlined
1 participant
What does this PR do?
Add endpoint request schema into the endpoint annotations so we can validate client toolcalls on the backend.
Change is only in
libs/server/kiln_server/utils/agent_checks/dump_annotations.py, the rest of the files are*.jsonannotations regenerated.Checklists