feat!: rebuild the client on API specification 2.26.1 - #30
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
All alerts resolved. Learn more about Socket for GitHub. This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. |
a13c24f to
a6437f5
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a13c24f379
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| ```shell | ||
| gem install ./flat_api-0.3.0.gem | ||
| client = FlatApi::FlatClient.new(access_token: 'YOUR_TOKEN') |
There was a problem hiding this comment.
Following this newly published setup raises NameError: a repo-wide search finds no FlatApi::FlatClient definition, while the package root exports only the generated *Api classes. Consequently, every user following either the README or QUICKSTART fails before making a request; implement and export this wrapper or document the actual Configuration/ApiClient setup.
Useful? React with 👍 / 👎.
| page_params = cursor ? params.merge(next: cursor) : params | ||
| data, _status, headers = fetch_page.call(page_params) | ||
| Array(data).each { |item| yielder << item } | ||
|
|
There was a problem hiding this comment.
_next option
For any response containing a next-page link, this passes the cursor as :next, but the generated paginated methods only copy opts[:_next] into the next query parameter (for example, CollectionApi#list_collections_with_http_info). The second iteration therefore repeats the first request, yields duplicate items, and then stops when seen encounters the same cursor instead of traversing subsequent pages.
Useful? React with 👍 / 👎.
| fail ApiError.new(code: 0, | ||
| message: response.return_message) | ||
| else | ||
| fail FlatApi.error_from_response( |
There was a problem hiding this comment.
When an idempotent request receives a 5xx response or the Flat-specific rate-limit 403, this path immediately raises the typed error. RetryPolicy is never referenced from ApiClient or elsewhere, so its attempt count, backoff, and reset handling are dead code and the automatic retries advertised in the README and changelog never occur.
Useful? React with 👍 / 👎.
| def access_token | ||
| @tokens.access_token | ||
| end |
There was a problem hiding this comment.
When expires_at has passed, access_token still returns the expired token without consulting Tokens#expired? or invoking refresh; neither expiry check is used anywhere else in the repository. Even if a caller installs this method as Configuration#access_token_getter, requests continue using the stale bearer token and receive 401 responses rather than getting the promised transparent refresh.
Useful? React with 👍 / 👎.
|
|
||
| # reconstruct content | ||
| content = stream.join | ||
| content = content.unpack('m').join if response.headers['Content-Transfer-Encoding'] == 'binary' |
There was a problem hiding this comment.
When a file endpoint responds with Content-Transfer-Encoding: binary, the payload already consists of raw octets, but unpack('m') treats it as base64 and decodes it. Such score exports, OMR files, or class exports are therefore returned corrupted; base64 decoding should only occur for a base64 transfer encoding.
Useful? React with 👍 / 👎.
a6437f5 to
cdeea25
Compare
The 0.3.x line was generated in 2024 and has drifted well past usefulness: it
misses every operation the API has shipped since, including OMR, and it offers
nothing above the raw generated surface. This replaces it with a generated
client that is reproducible from this repository alone, plus the ergonomic
layer the README has always claimed.
Regenerated with openapi-generator 7.24.0 (ruby, faraday) against API
specification 2.26.1: 123 operations, 196 models, 23 scopes.
Four defects that no check in this repository would have caught, all found by
installing the gem and making real requests:
* The gemspec declared typhoeus while tools/openapi-config.json selects the
faraday library. `gem install flat_api` pulled a dependency the code never
loads, and did not pull the ones it does. Now declares faraday,
faraday-multipart and marcel.
* The gem could not be loaded at all. The 200 response of
createLtiConfiguration was an inline allOf over LtiConfiguration, which is
a oneOf. This generator emits a oneOf as a module and an allOf as a
subclass, so the output read `class CreateLtiConfiguration200Response <
LtiConfiguration`, and a Ruby class cannot inherit a module. Fixed in the
specification (BE-1213), not patched here.
* The typed errors were dead code. errors.rb defined the hierarchy, and
api_client.rb went on raising the generated ApiError, so nothing could ever
rescue FlatNotFoundError. The raise site is rewired now.
* None of errors.rb, retry.rb, pagination.rb or oauth.rb was required from
lib/flat_api.rb, so `require 'flat_api'` gave you the generated client and
nothing built on top of it. The retry policy, the pagination helper and the
OAuth refresh were all unreachable. 95_requires.py wires them, errors
first, because api_client.rb raises from it.
Also:
* required_ruby_version moves from 3.0 to 3.3, matching the runtimes upstream
still supports and the matrix in .sdkgen.yaml.
* The gemspec ships LICENSE, README.md and CHANGELOG.md, and drops a
test_files list pointing at a directory that does not exist.
* Removes .gitlab-ci.yml. It is a leftover of an old generation: this
repository has no GitLab mirror, the file pins Ruby 3.0 and 3.1, which the
gemspec now refuses, and it runs rspec against a spec directory that is not
there.
* tools/smoke.rb drives the shared scenarios against the real API: score
lifecycle, a paginated traversal and both typed errors, cleaning up what it
creates. Nothing metered.
* Stops tracking .DS_Store.
* Removes the test harness that was never wired up: an rspec development
dependency and a .rspec requiring a spec_helper that does not exist, a
Rakefile task over a spec directory that does not exist, pry-byebug, and a
rubocop pinned to 0.66.0 from 2019 with a config targeting Ruby 2.4 and
excluding Rails paths. Nothing ran any of it, and rubocop at that pin
cannot parse the Ruby this gem now requires. CI builds the gem and loads
every file, which is the same shape as the Python client's.
A second round, after a review pointed at four more of exactly the same kind.
Each was written, documented in the README and never reached by a request:
* FlatApi::FlatClient did not exist. The first code sample in the README and
in QUICKSTART raised NameError, and so did the pagination example below it.
30_client.py writes it: it holds one ApiClient, exposes each generated API
by a short name, and paginates without the caller seeing a cursor. It builds
its own Configuration rather than mutating Configuration.default, so two
clients with different tokens do not overwrite each other.
* Pagination sent the cursor as :next. The generator renames the parameter to
:_next, because next is a Ruby keyword, and maps it back to the next query
parameter itself. Every iteration therefore refetched page one; the loop
guard saw a cursor it had already used and stopped. Traversals returned the
first page and looked like they had reached the end.
* RetryPolicy was never called. call_api is now a wrapper around the renamed
call_api_once, and Configuration carries a retry_policy to tune or disable
it. Wrapping the one request path rather than the 127 generated methods.
* TokenManager#access_token returned the token whether or not it had expired,
which made Tokens#expired? dead code. It now refreshes, re-checking inside
the lock so that two threads arriving together spend one round trip rather
than two, and so a provider that rotates refresh tokens does not have the
second call invalidate what the first stored.
The smoke suite passed through all four, so it changed too:
* It goes through FlatClient, the entry point the documentation points at.
* The pagination check creates three collections and traverses with limit=1,
so it cannot pass without following the cursor. The previous version asked
for ten items on an account holding fewer, never requested a second page,
and proved nothing.
* Two checks that need no network cover the retry wiring and the token
refresh, since a build cannot tell a module that runs from one that does
not.
* scenarios.yaml listed two scenarios no runner executed. It now describes
what actually runs.
cdeea25 to
45d9629
Compare
1 participant
The Ruby equivalent of the Python 2.0.0 bootstrap. Publishes as 1.0.0 (RubyGems is at 0.3.5).
✅
v2.26.1is released. Regenerating withSPEC_REF=v2.26.1 BUMP=none ./tools/generate.shproduces this tree byte for byte.Four defects, none of which any check in this repository would have caught
Every one was found by installing the gem and making real requests.
gem installpulled the wrong HTTP stacktyphoeus;tools/openapi-config.jsonselects thefaradaylibrary. It installed a dependency the code never loads, and skipped the ones it does. Now declaresfaraday,faraday-multipart,marcel.200response ofcreateLtiConfigurationwas an inlineallOfoverLtiConfiguration, aoneOf. This generator emits aoneOfas a module and anallOfas a subclass, so the output readclass CreateLtiConfiguration200Response < LtiConfigurationand Ruby refuses to inherit a module. Fixed in the specification (BE-1213), not patched here.errors.rbdefined the hierarchy;api_client.rbwent on raising the generatedApiError. Nothing could everrescue FlatNotFoundError.errors.rb,retry.rb,pagination.rb,oauth.rbwas required fromlib/flat_api.rb.require 'flat_api'gave you the generated client and nothing built on it.95_requires.pywires them, errors first, becauseapi_client.rbraises from it.That last one compounds the third: had the raise site been rewired without the requires, it would have thrown
NameErrorat the exact moment it tried to report an API error.Also
required_ruby_version3.0 → 3.3, matching the runtimes upstream still supports and.sdkgen.yaml.LICENSE,README.md,CHANGELOG.md, and drops atest_fileslist pointing at a directory that does not exist..gitlab-ci.yml: a leftover of an old generation. This repository has no GitLab mirror, the file pins Ruby 3.0 and 3.1 which the gemspec now refuses, and it runs rspec against aspec/directory that is not there.tools/smoke.rbdrives the shared scenarios against the real API..DS_Store.rspecdevelopment dependency and a.rspecrequiring aspec_helperthat does not exist, aRakefiletask over aspec/directory that does not exist,pry-byebug, and arubocoppinned to 0.66.0 from 2019 whose config targets Ruby 2.4 and excludes Rails paths. Nothing ran any of it, and rubocop at that pin cannot parse the Ruby this gem now requires. It was also the source of the Socket Security alert on this PR (byebug, viapry-byebug), which now passes.Second round: four more of the same, from the Codex review
Each was written, documented in the README, and never reached by a request. A build cannot tell a module that runs from one that does not, which is the whole problem.
FlatApi::FlatClientdid not existNameError, as did the pagination example.30_client.pywrites it: oneApiClient, each generated API by short name (client.scores,client.omr), andpaginatewithout the caller seeing a cursor. It builds its ownConfigurationrather than mutatingConfiguration.default, so two clients with different tokens do not overwrite each other:next; the generator renames the option to:_next(nextis a Ruby keyword) and maps it back to the query parameter itself. Every iteration refetched page one, the loop guard saw a repeated cursor, and the traversal stopped looking successfulRetryPolicywas never calledcall_apiis now a wrapper around the renamedcall_api_once, andConfiguration#retry_policytunes or disables it. One request path wrapped, not 127 generated methodsTokenManager#access_tokenignored expiryTokens#expired?dead code. It now refreshes, re-checking inside the lock so two threads spend one round trip rather than two, and a provider that rotates refresh tokens does not have the second call invalidate what the first storedThe suite that missed all four, changed
FlatClient, so the path the docs point at is the path that is proven.limit: 1, so it cannot pass without following the cursor. The old one asked for ten items on an account holding fewer, never requested a second page, and proved nothing.smoke/scenarios.yamllisted two scenarios no runner executed. It now describes what actually runs.Also caught here
The retry patch was not idempotent: after patching,
call_apiis the wrapper, so testing for the entry point re-wrapped it every run, and twice through it produced acall_api_oncethat called itself.check_idempotency.shfound it. It now keys off an explicit marker.Verification
ruby:3.3/ruby:3.4, frozenbundle install,require 'flat_api'FlatApi::FlatNotFoundErrorcheck_determinism.shcheck_idempotency.shsdk-lint-specsdk-check/sdk-check-docs/sdk-enforce-zones/sdk-validate-manifestFlatClientfrom the installed gem on 3.3 and 3.4