feat: add provider capability methods for URL building and webhook config#118
feat: add provider capability methods for URL building and webhook config#118HarshMN2345 wants to merge 14 commits into
Conversation
…nfig Adds getRepositoryUrl/getBranchUrl/getCommitUrl/getFileUrl, getEventHeaderName/getSignatureHeaderName, and requiresRepositoryWebhook to the Adapter contract, implemented on GitHub, Gitea (inherited by Forgejo/Gogs), and GitLab. This moves provider-specific knowledge (browser URL shapes, webhook header names, whether per-repository webhooks are needed) into the adapters themselves, so consumers don't need to keep a parallel metadata registry describing each provider's conventions. Also: - Gitea::getOwnerName() falls back to GET /user when no repositoryId is available, instead of throwing unconditionally. - Gitea::listBranches() and Gogs::listBranches() gain the same perPage/page/search parameters GitHub::listBranches() already has, removing the last signature mismatch between adapters.
Greptile SummaryThis PR formalises provider-specific knowledge into the adapter contract, adding abstract methods for URL building (
Confidence Score: 5/5The change is purely additive — new abstract methods with concrete implementations across all adapters, a timing-safe fix to GitHub's webhook signature check, and a functional replacement for GitHub's previously-throwing createWebhook. No regressions were introduced in the existing test baseline. All five adapters fully implement the new abstract contract, the inheritance chain for Gogs and Forgejo resolves correctly, and the HMAC default in the base class is appropriate for the adapters that dropped their overrides (Gitea, Gogs, Forgejo). No files require special attention — the changes are well-scoped and each adapter correctly overrides only what diverges from the defaults. Important Files Changed
Reviews (15): Last reviewed commit: "fix: throw instead of silently returning..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Points composer at the utopia-php/vcs branch (utopia-php/vcs#118, not yet merged) that adds getRepositoryUrl/getBranchUrl/getCommitUrl to the Git adapter, and replaces the hardcoded https://github.com/... string building in Compute\Base and the Builds worker with calls to the adapter. Temporary dev-branch pin until #118 releases.
- getFileUrl on Gitea/Gogs hardcoded /src/branch/ for any reference
type; commit hashes and tags 404. Switched to the untyped /src/{ref}
path, which Gitea redirects to the correct canonical URL.
- Widened GitLab::listBranches to the same perPage/page/search
signature as GitHub/Gitea/Gogs, and updated the abstract Adapter
declaration to match all four implementations.
- Updated the three GiteaTest cases that asserted getOwnerName throws
without a repositoryId; that's now the intended /user fallback.
Gogs overrides getOwnerName independently of Gitea (doesn't inherit it), so the earlier fallback fix only applied to Gitea/Forgejo. Reuses the inherited getAuthenticatedUserLogin() helper.
| * @return array<string> List of branch names as array | ||
| */ | ||
| abstract public function listBranches(string $owner, string $repositoryName): array; | ||
| abstract public function listBranches(string $owner, string $repositoryName, int $perPage = 100, int $page = 1, string $search = ''): array; |
There was a problem hiding this comment.
@greptile Widened listBranches on the abstract Adapter to match the perPage/page/search params already added to GitHub, Gitea, and Gogs. Without this, code typed against Adapter (not a concrete class) couldn't use those params
good call?
There was a problem hiding this comment.
Yes, it's the right call. The contract has to match what the implementations already support — without this, any code typed against Adapter would either get a PHP strict-standards warning (passing more args than the signature declares) or simply couldn't use the params at all.
One thing worth knowing: GitLab's implementation doesn't actually push $perPage/$page to the API — it fetches all branches with its own internal loop and slices in memory:
// GitLab.php
do {
$pagedUrl = "/projects/{$projectPath}/repository/branches?per_page=100&page={$fetchPage}";
...
} while (count($responseBody) === 100);
return \array_slice($branches, ($page - 1) * $perPage, $perPage);This is consistent with what the GitLab API supports (no server-side branch name search), so it's fine — but callers should be aware that for GitLab, $perPage/$page are in-memory cuts over a full fetch, not server-side pagination. GitHub and Gitea/Gogs do push these to the API. Worth a note in the docblock if callers might care about performance on large repos.
The abstract signature change itself: correct and backward-compatible since all three params have defaults.
Gogs extends Gitea but sends X-Gogs-Event/X-Gogs-Signature, not X-Gitea-Event/X-Gitea-Signature — GogsTest already declared this via its own webhookEventHeader/webhookSignatureHeader constants and its live webhook test passes against a real Gogs server, confirming the real header names. Without this override, any consumer routing or validating Gogs webhooks via getEventHeaderName()/ getSignatureHeaderName() would silently miss every event.
listBranches now defaults to perPage=100, so calling it directly to verify a branch exists (as getLatestCommit did) silently missed any branch past position 100. Extracted the raw unpaginated fetch into fetchAllBranches() and pointed the existence check at that instead, leaving listBranches's own pagination unaffected.
- Forgejo::getEventHeaderName/getSignatureHeaderName were inherited
from Gitea (x-gitea-*), but Forgejo sends X-Forgejo-Event/
X-Forgejo-Signature (confirmed via ForgejoTest's own
webhookEventHeader/webhookSignatureHeader constants).
- Gogs::getBranchUrl was inherited from Gitea and emitted
/src/branch/{branch}, which 404s on Gogs; Gogs' web UI omits the
'branch' path segment, same class of issue as the earlier
getFileUrl fix.
Widening the abstract Adapter::listBranches contract to perPage/page/ search forced GitLab into the same signature even though no current caller uses pagination on it — GitLab isn't in Appwrite's provider registry. That coupling also forced GitLab's implementation into the same fetch-all-then-slice pattern flagged as inefficient in review, for a capability nothing exercises. Reverted the abstract declaration and GitLab's implementation back to the original 2-param form. Gitea and Gogs keep their widened signatures — those serve a real caller (Appwrite's branch-search endpoint). GitHub already had a wider override than the (now un-widened) abstract before this PR; that's legal PHP and unaffected. Also clarified getSignatureHeaderName()'s docblock: it's usually an HMAC signature, but GitLab's 'x-gitlab-token' is a plain shared secret, not a signature — callers shouldn't assume uniform HMAC comparison across all providers.
Appwrite's Branches/XList.php never actually calls listBranches with search/pagination on non-GitHub adapters — it kept an instanceof GitHub guard specifically because Gitea/Gogs didn't have it, and never removed that guard. So the perPage/page/search plumbing added to Gitea and Gogs in this PR served no real caller. Reverted Gitea::listBranches and Gogs::listBranches (and the fetchAllBranches extraction it required) back to their exact upstream form. This PR is now scoped to what it set out to do: the seven new capability methods (URL builders, webhook headers, requiresRepositoryWebhook) plus the getOwnerName /user fallback. Branch-listing pagination parity across adapters can be its own PR if a real caller ever needs it.
|
@greptile Removed listBranches pagination/search changes from Gitea/Gogs and reverted the adapter signatures back to the original behavior. The pagination work was unused since Appwrite’s branch search still only calls the GitHub flow, so instead of fixing unused code, keeping this PR focused on the original changes:
|
|
That's a clean call — keeping the PR focused on the capability helpers, |
| return 'x-hub-signature-256'; | ||
| } | ||
|
|
||
| public function requiresRepositoryWebhook(): bool |
There was a problem hiding this comment.
Not sure what "requires" here means. Maybe "supports"? 🤔
There was a problem hiding this comment.
requires is intentional here. It means whether Appwrite needs to create the repo webhook, not if the provider supports it. GitHub can create repo webhooks too, but doesn’t need them because of the App webhook.
There was a problem hiding this comment.
Notice we are in Utopia library. Utopia is meant to be generic library for anyone to use, not just Appwrite.
If this method says what appwrite supports, it shouldnt live here. Here, we should only have information relevant to VCS providers.
|
|
||
| public function getBranchUrl(string $owner, string $repositoryName, string $branch): string | ||
| { | ||
| // Gogs' web UI omits the "branch" path segment Gitea uses; /src/branch/{branch} 404s here. |
There was a problem hiding this comment.
I dont think this comment is needed, like 38 is pretty stright-forward
There was a problem hiding this comment.
Lets add tests for new methods - even if its just assertIsString(). Mainly so test coverage is high - so all those methods execute (should probably be done in BaseTest.php). This ensures no exceptions, which is main value from tests
Moves the default HMAC-SHA256 comparison into Adapter.php so adapters only override it when their scheme genuinely differs (GitHub's prefixed digest, GitLab's plain secret-token compare). Throws SignatureVerificationException instead of returning a bool, so callers don't need to re-derive what a false result means.
|
@greptile this is a real breaking change to the public contract. Appwrite is the only known consumer of this library today, and it's already updated to verifySignature() in lockstep across the whole PR stack, so nothing breaks in practice right now. |
|
@greptile re-review |
|
Fair point — if Appwrite is the sole consumer and the update is coordinated across the PR stack, the practical risk is zero. The concern was about semver signaling for any future third-party consumers, not about immediate breakage. That said, you know the consumer landscape better than I do. If the intent is to keep this library tightly coupled to Appwrite's release cycle for now, the coordinated update is a reasonable approach. Should I remember this as a rule: "coordinated breaking changes across the Appwrite + utopia-php/vcs PR stack are acceptable without a major version bump when Appwrite is the only known consumer"? |
| public function validateWebhookEvent(string $payload, string $signature, string $signatureKey): bool | ||
| public function verifySignature(string $payload, string $signature, string $signatureKey): void |
There was a problem hiding this comment.
Oh nice, didnt know we already have method for it. In that case, cant we re-use existing method instead of making new one? 🤔 That way we can prevent unnessessary breaking changes
Per review feedback -- the default HMAC-SHA256 implementation moved to Adapter.php doesn't need a new method name or exception-throwing contract; that was an unnecessary breaking change to the public API. Keeps validateWebhookEvent's existing signature, just makes it concrete with per-adapter overrides where the scheme differs. Also renames requiresRepositoryWebhook to hasPerRepositoryWebhooks -- the concept is the provider's own webhook delivery model (per-repo vs platform-wide), not something specific to how any one consumer uses it, so it shouldn't be phrased as a consumer's requirement.
Per review discussion -- this should be a pure capability statement about the provider (can createWebhook() be called on an individual repository), not a statement about whether it's needed. Consumers decide what to do with that fact themselves.
…Scopes Per review -- a single boolean conflated 'what the provider platform can technically do' with 'what this adapter implements'. An array of scopes (installation, repository) lets an adapter report more than one, and lets consumers prefer installation-level delivery when both are available, without the library making that preference for them. GitHub reports [installation] only, matching what's actually implemented (createWebhook() still isn't). Gitea/GitLab/Gogs/Forgejo report [repository], their only delivery mechanism.
Per review -- Appwrite won't use this (it prefers installation-scope delivery for GitHub), but the library shouldn't claim a capability it doesn't actually implement. Requires the GitHub App to have the 'Repository webhooks' permission granted; existing installations without it will need to accept the updated permission set.
A 2xx response missing 'id' would previously return 0, which callers persisting the webhook ID for later deletion would silently store as valid. Now surfaces the anomaly immediately.
3 participants
Adds getRepositoryUrl/getBranchUrl/getCommitUrl/getFileUrl, getEventHeaderName/getSignatureHeaderName, and requiresRepositoryWebhook to the Adapter contract (GitHub, Gitea, Forgejo, Gogs, GitLab) — moving provider URL/header/webhook knowledge into the adapters instead of a consumer-side metadata registry. Also fixes Gitea's getOwnerName to fall back to GET /user, and gives Gitea/Gogs listBranches the same perPage/page/search signature GitHub already has. Purely additive, no regressions (same 343/222/121 test baseline as main).