fix: replace parameter-group hashes with typed variant classes#473
fix: replace parameter-group hashes with typed variant classes#473gjtorikian merged 7 commits intomainfrom
Conversation
`UserManagement#create_user` and `#update_user` raised
`TypeError: no implicit conversion of Symbol into Integer` whenever a
caller passed `password:` because the generated method advertised
`password:` as a flat `String` kwarg AND ran a hash dispatcher
(`password[:type]`) on it. The String/hash collision was a fluke of the
spec — `password` happens to be both the group name and one of its
variant leaf params — but it bypassed the SDK before the request hit the
wire.
Each `x-mutually-exclusive-(body|parameter)-groups` group now emits a
`Data.define` variant class per variant under `lib/workos/`, with
matching Sorbet sigs in `rbi/workos/`. Method signatures take
`T.any(VariantA, VariantB)` and dispatch on class identity instead of a
`:type` symbol. This matches how Python, Kotlin, .NET, and PHP already
expose these groups.
BREAKING: callers that previously passed a hash with a `:type`
discriminator must instantiate the variant class:
# Before
create_user(email: "…", password: { type: "plaintext", password: "x" })
authorization.check(…, resource_target: { type: "by_id", resource_id: "r" })
# After
create_user(email: "…", password: WorkOS::PasswordPlaintext.new(password: "x"))
authorization.check(…, resource_target: WorkOS::ResourceTargetById.new(resource_id: "r"))
Callers that passed leaf params as flat kwargs (e.g. `resource_id:`,
`parent_resource_id:`, `password_hash:`) must move them into the variant
constructor. Those kwargs were redundant for required groups and silently
let optional-group calls violate the spec's mutual-exclusivity contract.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Greptile SummaryThis PR replaces hash-based discriminated union parameters (e.g. Confidence Score: 5/5Safe to merge — the core TypeError fix is correct, all variant dispatch paths are well-tested, and the previously-flagged body-missing P1 in No P0 or P1 findings remain in the HEAD code. The previously noted No files require special attention. Important Files Changed
|
Operations whose body is exclusively managed by a parameter group (currently `update_organization_membership` via the `role` group) were having their body silently dropped. After filtering group-leaf params from `bodyFields`, `bodyFields.length === 0` and the `hasBody` gate went false, so neither the body hash nor the dispatcher were emitted and `body:` was missing from the request kwargs entirely. Any `role:` the caller passed went nowhere. Mirrors the existing query-side condition (`hasQuery = qEntries.length > 0 || groupsGoToQuery`): emit the body when there are non-group body fields OR a parameter group dispatches into the body. Caught by Greptile review on workos/workos-ruby#473. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The previous regen dropped the request body from `update_organization_membership` because its body is exclusively managed by the `role` parameter group, and the new variant-class filter left `bodyFields` empty — which the emitter (incorrectly) treated as "no body at all," skipping both the dispatcher and the `body:` request kwarg. Any `role:` argument was silently lost. Caught by Greptile review. Emitter fix in workos/oagen-emitters#66 (commit b87038b). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@greptile review |
Regenerated tests now pass optional parameter groups (e.g. `role:`, `password:`, `parent_resource:`) and assert the wire body via webmock's `hash_including` matcher for any operation whose body is constructed by a group dispatcher. Catches silent-drop regressions like the one fixed in 154b350 — without this, `test_update_organization_membership` was calling with `id:` only, never exercising the role dispatcher. Emitter change: workos/oagen-emitters@a8bba55 on workos/oagen-emitters#66. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Regenerated against oagen-emitters fix that stops the variant emitter from inheriting the parent group's nullability. `PasswordPlaintext.password` is now `String` instead of `T.nilable(String)`, so callers can no longer construct `WorkOS::PasswordPlaintext.new(password: nil)` and emit a literal `"password": null`. Caught by Greptile review. Emitter fix in workos/oagen-emitters@83cd567. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@greptile review |
Regenerated against three oagen-emitters fixes: - workos/oagen-emitters@8379c04: every `case prop / when WorkOS::Variant` dispatcher in user_management and authorization gains an `else raise ArgumentError` arm. Plain Ruby callers that pass a stale hash, a `nil`, or an unknown variant now fail fast locally with the expected variant classes named, instead of silently sending a request with the dispatcher's wire fields missing and getting a less-helpful 4xx. - workos/oagen-emitters@a1be4c9: ~10 dead `body = {}.compact` and `body = { "x" => x }.compact` calls disappear from operations whose body is built entirely from required kwargs. Cosmetic only. - workos/oagen-emitters@a53154c: 12 new `test_<method>_with_<variant>_returns_expected_result` tests cover the second/third arm of every parameter-group dispatcher (ResourceTargetByExternalId, ParentResourceByExternalId, ParentByExternalId, PasswordHashed, RoleMultiple). Body matchers track the selected variant's wire-name leaves via `hash_including`. bundle exec rake test: 870 runs / 4395 assertions / 0 failures (up from 858 / 4383). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Regenerated against workos/oagen-emitters@65932a5: variant classes now live inside their owning resource class instead of at WorkOS top level — matching how Python exposes them under `workos.<resource>`: Before After WorkOS::PasswordPlaintext → WorkOS::UserManagement::PasswordPlaintext WorkOS::PasswordHashed → WorkOS::UserManagement::PasswordHashed WorkOS::RoleSingle → WorkOS::UserManagement::RoleSingle WorkOS::RoleMultiple → WorkOS::UserManagement::RoleMultiple WorkOS::ResourceTargetById → WorkOS::Authorization::ResourceTargetById WorkOS::ResourceTargetByExternalId → WorkOS::Authorization::ResourceTargetByExternalId WorkOS::ParentById → WorkOS::Authorization::ParentById WorkOS::ParentByExternalId → WorkOS::Authorization::ParentByExternalId WorkOS::ParentResourceById → WorkOS::Authorization::ParentResourceById WorkOS::ParentResourceByExternalId → WorkOS::Authorization::ParentResourceByExternalId 20 separate top-level files (10 .rb + 10 .rbi) are deleted; their contents now appear as inline `Data.define` constants and inline RBI class blocks at the top of `lib/workos/<service>.rb` and `rbi/workos/<service>.rbi`. Inline placement is required by Zeitwerk: `lib/workos.rb` calls `loader.collapse("workos/<service>")`, which flattens subdirectories so a separate file at `lib/workos/user_management/password_plaintext.rb` would still resolve to `WorkOS::PasswordPlaintext` (the very name we're trying to move away from). Defining the constants directly inside the already-existing `class UserManagement` body sidesteps the collapse and matches Python's per-resource layout. bundle exec rake test: 870 runs / 4395 assertions / 0 failures. This is breaking on top of the parameter-group refactor in 5d4fe45 — that one moved callers from hashes to typed variant classes; this one moves the same classes from `WorkOS::Foo` to `WorkOS::Service::Foo`. Both are part of the same major-version cut. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Regenerated against workos/oagen-emitters@e56acd8: test stubs for parameter-group variants now use the recovered (body-model) type instead of the raw IR leaf type, and array stubs produce single-element arrays instead of empty `[]`. Concretely fixes the RoleMultiple coverage gap flagged in review: Before @client.user_management.create_organization_membership( ..., role: WorkOS::UserManagement::RoleMultiple.new(role_slugs: "stub") ) .with(body: hash_including("role_slugs" => "stub")) After @client.user_management.create_organization_membership( ..., role: WorkOS::UserManagement::RoleMultiple.new(role_slugs: ["stub"]) ) .with(body: hash_including("role_slugs" => ["stub"])) The `RoleMultiple` class declares `role_slugs: T::Array[String]`; the old stub passed a String. Data.define doesn't validate at construction so the test passed, but the wire body the SDK was sending didn't match the API contract — and a regression that flipped serialization to a (correct) array would have failed that test. Other touched files (audit_logs.create_schema targets, webhooks.create_webhook_endpoint events) get the same single-element-array treatment for array body fields. bundle exec rake test: 870 runs / 4395 assertions / 0 failures. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(ruby): emit typed variant classes for parameter groups
The Ruby emitter exposed `x-mutually-exclusive-(body|parameter)-groups`
groups as a hash with a `:type` symbol discriminator. When a group's
name happened to match one of its variant leaf params (the spec's
`password` group has a `password` leaf in the `plaintext` variant), the
emitter generated a flat `String` kwarg that shadowed the group
dispatcher's hash kwarg in the method signature. Calling
`UserManagement#create_user(password: …)` then ran `password[:type]`
against a String and raised `TypeError: no implicit conversion of Symbol
into Integer` — reported by Mentimeter on workos-ruby 7.x.
Two fixes combined:
1. Filter `bodyFields` through `collectGroupedParamNames` everywhere
`bodyFields` is iterated in `ruby/resources.ts` and `ruby/rbi.ts`.
Python, Go, PHP, .NET, and Kotlin already do this — Ruby was the
outlier, and the leaks let callers bypass the spec's
mutual-exclusivity contract for non-colliding groups too.
2. Replace the hash-with-`:type` API surface with typed variant classes,
matching every other language emitter:
- new `ruby/parameter-groups.ts` emits one `Data.define(...)` class
per (group, variant) under `lib/workos/<variant>.rb`, plus a
Sorbet `.rbi` shim.
- `ruby/resources.ts` dispatcher: `case prop[:type] when "x"` →
`case prop when WorkOS::VariantClass`; field reads switch from
hash subscripts to Data accessors.
- `ruby/rbi.ts` group kwarg type: `T::Hash[Symbol, T.untyped]` →
`T.any(WorkOS::VariantA, WorkOS::VariantB)` (nilable for
optional groups). Group kwargs were also entirely missing from
`.rbi` sigs before — now emitted.
- `ruby/tests.ts` stub: `{ type: "by_id" }` → `WorkOS::VariantClass.new(...)`.
- YARD docs: variant union in the type bracket; one-line description.
This is a breaking change for SDK callers (separate PR in workos-ruby
captures the regenerated output and migration notes). All 388 emitter
tests pass; regenerated workos-ruby suite is 858 runs / 4383 assertions
green.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(ruby): emit body when only a parameter group dispatches into it
Operations whose body is exclusively managed by a parameter group
(currently `update_organization_membership` via the `role` group) were
having their body silently dropped. After filtering group-leaf params
from `bodyFields`, `bodyFields.length === 0` and the `hasBody` gate
went false, so neither the body hash nor the dispatcher were emitted
and `body:` was missing from the request kwargs entirely. Any `role:`
the caller passed went nowhere.
Mirrors the existing query-side condition (`hasQuery = qEntries.length
> 0 || groupsGoToQuery`): emit the body when there are non-group body
fields OR a parameter group dispatches into the body.
Caught by Greptile review on workos/workos-ruby#473.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* test(ruby): assert request body for parameter-group dispatchers
Tests previously stubbed the URL but never verified the wire body, so
the silent-drop bug fixed in b87038b — `update_organization_membership`
shipping a PUT with no body when only the `role` group managed it —
slipped past the suite. Two changes catch this class of regression:
- `buildCallArgsStub` now passes optional parameter groups too, not
just required ones. The old behavior left the dispatcher code path
unexercised whenever a group was optional, hiding the bug entirely.
- New `buildBodyMatcher` emits a webmock `.with(body: hash_including(...))`
matcher whenever an operation has any parameter group dispatched into
the body. The matcher includes required non-group body fields plus
the first variant's wire-name leaves, so any mismatch (missing body,
dropped variant leaf, wrong wire name) fails the test.
Verified by temporarily reverting b87038b: the regenerated
`test_update_organization_membership` errors with
`WebMock::NetConnectNotAllowedError` because the request body is empty
and doesn't match the stub. Restoring the fix makes it pass.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(ruby): drop spurious nullable on variant leaf params
The parameter-group variant emitter was over-applying its body-model
fallback. When a variant leaf param shared a name with the parent group
field (e.g. `password` inside the `password` group's `passwordPlaintext`
variant), the lookup picked up the body's group-slot type — which is
nullable when the group is optional — and clobbered the IR's leaf type.
That produced `T.nilable(String)` for `PasswordPlaintext.password`,
letting callers construct `WorkOS::PasswordPlaintext.new(password: nil)`
and emit a literal `"password": null` in the request body.
Restrict the body fallback to its original purpose: recovering structure
when the IR loses array/enum/model/map fidelity. Strip outer nullable
from the body fallback, since body nullability reflects parent-group
optionality, not the leaf's required-ness within the variant.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(ruby): raise ArgumentError on unmatched parameter-group dispatch
Every generated `case prop / when WorkOS::Variant` block lacked an `else`
arm, so a caller passing anything other than a known variant instance —
a stale hash from migration code, a `nil` slipping past where the kwarg
is required, or a future variant the gem hasn't been regenerated for —
would silently send a request with the variant's wire fields missing.
The API would then reject it with a less-helpful error than a local
ArgumentError would have produced.
Now every dispatcher (both query-side for GET/DELETE and body-side for
POST/PUT/PATCH) raises with the expected variant classes and the actual
class the caller passed:
raise ArgumentError, "expected resource_target to be one of: " \
"WorkOS::ResourceTargetById, WorkOS::ResourceTargetByExternalId, " \
"got #{resource_target.class}"
Sorbet-typechecked callers were already protected by `T.any(...)` sigs;
this turns the silent corruption case into fail-fast for plain Ruby.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(ruby): omit .compact on hash literals with no nilable entries
The Ruby emitter unconditionally appended `.compact` to every `params` /
`body` hash literal, even when every contributing entry was a required
kwarg or a static default — leaving dead code like `body = {}.compact`
and `body = { "client_id" => client_id }.compact` after the variant-
class refactor filtered group leaves out of the literal.
Now `.compact` is emitted only when at least one entry can resolve to
nil. For query, that means at least one `qEntry` is from an optional
`queryParam`. For body, that means at least one `bodyField` is optional
— `defaults` and `inferFromClient` always resolve to non-nil values, so
they don't trigger compaction.
Cosmetic, but the regenerated SDK loses ~10 dead `.compact` calls.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* test(ruby): emit per-variant test cases for parameter-group dispatchers
Generated tests for operations with `x-mutually-exclusive-(body|parameter)
-groups` only exercised the first variant of each group — `ResourceTargetById`,
`ParentResourceById`, `PasswordPlaintext`, `RoleSingle` — leaving the
second/third arm of every dispatcher untested. A wrong wire-name mapping
in (e.g.) `ResourceTargetByExternalId` would not be caught locally; it
would only surface when an integration test or production caller hit that
specific shape.
`buildCallArgsStub` and `buildBodyMatcher` now accept a
`variantOverrides: Map<groupName, variantIndex>` and the test emitter
loops over each non-first variant, emitting one extra
`test_<method>_with_<group>_<variant>_returns_expected_result` per
variant. Body matchers stay in sync via `hash_including(...)` over the
selected variant's wire-name leaves, so a wrong attr accessor in the
generated dispatcher will fail the request body match.
Adds 12 tests to the regenerated workos-ruby covering
ResourceTargetByExternalId, ParentResourceByExternalId, ParentByExternalId,
PasswordHashed, and RoleMultiple across authorization and user_management.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(ruby): scope parameter-group variant classes under their resource module
Variant classes were emitted at top level — `WorkOS::PasswordPlaintext`,
`WorkOS::ResourceTargetById` — claiming generic names in the global
WorkOS namespace. Two issues:
1. Cross-language inconsistency. The variant-class refactor (a1da163)
justified itself as "in line with Python, Kotlin, .NET, PHP," but
Python actually scopes these under the resource module —
`workos.user_management.PasswordPlaintext`. Ruby was the outlier.
2. Namespace shallowness. `WorkOS::ParentById` vs
`WorkOS::ParentResourceById` differ only by a token but represent
semantically identical "by id" groups in different operations. As
more parameter groups land, collisions become a question of when,
not if.
Variants now live as inlined `Data.define` constants under their owning
resource class — `WorkOS::UserManagement::PasswordPlaintext`,
`WorkOS::Authorization::ResourceTargetById`. Inline placement is
required: `lib/workos.rb` calls `loader.collapse("workos/<service>")`,
which flattens subdirectories so a separate file at
`lib/workos/user_management/password_plaintext.rb` would resolve to
`WorkOS::PasswordPlaintext` (same name we're trying to move away from).
The resource file already declares `class UserManagement` — we just
add the variant constants inside that class body, mirroring the Python
layout exactly.
Implementation:
- `parameter-groups.ts` exposes `buildGroupOwnerMap`,
`collectVariantsForMountTarget`, `emitInlineVariantClass`,
`emitInlineVariantRbi`. The previous top-level file emitters are
deleted along with two unused union helpers.
- Each group has a single canonical owner (first mount target
alphabetically), so dispatchers in other resources reference the
same class — no cross-mount-target duplicates.
- `resources.ts` and `rbi.ts` each call into parameter-groups to
inline variant blocks at the top of their service class body.
- `naming.ts` adds `scopedGroupVariantClassName` for fully-qualified
references in dispatcher / RBI / test code.
Regenerated workos-ruby loses 20 separate top-level files and recovers
their content inline. SDK suite: 870 runs / 4395 assertions / 0 failures.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(ruby): recover variant param types when stubbing tests
Generated tests for `RoleMultiple` were calling
`WorkOS::UserManagement::RoleMultiple.new(role_slugs: "stub")` and
asserting via `hash_including("role_slugs" => "stub")` — both wrong:
the variant class declares `role_slugs: T::Array[String]`, and the wire
body the API expects is `["stub"]`. The test passed locally because
`Data.define` doesn't enforce types at construction, but the SDK was
serializing a String into a slot the API contract calls an Array. A
regression that flipped the body to a (correct) array would have failed
this test, defeating the matcher commit (205b77e).
Two fixes in the test emitter:
1. Variant param stubs now go through `pickVariantParamType` (newly
exported from parameter-groups.ts) so the stub matches the type the
variant class actually declares — not the IR's bare-primitive leaf
type. The same recovery already runs when the variant class is
emitted; tests just weren't using it.
2. `stubValueFor` for `array` types now returns `[<inner>]` instead of
`[]`. An empty array under `hash_including` matches a body with
`"key": []` on the wire, which would hide regressions that
serialize the wrong element type. Single-element arrays exercise
the actual shape.
Touches one of the regenerated tests directly (RoleMultiple) plus a few
unrelated tests for endpoints with array body fields (audit_logs.create_schema
targets, webhooks.create_webhook_endpoint events) — all stubs now exercise
non-empty arrays.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
Fixes a
TypeError: no implicit conversion of Symbol into Integerthat any caller hits when passingpassword:toUserManagement#create_useror#update_user. The generated method advertisedpassword:as a flatStringkwarg and ran a hash dispatcher (password[:type]) on it. The String/Hash collision was a fluke of the spec —passwordhappens to be both the group name and one of its variant leaf params — but it bypassed the SDK before the request reached the wire.Each
x-mutually-exclusive-(body|parameter)-groupsgroup now emits a typed variant class per variant underlib/workos/, with matching Sorbet sigs inrbi/workos/. Method signatures takeT.any(VariantA, VariantB)and dispatch on class identity instead of a:typesymbol. This brings the Ruby SDK in line with how Python, Kotlin, .NET, and PHP already expose these groups.Breaking changes
Callers that passed a hash with a
:typediscriminator must instantiate the variant class:Callers that passed variant leaf params as flat kwargs (e.g.
resource_id:,parent_resource_id:,password_hash:) must move them into the variant constructor. Those kwargs were silent dead weight for required groups, and let optional-group calls violate the spec's mutual-exclusivity contract.Generated changes
lib/workos/andrbi/workos/for the eight variant classes (PasswordPlaintext,PasswordHashed,ResourceTargetById,ResourceTargetByExternalId,ParentResourceById,ParentResourceByExternalId,ParentById,ParentByExternalId,RoleSingle,RoleMultiple).lib/workos/{user_management,authorization}.rbrewritten tocase prop when WorkOS::VariantClass..rbisigs useT.any(...)instead of the previousT::Hash[Symbol, T.untyped].test/workos/test_authorization.rbregenerated to call the variant constructors.The corresponding emitter changes live in
oagen-emitters(separate PR).Test plan
bundle exec rake test— 858 runs / 4383 assertions / 0 failures🤖 Generated with Claude Code