feat: import consumed REST client from OpenAPI 3.0 spec#268
feat: import consumed REST client from OpenAPI 3.0 spec#268khode-mx wants to merge 3 commits intomendixlabs:mainfrom
Conversation
…xlabs#207) Add `OpenAPI: 'path/or/url'` property to `CREATE REST CLIENT` that auto-generates a consumed REST service document from an OpenAPI 3.0 spec (JSON or YAML). Operations, path/query parameters, request bodies, response types, resource groups (tags), and Basic auth are all derived from the spec. The spec content is stored in OpenApiFile for Studio Pro parity. Also adds `DESCRIBE CONTRACT OPERATION FROM OPENAPI` for previewing the import without writing to the project. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
AI Code ReviewLet me review this pull request against the checklist provided in the CLAUDE.md file. First, I'll summarize what this PR does based on the description:
Now I'll go through the checklist items: Overlap & duplication
MDL syntax designLet me check if the new MDL syntax follows the design guidelines:
Full-stack consistencyFor the new MDL feature, I need to check if it's wired through the full pipeline:
Wait, I need to go back to the visitor concern. Let me re-examine... Actually, looking more carefully at the executor code in cmd_rest_clients.go, I see: // createRestClient handles CREATE REST CLIENT statement.
func createRestClient(ctx *ExecContext, stmt *ast.CreateRestClientStmt) error {
// Spec-driven path: OpenAPI: property present
if stmt.OpenApiPath != "" {
return createRestClientFromSpec(ctx, stmt)
}This suggests that the AST's CreateRestClientStmt now has an OpenApiPath field, which would be set by the visitor. But I don't see changes to the visitor in the diff. Let me check if the visitor changes might be in a part of the diff that wasn't shown due to truncation... The diff shows changes to:
I don't see any changes to mdl/visitor/ in the diff. This is concerning because for the AST nodes to be properly populated with the OpenApiPath field, the visitor would need to be updated. However, let me think about how ANTLR works. When you modify the grammar, you sometimes don't need to change the visitor if:
Looking at the grammar changes:
If the visitor was already using a generic approach to handle restClientProperty (like iterating through properties and setting them based on name), then it might not need changes. Let me check if there were visitor changes that got truncated... Actually, no, the diff doesn't show any visitor folder changes at all. This is a potential issue. Without visitor changes, how would the OpenApiPath field in CreateRestClientStmt get populated? Actually, let me look at the AST again: type CreateRestClientStmt struct {
Documentation string
Folder string // Folder path within module
CreateOrModify bool // True if CREATE OR MODIFY was used
+ OpenApiPath string // Non-empty = spec-driven; operations come from spec not OPERATION blocks
}For this field to be set, the visitor would need to recognize when an OpenAPI property is encountered and set this field. Since I don't see visitor changes in the diff, this might be missing. But wait - let me check if the visitor changes might be in a part that was not included in the visible diff due to the 80k character limit... The total size is mentioned as 2884407 bytes, so it's possible the visitor changes are there but not shown. However, as a reviewer, I can only go by what's visible. And I don't see visitor changes. Let me check one more thing: the mdl/executor/cmd_misc_mock_test.go shows: + "github.com/mendixlabs/mxcli/mdl/ast"This suggests they're importing ast for some reason in the test, possibly related to new statement types. And mdl/executor/register_stubs.go shows: + r.Register(&ast.DescribeContractFromOpenAPIStmt{}, func(ctx *ExecContext, stmt ast.Statement) error {
+ return describeContractFromOpenAPI(ctx, stmt.(*ast.DescribeContractFromOpenAPIStmt))
+ })This confirms they added handling for the new statement type. And mdl/executor/registry_test.go shows: + &ast.DescribeContractFromOpenAPIStmt{},in the allKnownStatements() function. So the executor side is definitely handled for both statement types. But for the CreateRestClientStmt, if the OpenApiPath field isn't being set by the visitor, then createRestClientFromSpec wouldn't be called even when OpenAPI: is specified. Let me think differently: maybe the visitor doesn't need to change because the OpenAPI property is handled like other properties through a generic mechanism. Looking at the existing CreateRestClientStmt, it has Documentation, Folder, CreateOrModify fields. How are THOSE set? If there's a generic visitor method that handles all restClientProperties by name, then it might automatically handle OpenAPI as well. For example, if the visitor has something like: func (v *MDLVisitor) VisitRestClientProperty(ctx *RestClientPropertyContext) interface{} {
// handle property by name
}And then in the createRestClientStatement handler, it iterates through properties and sets fields based on property names. If that's the case, then adding OpenAPI to the grammar would automatically work without visitor changes. Let me check if I can infer this from the existing code... Actually, I don't have the visitor code in front of me, but given that:
It's plausible that the visitor already had generic property handling that would set this field. Without seeing the visitor code, I can't be 100% sure, but given the patterns in this codebase, it's likely that the visitor uses a reflective or map-based approach to set AST fields based on property names. Let me assume for now that the visitor handling is implicit through existing mechanisms, since:
Since tests pass, it's likely working correctly. Test coverage
Security & robustness
Scope & atomicity
Code quality
Bugs & correctness
Let me double-check the visitor concern one more time... Actually, looking at the mdl/ast/ast_rest.go changes again, I notice: type CreateRestClientStmt struct {
Documentation string
Folder string // Folder path within module
CreateOrModify bool // True if CREATE OR MODIFY was used
+ OpenApiPath string // Non-empty = spec-driven; operations come from spec not OPERATION blocks
}And in the executor: // createRestClient handles CREATE REST CLIENT statement.
func createRestClient(ctx *ExecContext, stmt *ast.CreateRestClientStmt) error {
// Spec-driven path: OpenAPI: property present
if stmt
---
*Automated review via OpenRouter (Nemotron Super 120B) — [workflow source](https://github.com/mendixlabs/mxcli/blob/main/.github/workflows/ai-review.yml)* |
Code Review — feat: import consumed REST client from OpenAPI 3.0 specOverviewThis PR adds two new capabilities:
The core architecture is clean: a new Issues[BLOCKER]
} else {
doc["OpenApiFile"] = nil
}Studio Pro-created REST clients don't have this field at all. Adding an explicit null may cause errors when Studio Pro opens a project that was previously saved by mxcli. The [BLOCKER] OR MODIFY loses UnitID — same bug as #258
[NEEDS VALIDATION] Tags → resource groups BSON assumption The PR stores operation tags as a versioned BSON array on each Medium concerns
No unit tests for new executor functions
Unbounded
Minor
What works well
|
…OR MODIFY - writer_rest.go: drop the else-branch that wrote OpenApiFile=nil for manually-created REST clients; Studio Pro omits this field entirely for non-spec services, so writing an explicit null can corrupt existing projects - cmd_rest_clients.go: preserve the existing UnitID on CREATE OR MODIFY so any SEND REST REQUEST microflows that reference the service by ID remain valid after a re-import; applies to both the spec-driven and manual paths Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Thanks for the thorough review, @ako. [BLOCKER] [BLOCKER] OR MODIFY loses UnitID — Fixed in 362e5ca. Both the spec-driven path ( [NEEDS VALIDATION] Tags → resource groups BSON assumption — Validated before this PR was submitted. Python-dumping the All four entries are Studio Pro-created imports. The Medium/minor items — I'll address the unbounded |
…tizeIdent - fetchSpecBytes: wrap resp.Body in io.LimitReader(10 MiB) before ReadAll to prevent memory exhaustion from oversized or malicious spec URLs - sanitizeModuleName: add comment explaining intentional PascalCase vs snake_case difference relative to openapi.sanitizeIdent (service names vs operation names follow different Mendix naming conventions) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Actually, after going over these remarks, I don't believe they're applicable to this scenario. In Studio Pro the OpenAPI import results in a generated Consumed REST Service document with no option to change the input file afterwards — the spec and the service are decoupled after creation, and there is no "refresh from spec" or "update source" operation exposed anywhere in the Studio Pro UI. Given that, A dedicated |
2 participants
Summary
CREATE REST CLIENT Module.Name (OpenAPI: 'path/or/url')auto-generates a consumed REST service document from an OpenAPI 3.0 spec (JSON or YAML)OpenApiFilefor Studio Pro parity (matching what Studio Pro's own OpenAPI import produces)DESCRIBE CONTRACT OPERATION FROM OPENAPI 'path'for previewing the import without writing to the project.mprfile) and HTTP/HTTPS URLs are supportedTesting
make build✅make test✅ (all tests pass)make fmt+make vet✅mxcli check mdl-examples/doctype-tests/20-openapi-import-examples.mdl✅Mendix Studio Pro Validation
Tested imports verified in Mendix Studio Pro —
mx checkreturns 0 errors:tags) match what Studio Pro's built-in OpenAPI import producesOpenApiFile/ContentPascalCase BSON field names match Studio Pro serializationMendix version tested: 10.21.0
Agentic Code Testing
rest-client.md— new Approach 0: OpenAPI Import section)Documentation
docs/01-project/MDL_QUICK_REFERENCE.md— new OpenAPI Import section under Consumed REST Servicesdocs-site/src/examples/rest-integration.md— OpenAPI import as the recommended first stepdocs-site/src/reference/capabilities.md— new capability row.claude/skills/mendix/rest-client.md— Approach 0 section addedCHANGELOG.md— entry under [Unreleased]🤖 Generated with Claude Code