Skip to content

[ruby] fix: build_from_hash drops every attribute inherited from an allOf parent - #24892

Open
wiebren wants to merge 5 commits into
OpenAPITools:masterfrom
wiebren:fix/ruby-build-from-hash-inherited-attributes
Open

[ruby] fix: build_from_hash drops every attribute inherited from an allOf parent#24892
wiebren wants to merge 5 commits into
OpenAPITools:masterfrom
wiebren:fix/ruby-build-from-hash-inherited-attributes

Conversation

@wiebren

@wiebren wiebren commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

A discriminator-based allOf child generates as a real subclass (class Cat < Animal), but
its build_from_hash maps only the child's own attributes. openapi_types and
attribute_map are class methods that dispatch on the child class in every frame of the
ancestry — including inside the parent's build_from_hash — so the parent's attributes are
never transformed out of the wire hash:

  • a required inherited attribute makes deserialization raise
  • an optional inherited attribute comes back silently nil

Executed against the generated petstore sample, current master vs this PR:

Petstore::Cat.build_from_hash({ 'className' => 'Cat', 'color' => 'black', 'declawed' => true })
# master:   ArgumentError: class_name cannot be nil
# this PR:  #<Petstore::Cat class_name="Cat", color="black", declawed=true>

Found while comparing generators against a production registry API: discriminator-mapped
error models came back without their parent's type and message — both optional there, so
the loss was silent.

Why the existing super(attributes) didn't save it

The child's build_from_hash already called super(attributes) — but its return value was
discarded, and because openapi_types/attribute_map dispatch on self (the child) even in
the parent's frame, the call only built a second instance from the child's own attributes and
threw it away. It is removed rather than kept as dead code.

The fix

base_object.mustache walks the ancestry, merging each ancestor's openapi_types and
attribute_map (the child's own declaration wins on a name clash):

types = openapi_types
map = attribute_map
klass = superclass
while klass.respond_to?(:openapi_types)
  types = klass.openapi_types.merge(types)
  map = klass.attribute_map.merge(map)
  klass = klass.superclass
end

This mirrors the acceptable_attribute_map ancestry merge the generated initialize already
uses to accept those keys — the initializers were always willing to set inherited
attributes; build_from_hash just never handed them over.

Tests

  • RubyClientCodegenTest#testBuildFromHashMapsInheritedAttributes — generates the two-level
    Lizard < Reptile < Pet chain from the existing 3_0/allOf_composition_discriminator.yaml
    fixture and asserts the walk is in place. Fails without the template change (verified by
    stashing only the template).
  • spec/custom/base_object_spec.rb in the three petstore samples (typhoeus, faraday, httpx)
    gains a regression case deserializing a Cat that carries its parent's className/color;
    runs green locally.

PR checklist

  • Read the contribution guidelines.
  • Built the project and updated samples (./bin/generate-samples.sh ./bin/configs/ruby*.yaml).
    Every ruby-client model re-renders build_from_hash, hence the broad but mechanical
    sample diff. ruby-nextgen is unaffected — its models do not use build_from_hash.
  • Technical committee: @cliffano @zlx @autopp

Generated with Claude Code


Summary by cubic

Fixes the Ruby client so a discriminator-based allOf child keeps the attributes inherited from its parents in both deserialization and serialization. Previously a required inherited attribute raised on deserialize, an optional one came back silently nil, and a round-tripped model lost its parent's fields again on the way out.

  • build_from_hash and to_hash now walk the ancestry, merging each ancestor's openapi_types, attribute_map, and nullability with the child's own declaration winning on a name clash.
  • An ancestor's nullable flag no longer survives a child redeclaring the attribute as non-nullable.
  • Generated models now require 'set' explicitly, since the nullability walk evaluates eagerly and older rubies may not autoload Set.
  • The discarded-result super(attributes) call is removed.
  • Adds a codegen test on the two-level Lizard < Reptile < Pet fixture and regression specs in the typhoeus, faraday, and httpx petstore samples.

Written for commit 69d6e25. Summary will update on new commits.

Review in cubic

wiebren and others added 2 commits September 7, 2026 16:53
A discriminator allOf child generates as a real subclass, but
build_from_hash iterated only openapi_types/attribute_map - class methods
that dispatch on the child class in every frame of the ancestry, so a
parent's attributes were never mapped from the wire hash: a required
inherited attribute made deserialization raise ArgumentError, an optional
one came back silently nil. The pre-existing super(attributes) call could
not help: its result was discarded, and it iterated the child's own types
anyway.

Walk the ancestry merging each ancestor's openapi_types and attribute_map
(the child wins on a name clash) and drop the discarded-result super
call. RubyClientCodegenTest asserts the walk on the generated two-level
Lizard < Reptile < Pet fixture (fails without the template change), and
the three petstore custom base_object specs now deserialize a Cat that
carries its parent's attributes.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GcwZ1arjLZNpetHz2a3TJz
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GcwZ1arjLZNpetHz2a3TJz

@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 220 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="samples/client/petstore/ruby-faraday/lib/petstore/models/dog.rb">

<violation number="1" location="samples/client/petstore/ruby-faraday/lib/petstore/models/dog.rb:129">
P2: When a consumer subclasses a generated model but overrides its map and initializer, this ancestry walk passes parent fields that the subclass cannot accept. Restrict the walk to generated allOf inheritance or filter the transformed attributes through the receiver’s acceptable attribute map.</violation>
</file>

Note: This PR contains a large number of files. cubic selects up to 200 of the highest-priority eligible files for this review, so some files may not have been reviewed.

Re-trigger cubic

Comment thread samples/client/petstore/ruby-faraday/lib/petstore/models/cat.rb
Comment thread samples/client/petstore/ruby-autoload/lib/petstore/models/cat.rb
Comment thread samples/client/petstore/ruby/lib/petstore/models/cat.rb
map = attribute_map
klass = superclass
while klass.respond_to?(:openapi_types)
types = klass.openapi_types.merge(types)

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: When a consumer subclasses a generated model but overrides its map and initializer, this ancestry walk passes parent fields that the subclass cannot accept. Restrict the walk to generated allOf inheritance or filter the transformed attributes through the receiver’s acceptable attribute map.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At samples/client/petstore/ruby-faraday/lib/petstore/models/dog.rb, line 129:

<comment>When a consumer subclasses a generated model but overrides its map and initializer, this ancestry walk passes parent fields that the subclass cannot accept. Restrict the walk to generated allOf inheritance or filter the transformed attributes through the receiver’s acceptable attribute map.</comment>

<file context>
@@ -118,20 +118,30 @@ def hash
+      map = attribute_map
+      klass = superclass
+      while klass.respond_to?(:openapi_types)
+        types = klass.openapi_types.merge(types)
+        map = klass.attribute_map.merge(map)
+        klass = klass.superclass
</file context>

wiebren and others added 2 commits September 7, 2026 21:27
The serialization direction has the identical defect: attribute_map and
openapi_nullable dispatch on the child class in every ancestor frame, so
the inherited super chain only repeated the child's own attributes and a
round-tripped allOf child lost its parent's fields again on the way out.
Walk the ancestry the same way build_from_hash now does; for a model
without a generated parent the walk does not run and the output is
unchanged.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GcwZ1arjLZNpetHz2a3TJz
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GcwZ1arjLZNpetHz2a3TJz
@wiebren

wiebren commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

cubic's first remark was a real catch, now fixed: to_hash had the identical defect in the serialization direction - attribute_map/openapi_nullable dispatch on the child class in every ancestor frame, so the inherited super chain only repeated the child's own attributes and a round-tripped allOf child lost its parent's fields again on the way out. Verified before the fix: Cat.build_from_hash({className, color, declawed}).to_hash returned {declawed: true}. to_hash now walks the ancestry the same way build_from_hash does; for a model without a generated parent the walk does not run and the output hash is unchanged (same key order included). The petstore custom specs gain the round-trip case, and RubyClientCodegenTest asserts the walk.

On the other remark (a consumer subclass with its own attribute_map and a strict initializer now receiving parent keys): the generated initializers accept exactly the keys the walk maps - acceptable_attribute_map is the same ancestry merge - so every generated chain is consistent. A hand-written subclass that rejects its superclass's attributes in initialize was already producing objects with those fields silently dropped; the existing ArrayMapObject fixture (exactly such a subclass) still passes because its inputs carry none of its parent's keys. I'd rather surface that mismatch than silently drop wire data, but happy to add a filter if a maintainer prefers.

@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 219 files (changes from recent commits).

Note: This PR contains a large number of files. cubic selects up to 200 of the highest-priority eligible files for this review, so some files may not have been reviewed.

Re-trigger cubic

Comment thread samples/client/petstore/ruby-faraday/lib/petstore/models/pet.rb
Comment thread samples/client/petstore/ruby/lib/petstore/models/foo_get_default_response.rb Outdated
Comment thread samples/client/petstore/ruby-faraday/lib/petstore/models/nullable_class.rb Outdated
Comment thread samples/client/petstore/ruby/lib/petstore/models/zebra.rb Outdated
Comment thread samples/client/petstore/ruby-httpx/lib/petstore/models/list.rb Outdated
Comment thread samples/client/petstore/ruby-autoload/lib/petstore/models/client.rb Outdated
…nd Set loads

Two findings from review: the merged-nullable union let a parent's
nullable flag survive a child redeclaring the attribute as non-nullable -
an ancestor's entry now only applies to attributes no nearer class
declares - and the walk evaluates openapi_nullable eagerly for every
to_hash call, so the generated models require 'set' explicitly instead of
leaning on the Set builtin autoload rubies before 3.2 do not have.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GcwZ1arjLZNpetHz2a3TJz
@wiebren

wiebren commented Sep 8, 2026

Copy link
Copy Markdown
Contributor Author

Both of cubic's round-2 findings were real and are fixed:

  • Nullable precedence: the merged-nullable union let an ancestor's nullable flag survive a child redeclaring the attribute as non-nullable. The walk now adds an ancestor's nullable entries only for attributes no nearer class declares - the same nearest-declaration-wins rule openapi_types/attribute_map already follow. The custom specs gain a case pinning both directions (the ancestor's own instance still serializes its explicit nil; the redeclaring child omits it).
  • Set availability: the walk evaluates openapi_nullable on every to_hash call, where the old code only touched it for a nil value - so on rubies before 3.2 (no Set builtin autoload) an SDK loaded without something else requiring set would raise NameError. The generated models now require 'set' themselves.

All ruby samples regenerated; RubyClientCodegenTest pins the corrected walk and the require.

@wing328

wing328 commented Sep 8, 2026

Copy link
Copy Markdown
Member

@wiebren thanks for all the PRs to improve this project.

When you've time, can you please PM me via Slack for a quick discussion on these PRs?

https://join.slack.com/t/openapi-generator/shared_invite/zt-36ucx4ybl-jYrN6euoYn6zxXNZdldoZA

@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 262 files (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. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="samples/client/echo_api/ruby-httpx/lib/openapi_client/models/string_enum_ref.rb">

<violation number="1" location="samples/client/echo_api/ruby-httpx/lib/openapi_client/models/string_enum_ref.rb:14">
P3: `require 'set'` is added unconditionally to model.mustache, so enum models like StringEnumRef also get it, but enums render through partial_model_enum_class.mustache and never use Set (only partial_model_generic's openapi_nullable does). The require is harmless but is dead code in every generated enum file; scope it to the non-enum branch (`{{^isEnum}}`) or move it into partial_model_generic.</violation>
</file>

Note: This PR contains a large number of files. cubic selects up to 200 of the highest-priority eligible files for this review, so some files may not have been reviewed.
Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

=end

require 'date'
require 'set'

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.

P3: require 'set' is added unconditionally to model.mustache, so enum models like StringEnumRef also get it, but enums render through partial_model_enum_class.mustache and never use Set (only partial_model_generic's openapi_nullable does). The require is harmless but is dead code in every generated enum file; scope it to the non-enum branch ({{^isEnum}}) or move it into partial_model_generic.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At samples/client/echo_api/ruby-httpx/lib/openapi_client/models/string_enum_ref.rb, line 14:

<comment>`require 'set'` is added unconditionally to model.mustache, so enum models like StringEnumRef also get it, but enums render through partial_model_enum_class.mustache and never use Set (only partial_model_generic's openapi_nullable does). The require is harmless but is dead code in every generated enum file; scope it to the non-enum branch (`{{^isEnum}}`) or move it into partial_model_generic.</comment>

<file context>
@@ -11,6 +11,7 @@
 =end
 
 require 'date'
+require 'set'
 require 'time'
 
</file context>

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants