[BAC-1482] Add read_only attributes and request-variant schema generation - #24
Closed
HolyWalley wants to merge 1 commit into
Closed
[BAC-1482] Add read_only attributes and request-variant schema generation#24HolyWalley wants to merge 1 commit into
HolyWalley wants to merge 1 commit into
Conversation
…tion One entity class serves as both request and response body, so server-owned fields (id, balance, phone_numbers, caller_ids) appear settable in the generated TypeScript client. Emitting OpenAPI `readOnly: true` is necessary but not sufficient: @hey-api/openapi-ts maps it to a TypeScript `readonly` property *modifier* on a single shared type. That prevents reassignment, not inclusion in an object literal — it would not even stop a client sending the field. So the gem now emits a second component per schema, the request variant, with read-only properties omitted. - `read_only: true` on `attribute`, intercepted before ActiveModel forwards options to Type.lookup (which raises ArgumentError on unknown keywords). - `json_schema_ref(:request)` / `as_json_schema(variant: :request)`. The no-argument path is unchanged. - The variant propagates transitively: a request variant that still refs the full nested component achieves nothing, since read-only fields come back through the ref. - `json_schema_variants` returns exactly the components a spec must emit, so nothing unreferenced lands in the generated client. `json_schema_id` returns the base id when a subtree declares no read-only attribute anywhere, which is what keeps a clean subtree from emitting a pointless variant and keeps `json_schema_ref(:request)` off a dangling $ref. `read_only_subtree?` recomputes per call rather than memoizing: memoizing an in-progress node as false is a real false negative on cycles. readOnly is wrapped in allOf on $ref properties, mirroring the existing make_schema_nullable!, because an OpenAPI 3.0 sibling of $ref is ignored. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RFiA6kAjnUJ7t2dwWwWrjT
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes BAC-1482. Blocks BAC-1481.
Why
One entity class serves as both request and response body, so server-owned fields (
id,balance,phone_numbers,caller_ids) appear settable in the generated TypeScript client.Emitting OpenAPI
readOnly: trueis necessary but not sufficient.@hey-api/openapi-ts(0.64.5, and 0.99.0 checked) maps it toaccessScope: 'read'and renders a TypeScriptreadonlyproperty modifier on a single shared type. Areadonlymodifier prevents reassignment, not inclusion in an object literal — it would not even stop a client sending the field. So the gem emits a second component per schema: the request variant, with read-only properties omitted.What
read_only: trueonattribute, intercepted before ActiveModel forwards options toType.lookup(which raisesArgumentErroron unknown keywords for every symbolic type).json_schema_ref(:request)/as_json_schema(variant: :request); the no-argument path is unchanged.json_schema_variantsreturns exactly the components a spec must emit, so nothing unreferenced reaches the generated client.Design notes
$ref.json_schema_idreturns the base id when nothing in the subtree declares a read-only attribute. One line covers both requirements.read_only_subtree?recomputes per call rather than memoizing — memoizing an in-progress node asfalseis a real false negative on cycles. Two alternative designs built a graph subsystem (backward-BFS fixpoint; a process-wide registry) to avoid this; fuzzing 12,000 node queries against ground truth showed the failure mode only bites a memoized formulation, so the subsystem bought nothing.readOnlyon$refproperties is wrapped inallOf, mirroring the existingmake_schema_nullable!— an OpenAPI 3.0 sibling of$refis ignored, which is exactly the ticket's motivating case.-Request, hyphenated deliberately: a hyphen cannot appear in a Ruby constant path, so a variant id can never collide with a real inner class in the flatcomponents/schemasnamespace. A dotted.Requestwould silently clobberAPI::V1::Businesses::Update::Request.Testing
74 examples, 0 failures; rubocop clean, no
rubocop:disableand no.rubocop.ymledits. The 50 pre-existing examples pass unedited — including the byte-identity assertion inschemas/json_spec.rb, which is the guard that the no-argument path did not shift.Covered: flat entity; nested child with read-only; nested child without (no variant emitted); cycles; arrays-of-entities; multi-word attribute names; inheritance;
descinteraction; unknown-variantKeyError.Two spec cases exist specifically because mutation testing showed the fixtures could not otherwise detect a broken implementation — single-word attributes hid snake_case/camelCase pruning bugs, and the original cycle fixture short-circuited before memoization could poison anything. Both now fail against those mutants.
Not in this PR
karen-side verification (bump the pinned rev, regenerate swagger under
RAILS_ENV=test,npm run oapi) — the ticket asks that this land on its own first, with the no-marker diff proving nothing moved, before BAC-1481 starts marking attributes.No version bump: karen pins this gem by rev, not version.
Note for reviewers
The suite does not load on Ruby 4.0.x at all — activesupport 7.1.3.4 assumes stdlib gems Ruby 4 ejected. CI is on 3.3.0 and unaffected;
Gemfile/Gemfile.lockare untouched here. Run locally withASDF_RUBY_VERSION=3.3.0 bundle exec rspec. Worth fixing separately.🤖 Generated with Claude Code
https://claude.ai/code/session_01RFiA6kAjnUJ7t2dwWwWrjT