feat!: rebuild the client on API specification 2.26.1 - #9
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.
|
![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: 5ab0da02bf
ℹ️ 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".
| $stack->push($this->typedErrors(), 'flat_typed_errors'); | ||
| $stack->push($this->retries(), 'flat_retries'); |
There was a problem hiding this comment.
When Flat returns a rate-limit 403 or a 5xx response, Guzzle resolves its base handler successfully and the outer typed-error middleware converts that response into a FlatError only after the retry middleware has already completed. Consequently, RetryPolicy::shouldRetry() never sees FlatRateLimitError or FlatServerError, so the advertised automatic retries do not happen for either condition; push the retry middleware first so it wraps typed-error conversion.
Useful? React with 👍 / 👎.
| if ($accessToken !== null) { | ||
| $this->config->setAccessToken($accessToken); |
There was a problem hiding this comment.
For OAuth users whose access token expires, FlatClient copies only a static token into Configuration, and no request path references TokenManager::accessToken(). The client therefore keeps sending the expired token and receives a 401 instead of performing the transparent refresh advertised in the README; accept or install a token manager and obtain its current token when building each authenticated request.
Useful? React with 👍 / 👎.
| usleep((int) ($policy->delayFor($error, $attempt) * 1_000_000)); | ||
| return $next($request, $options); |
There was a problem hiding this comment.
When an idempotent request fails at the transport layer, the retry callback invokes $next directly, bypassing this retry middleware on the second request. Thus the default attempts: 3 performs only two total attempts, and any second failure is returned immediately; schedule the retry through a recursive attempt function or Guzzle's retry middleware so the attempt count is honored.
Useful? React with 👍 / 👎.
| if ($accessToken !== null) { | ||
| $this->config->setAccessToken($accessToken); | ||
| } | ||
| $this->config->setHost($baseUrl ?? self::DEFAULT_BASE_URL); |
There was a problem hiding this comment.
When callers pass a Configuration already pointed at a staging server, proxy, or mock endpoint and omit the separate $baseUrl argument, this unconditional assignment replaces their host with production. This makes the configuration parameter unexpectedly unusable for its standard host-setting purpose; only apply the default when creating a new configuration, or override the host when $baseUrl is explicitly provided.
Useful? React with 👍 / 👎.
| NEW_VERSION="$(apply_bump "$CUR_VERSION")" | ||
| log "Version -> $NEW_VERSION" | ||
| # Nothing to write: Packagist reads the tag, and VERSION is written just below. | ||
| echo "$NEW_VERSION" > VERSION |
There was a problem hiding this comment.
On every future generation with the default patch bump, 80_user_agent.py runs earlier and reads the old VERSION, while these lines calculate and write the new version afterward. The resulting package therefore advertises the previous release in every User-Agent despite the patch's stated invariant; compute/write the new version before applying the user-agent patch or pass the new version into it.
Useful? React with 👍 / 👎.
| fi | ||
| git config user.name "Flat SDK bot" | ||
| git config user.email "developers@flat.io" | ||
| git tag -a "$VERSION" -m "Release $VERSION" |
There was a problem hiding this comment.
Every automatically created release uses git tag -a, which git tag -h identifies as an annotated tag, while -s is the option for an annotated and GPG-signed tag. As a result, the README's git verify-tag command reports that no signature was found and the tag provides none of the promised signed provenance; sign the tag or remove the signature-verification claim.
Useful? React with 👍 / 👎.
5ab0da0 to
7935b7c
Compare
Replaces the swagger-codegen output with a php-nextgen client that is
reproducible from this repository alone, plus the ergonomic layer the README
has always claimed.
Regenerated with openapi-generator 7.24.0 (php-nextgen) against API
specification 2.26.1: 123 operations, 23 scopes.
Six defects that no check here would have caught. Each was written, documented
in the README, and never reached by a request:
* Nothing in the ergonomic layer could be loaded. PSR-4 resolves a class to a
file named after it, and FlatError, RetryPolicy, Tokens, OAuth2Helper,
TokenManager and ErrorFactory all lived in files named for their topic
rather than their class, so every one of them was an unresolvable class
name. composer.json now carries a classmap over src/, which keeps an
exception hierarchy in one file without making it unreachable. The
generated Api and Model trees follow PSR-4 as they always did.
* FlatClient did not exist. The first example in the README and in QUICKSTART
was a fatal error. It is written now, holds one Guzzle client, exposes each
generated API by a short name, and paginates without the caller seeing a
cursor. It builds its own Configuration rather than mutating the default
one, so two clients with different tokens do not overwrite each other.
* The typed errors were dead code. The generated Api classes raise their own
ApiException at more than a hundred sites, so a caller following the README
and catching FlatNotFoundError caught nothing. Rather than rewrite every
site, a Guzzle middleware below all of them converts a non-2xx response into
the typed error. It is not a RequestException, so the generated catch blocks
do not swallow it and rewrap it.
* The retry policy was never applied. It is a second middleware on the same
stack, which is the one place in a Guzzle client where every request passes.
* Pagination sent the cursor back encoded twice. It arrives percent-encoded
inside the Link header's URL and the client encodes whatever it is handed,
so the server rejected the cursor it had issued a moment earlier. Read with
parse_str now, which applies the rules the server used to write it.
* TokenManager returned the access token whether or not it had expired, which
made Tokens::expired() dead code: every request after the expiry failed with
a 401 a refresh would have avoided.
The smoke suite passed through all of it, and could never have failed: it made
no request, printing "create/read/update/export ... ok" unconditionally, and its
guard against metered operations tested the whole scenario file for the
forbidden names, which the forbidden list itself always matched, so the suite
exited 2 before reaching anything. It now drives the real API through
FlatClient, creates three collections and traverses with limit=1 so it cannot
pass without following the cursor, and covers the retry wiring and the expiry
check without a network.
paginate spreads its parameters as PHP 8 named arguments. The generated methods
take positional parameters in specification order, so spreading the values would
hand `limit` to `$sort` the moment a caller omits one.
Also stops tracking .DS_Store, and corrects the operation count in MIGRATION.md.
The release path also needed two fixes shared with the other three clients:
* The tag carried no prefix. Every tag in this repository, and every tag in
api-reference, is v-prefixed, and the workflows introduced with the new
pipeline created a bare one from the VERSION file. tag-on-merge creates
vX.Y.Z, release.yml triggers on it, and the assertion that the tag matches
the packaged version compares with the v stripped, because the version
inside the package never carries it. Packagist reads the tag, so this is
also what the published version will be called.
* Nothing created a GitHub release, only a tag, so the Releases page would
have gone on presenting a version from years ago as the latest one.
release.yml creates it now, taking the notes from the CHANGELOG section for
the version.
Adds the 1.0.0 CHANGELOG entry those notes come from.
A re-tag is a no-op now, not a failed release. npm error You cannot publish over the previously published versions: 1.0.0. Nothing was damaged: the published package is the same artifact and is still installable. But a red release on a version that is correctly out is the kind of failure people learn to ignore, and re-running a release, or changing the tag scheme as this just did, are both ordinary things to do. The publish step is skipped when the version is already in the registry, and the GitHub release step reports and moves on when the release exists rather than erroring on the create.
7935b7c to
a42125a
Compare
1 participant
Replaces the swagger-codegen output with a
php-nextgenclient. Publishes as 1.0.0.Regenerated with openapi-generator 7.24.0 against API specification 2.26.1: 123 operations, 23 scopes.
Six defects, none of which any check here would have caught
Each was written, documented in the README, and never reached by a request.
FlatError,RetryPolicy,Tokens,OAuth2Helper,TokenManagerandErrorFactoryall lived in files named for their topic, so every one was an unresolvable class name.composer.jsonnow carries a classmap oversrc/, which keeps an exception hierarchy in one file without making it unreachableFlatClientdid not existApiExceptionat more than a hundred sites. Rather than rewrite each one, a Guzzle middleware below all of them converts a non-2xx response into the typed error. It is not aRequestException, so the generated catch blocks do not swallow and rewrap itLinkheader's URL and the client encodes whatever it is handed, so the server rejected the cursor it had just issuedTokenManagerignored expiryTokens::expired()existed and nothing called itpaginatespreads its parameters as PHP 8 named arguments. The generated methods take positional parameters in specification order, so spreading the values would handlimitto$sortthe moment a caller omits one.The suite that missed all of it
It made no request at all, printing
create/read/update/export ... okunconditionally, and its guard against metered operations tested the whole scenario file for the forbidden names, which the forbidden list itself always matched, so it exited 2 before reaching anything.It now drives the real API through
FlatClient, creates three collections and traverses withlimit: 1so it cannot pass without following the cursor, and covers the retry wiring and the expiry check without a network.Verification
composer install+ client construction on PHP 8.3sdk-check/sdk-check-docsflat/smoke-test=successTokens::expired()produced a PHP fatal error, andcheck_idempotency.shpassed the whole time. It checks that re-applying a patch changes nothing, which was true; the output was simply invalid. Worth remembering that determinism and correctness are different properties.Also stops tracking
.DS_Storeand corrects the operation count inMIGRATION.md.