Skip to content

[ruby-nextgen] Expose nested resources through clients - #25002

Open
axelray-dev wants to merge 12 commits into
OpenAPITools:masterfrom
axelray-dev:fix/ruby-nextgen-nested-resources-24999
Open

axelray-dev wants to merge 12 commits into
OpenAPITools:masterfrom
axelray-dev:fix/ruby-nextgen-nested-resources-24999

Conversation

@axelray-dev

@axelray-dev axelray-dev commented Sep 23, 2026 •

Copy link
Copy Markdown

Fixes #24999

Summary

Expose nested Ruby-nextgen resource clients through their namespace clients. This makes paths such as client.store.order available 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

  • CircleCI node0 through node3 passed.
  • git diff --check passed.
  • The focused Maven test could not run on the VPS because Java is not installed; the generator test suite is covered by CI.

@axelray-dev
axelray-dev marked this pull request as ready for review September 24, 2026 20:50

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 7 files

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 1 file (changes from recent commits).

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

@axelray-dev

Copy link
Copy Markdown
Author

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.

@wing328

wing328 commented Sep 25, 2026

Copy link
Copy Markdown
Member

thanks for the PR. please review the build failure when you've time.

cc @n-rodriguez (author of ruby-nextgen)

@wing328 wing328 modified the milestone: 7.26.0 Sep 25, 2026
@axelray-dev

Copy link
Copy Markdown
Author

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.

@wing328

wing328 commented Sep 28, 2026

Copy link
Copy Markdown
Member

@n-rodriguez n-rodriguez left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
end

ruby -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 Integer

Resource 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.

@axelray-dev

Copy link
Copy Markdown
Author

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
uby-nextgen Petstore and
uby-nextgen-qdrant samples; the Qdrant diff is limited to the expected cluster.peer and collections.index/points/shards/snapshots accessors. Validation on the VPS: RubyNextgenClientCodegenTest 22/22 passed, reactor build for the generator CLI passed, both sample-generation commands passed, and git diff --check passed. Please re-review the new head.

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@axelray-dev

Copy link
Copy Markdown
Author

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:

  • RubyNextgenClientCodegenTest: 22 passed
  • Maven reactor test build: passed
  • git diff --check: passed

@wing328 wing328 added this to the 7.26.0 milestone Sep 30, 2026

@n-rodriguez n-rodriguez left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@axelray-dev

Copy link
Copy Markdown
Author

Addressed the latest review follow-ups in 878043d:

  • Removed the synthetic namespace-only CodegenOperation; empty namespace groups now render the namespace file directly.
  • Reused the shared namespace metadata instead of rebuilding it during supporting-file processing.
  • Updated the Ruby reserved accessor list for current Ruby, including public_method and removing obsolete names.
  • Removed the namespace-only trailing blank line and added an exact-content regression assertion.
  • Made generated RSpec descriptions unique per resource.

Validation on the VPS:

  • RubyNextgenClientCodegenTest: 22 passed
  • Maven reactor test build: passed
  • git diff --check: passed

@axelray-dev

Copy link
Copy Markdown
Author

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:

  • RubyNextgenClientCodegenTest: 22 passed
  • Rebuilt the CLI with the final templates
  • Regenerated both ruby-nextgen samples; working tree remains unchanged
  • git diff --check: passed

@axelray-dev

Copy link
Copy Markdown
Author

@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.

@axelray-dev

Copy link
Copy Markdown
Author

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.

@n-rodriguez n-rodriguez left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
end
client.store # NameError: uninitialized constant Petstore::Api::Store

Same 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.

@axelray-dev

Copy link
Copy Markdown
Author

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).

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>

@wing328 wing328 modified the milestones: 7.26.0, 7.27.0 Oct 6, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG][RUBY-NEXTGEN] Nested resources are not reachable from the client

3 participants