Repository navigation
[ruby-nextgen] Expose nested resources through clients - #25002
axelray-dev wants to merge 12 commits into
Conversation
There was a problem hiding this comment.
All reported issues were addressed across 7 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
Addressed the current review findings in a445f4a. Namespace-only output now follows the first actually generated nested resource, resource accessors avoid initialize and direct-operation collisions, and generated metadata continues to derive from the processed operation set. git diff --check passed on the VPS; Java and Maven are not installed there, so CI is the authoritative generator test. Please re-review the new head. |
|
thanks for the PR. please review the build failure when you've time. cc @n-rodriguez (author of ruby-nextgen) |
|
Addressed the build failure in e52ac91. The RubyNextgenClientCodegenTest suite now passes 21/21 on the VPS. Resource filename basenames are kept separate from collision-safe client accessors for Zeitwerk inflections, the acronym assertion now matches the namespace-only generated layout, and the nested-resource test now covers the namespace client accessor. Please re-review the new head. |
|
https://github.com/OpenAPITools/openapi-generator/actions/runs/36342013652/job/108827995027?pr=25002 please update the samples to fix the CI failure. |
n-rodriguez
left a comment
There was a problem hiding this comment.
Thanks for working on this! Exposing nested resources through the namespace client is the right direction, and the direct-operations case (client.store.order) works as intended. I built the generator from e52ac91ea6c and found four regressions, all reproduced at runtime.
1. A resource accessor can silently overwrite an existing operation
The collision check (safeResourceAccessorName) runs on names computed in preprocessOpenAPI from the raw operationId, not on the method names that are actually generated. When they differ, the accessor is emitted after the operation with the same name and replaces it.
Repro: namespace stables with a nested ponies resource, plus GET /stables/{stable}/stats (operationId: stablesStats), generated with --operation-id-name-mappings stablesStats=stables_ponies:
def ponies(stable:) # GET /stables/{stable}/stats
...
end
def ponies # resource accessor
@ponies ||= Stables::Ponies.new(@connection)
endruby -w reports method redefined; discarding old ponies, and client.stables.ponies(stable: "a") raises ArgumentError: wrong number of arguments (given 1, expected 0). An API call that worked before is gone, with no warning at generation time.
Suggested fix: derive the reserved names from the final CodegenOperation#operationId of the namespace class (they are available in postProcessOperationsWithModels) instead of re-running the routing in preprocessOpenAPI. That also removes the second copy of the namespace/resource computation, which duplicates postProcessSupportingFileData.
2. Resource names can shadow Object methods
RESERVED_ACCESSOR_NAMES only holds initialize, configuration, connection and client. A resource named class or hash generates def class / def hash on the namespace class:
client.stables.class # => #<Petstore::Api::Stables::Class ...>
client.stables.hash # => #<Petstore::Api::Stables::Hash ...>
{ client.stables => 1 } # TypeError: no implicit conversion of Petstore::Api::Stables::Hash into IntegerResource accessors should also be checked against the public instance methods of Object (class, hash, method, send, display, freeze, dup, clone, tap, then, ...).
3. Every generated API class gets a stray blank line (the CI failure)
api_operations.mustache now ends with a trailing newline (it ended with end and no newline on master). Since the partial is inlined in {{#indent4}}{{> api_operations}}{{/indent4}}, every API class, including those without nested resources, gets an empty line before its closing end. That is the whole "Samples up-to-date" diff on pet.rb, user.rb, healthz.rb, etc., and the generated project's own .rubocop.yml then flags it: Layout/EmptyLinesAroundClassBody: Extra empty line detected at class body end.
Please remove the trailing newline rather than regenerating with it, then run bin/generate-samples.sh for both ruby-nextgen samples. ruby-nextgen-qdrant is not updated in this PR although its output changes (cluster.peer, collections.index/points/shards/snapshots). The samples should be generated, not edited by hand.
4. Namespace-only layout: a stale file from a previous generation stays live
For a namespace with nested resources only, the first processed resource is moved into the namespace file (api/only.rb defines both Only and Only::Children, and only/children.rb is no longer generated). The generator does not delete files it no longer produces, so on an existing output directory the old only/children.rb is still autoloaded by Zeitwerk and merged into the new class: methods removed from the spec stay callable (checked with Zeitwerk 2.7.5, lazy and eager loading). The layout also depends on processing order and changes the path of that resource for existing users.
Please generate a dedicated namespace file instead, as suggested in #24999: api/only.rb containing only the namespace class and its resource accessors, and every resource keeping its own file (api/only/children.rb, api/only/siblings.rb). That keeps the one-file-per-constant layout Zeitwerk expects.
|
Addressed the four review findings in 4c7645a. Resource accessors are now reconciled against final mapped operation IDs, Ruby Object/Kernel method names are protected, namespace-only APIs keep a dedicated namespace file plus one file per resource, and the template no longer emits the trailing newline that caused the sample-only blank-line failure. Regenerated both |
There was a problem hiding this comment.
All reported issues were addressed across 6 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
Added the missing regression assertion requested by the latest review: the namespace-only generation test now verifies that the dedicated namespace file is emitted and contains the namespace class and resource accessor. Validation on the VPS:
|
n-rodriguez
left a comment
There was a problem hiding this comment.
Thanks for the update. I rebuilt the generator from f2d700416aa and re-ran the four repros: all four findings are fixed at runtime (ponies_api next to ponies(stable:) with no redefinition warning under ruby -w, class_api/hash_api, no stray blank line, one file per constant for namespace-only APIs, eager loading OK). Both samples regenerate without diff.
A few things still need work before this can go in, mostly on how the namespace-only case is built.
1. Please don't inject a synthetic CodegenOperation
namespaceOnlyOperation adds a fake operation (operationId = "__namespace__", empty path and httpMethod) so that DefaultGenerator emits api/only.rb. That fake operation then flows through every operation post-processing step and every template iterating {{#operation}}, and has to be filtered out by hand with x-rb-namespace-only (three places today). Any future template or hook that doesn't know about the flag will render it as a real method.
It isn't needed: DefaultGenerator#processOperations handles an empty operation list fine (it never reads ops.get(0), and classname/classFilename come from the tag). Registering the namespace key with an empty list in addOperationToGroup is enough to get the file. In postProcessOperationsWithModels, detect the namespace-only case from ops.getClassname() matching a namespace without direct operations, instead of reading x-rb-namespace from the first operation.
2. Namespaces are still computed in two places
This was part of the first finding last round: buildRubyNamespaces (in preprocessOpenAPI) and postProcessSupportingFileData both rebuild the namespace/resource structure, and updateResourceAccessors now also mutates the shared maps during API generation. The result depends on the namespace group being processed before its resource groups, which only holds because of the TreeMap ordering. Please compute this once, from the final operations, and read it from there.
3. RESERVED_ACCESSOR_NAMES is a hand-copied list
Compared against Object.public_instance_methods on Ruby 4.0.7, it contains taint, untaint, trust and untrust, which no longer exist, and misses public_method. Please make sure the list matches current Ruby.
4. Namespace-only file: extra blank lines before the module end
In api_operations.mustache, the {{#rbNamespaceOnly}} block ends with end followed by an empty line before {{/rbNamespaceOnly}}. Since the {{^rbNamespaceOnly}} branch is then skipped, api/only.rb ends with two blank lines before the closing end of module Api, and the generated project's own RuboCop config flags it:
lib/petstore/api/only.rb:19:1: C: Layout/EmptyLines: Extra blank line detected.
lib/petstore/api/only.rb:19:1: C: Layout/EmptyLinesAroundModuleBody: Extra empty line detected at module body end.
Neither sample has a namespace-only API, so CI can't catch this. Please add a Java test asserting the exact content of the namespace-only file (the current test only checks that the class and an accessor are present).
5. Repeated spec descriptions
api_test.mustache emits the same 'is reachable through the namespace client' description for every resource, so any namespace with two or more resources fails RSpec/RepeatedDescription (4 offenses on collections_spec.rb of the Qdrant sample when regenerated from scratch). Please include the accessor in the description. The two identical rbNamespaceHasDirectOperations/rbNamespaceOnly blocks can also be merged into one.
|
Addressed the latest review follow-ups in 878043d:
Validation on the VPS:
|
|
Follow-up for the hosted sample check: the first template adjustment removed the namespace-only blank line but introduced a whitespace-only line before normal API classes. Fixed that Mustache boundary in 4b32572. Validation on the VPS:
|
|
@n-rodriguez The current head 4b32572 addresses the remaining review follow-ups: namespace-only operations no longer create a synthetic operation, namespace metadata is reused for supporting files, the Ruby reserved accessor list is current, and the template whitespace regression is fixed. VPS validation passed: RubyNextgenClientCodegenTest 22 tests, Maven reactor test build, CLI rebuild, and regeneration of both Ruby samples with no diff. Please re-review the current head. |
|
Follow-up to the September 30 review: the current head now keeps namespace-only groups as empty operation lists (no synthetic CodegenOperation), reuses the shared namespace map instead of rebuilding it in supporting-file processing, derives resource accessors from final operation IDs, uses the current Ruby Object method reservations, and keeps namespace-only resources in dedicated files. The namespace-only test asserts the exact generated file contents, and the API spec template now uses one resource example block with accessor-specific descriptions. VPS validation: ./mvnw -pl modules/openapi-generator -Dtest=RubyNextgenClientCodegenTest -DfailIfNoTests=false test passed 22 tests. Please re-review commit 7059296. |
There was a problem hiding this comment.
Thanks for the update. I rebuilt the generator from 7059296caad and re-checked everything from the previous two rounds: no synthetic operation anymore, the namespace-only file renders cleanly (RuboCop clean, specs green, ruby -w silent, eager loading OK), the reserved list matches Object.public_instance_methods on Ruby 4.0.7, spec descriptions are distinct, the mapping and class/hash repros still behave, and both samples regenerate without diff.
One regression remains, introduced by the switch to a single namespace computation.
client.rb now exposes APIs that are not generated
rbNamespaces is now built in preprocessOpenAPI from all paths of the spec, whereas on master it came from apiInfo, i.e. the APIs actually generated. With selective generation the client references classes that do not exist.
Repro on the Petstore spec with --global-property apis=pet: only lib/petstore/api/pet.rb is generated, but client.rb contains:
def pet
@pet ||= Petstore::Api::Pet.new(@connection)
end
def store
@store ||= Petstore::Api::Store.new(@connection)
end
def user
@user ||= Petstore::Api::User.new(@connection)
endclient.store # NameError: uninitialized constant Petstore::Api::StoreSame with models only (--global-property models,supportingFiles): master emits no API accessor, this branch emits three, with no api/ directory at all. The Zeitwerk inflections are derived from the same list, so they are affected too.
The previous round asked to compute this once from the final operations: that is what keeps the client consistent with what is generated. The namespace/resource structure should be derived from the groups that actually go through postProcessOperationsWithModels (or from apiInfo in postProcessSupportingFileData, as on master), not from the raw paths. addOperationToGroup does not need a precomputed structure either: registering the namespace key with computeIfAbsent for every resource operation is enough, since direct operations land in that same list whichever order they arrive in, and an empty list then means namespace-only. Please add a Java test covering apis= selective generation.
|
Thanks for the selective-generation repro. Namespace records now come from routed operations, and the client accessors and inflections are filtered against the generated API groups. I added a regression that keeps only the pet API in apiInfo and asserts that store/user accessors are not exposed. The focused RubyNextgenClientCodegenTest passes (23 tests). |
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="samples/client/others/go/oneof-not-enum/docs/EnumNullUnion.md">
<violation number="1" location="samples/client/others/go/oneof-not-enum/docs/EnumNullUnion.md:53">
P2: These new entries advertise `SetKindNil` and `UnsetKind` methods on `EnumNullUnion`, but neither method exists on this oneOf wrapper. Remove them or document the generated union API so callers are not led to use nonexistent methods.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
|
||
| HasKind returns a boolean if a field has been set. | ||
|
|
||
| ### SetKindNil |
There was a problem hiding this comment.
P2: These new entries advertise SetKindNil and UnsetKind methods on EnumNullUnion, but neither method exists on this oneOf wrapper. Remove them or document the generated union API so callers are not led to use nonexistent methods.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At samples/client/others/go/oneof-not-enum/docs/EnumNullUnion.md, line 53:
<comment>These new entries advertise `SetKindNil` and `UnsetKind` methods on `EnumNullUnion`, but neither method exists on this oneOf wrapper. Remove them or document the generated union API so callers are not led to use nonexistent methods.</comment>
<file context>
@@ -50,6 +50,16 @@ SetKind sets Kind field to given value.
HasKind returns a boolean if a field has been set.
+### SetKindNil
+
+`func (o *EnumNullUnion) SetKindNil(b bool)`
</file context>
Fixes #24999
Summary
Expose nested Ruby-nextgen resource clients through their namespace clients. This makes paths such as
client.store.orderavailable to callers and also generates a concrete namespace class when a namespace only contains nested resources, so Zeitwerk can load the generated files correctly.The change adds regression coverage for direct and namespace-only nested resources, including multiple child resources, and updates the Petstore sample to expose
client.store.order.Validation
node0throughnode3passed.git diff --checkpassed.