{{ message }}
Repository navigation
feat: add description to functions, sites, buckets, topics, databases, keys, webhooks and variables - #14595
Merged
Conversation
Optional description on create and update, stored on the functions collection (1024 chars, default '') and included in search. Mirrors WAF rule descriptions on Cloud. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Same shape as functions: optional, up to 1024 chars, default '', kept on update when omitted. Covers sites, function/site/project variables, API keys, webhooks, databases, tables/collections, columns/attributes (every type, legacy and TablesDB) and indexes, including inline column and index definitions on table create. Searchable where the list endpoint supports search. Description-only variable updates no longer mark functions or sites for redeploy. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…bases Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Main added a webhooks case to V25 for httpPass, which shadowed the webhooks label in the keys case. Move the description step into it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Bucket update now also rewrites search, which it never did, so a renamed bucket was only found by its old name. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The schema half of this PR moved to feat/description-schema so it can merge first. Merging it here also brings in main; the only conflicts were the topic `expiry` param rewording next to the new `description` param. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
feat: add description to functions, sites, variables, keys, webhooks and databases
The API params for resource descriptions were merged into feat/description-schema by #14355 ten hours after #14471 had already merged that branch's then-tip into main, so the params never reached main. This merge brings them across. 51 files conflicted, 49 of them under the Databases module, because main refactored the attribute/column endpoints after the branch was cut. Every conflict is resolved by keeping main's structure and applying only the branch's description addition on top: - Database::VAR_* -> ColumnType::*->value (main's enum migration) - getAdapter()->getMaxUIDLength() -> getMaxUidLength() - getAdapter()->get{Min,Max}DateTime() -> getAdapter()->limits()->{min,max}DateTime - new Spatial(Database::VAR_*) -> new Spatial(ColumnType::*->value) tests/e2e/Services/Databases/DatabasesBase.php is rebuilt from main's version plus the branch's five new description tests, ported to main's enums and Id::unique() casing, because git had interleaved the branch's new tests with three main-only tests added in the same region. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Utopia skips validation for an optional param whose value is an explicit
null, then hands that null to the action rather than the declared default:
// Utopia\Http\Http::validate()
if ($param['optional'] && \is_null($value)) {
return;
}
// Utopia\Http\Http::getArguments()
$arg = $existsInRequest ? $requestParams[$requestKey] : $param['default'];
So `{"description": null}` arrived as null whatever the default was, and the
58 endpoints whose action declared a non-nullable `string $description`
raised a TypeError and returned 500 instead of treating it as not provided.
A validator cannot produce a clean 400 here, because validation is skipped
before the validator is ever consulted.
Fixed the way main already handles this for `schedule` in
Functions/Http/Functions/Create.php: declare `?string $description` and
coalesce to the declared default. The param declarations and defaults are
untouched, so the generated specs and SDKs do not change.
This keeps create and update consistent: null means "use what is already
there", which on create is the empty default and on update is the saved
value, so only '' ever clears a description.
34 action signatures cover all 58 endpoints; the other 24 are subclasses
that re-declare the params and reuse a parent's action(), such as
TablesDB\Create extending Databases\Create.
tests/unit/Platform/DescriptionParamTest.php pins the signature half by
reflecting over every endpoint that declares an optional description param,
discovered from the source so new endpoints are covered too. It reports 58
failures without this change. The behaviour either side of the null is
covered by the bucket and function e2e suites.
Also ports the branch's two new validator tests onto main's enums
(ColumnType/IndexType in place of Database::VAR_*/INDEX_*), matching what
the surrounding files on main already use.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Review complete. No blocking issues — approved ✅; 3 nitpicks below. 🧹 Nitpicks (3) — 🟢 3 low
This PR introduces a user-facing
Reviewed commit: 4805600 |
Contributor
🔵 Tier A · Mergeable after minor fixes
Adds optional descriptions to database resources, functions, sites, buckets, topics, keys, webhooks, and variables, persisting them and exposing them through response models and reference docs. Updates the SDK parameter-order lock and adds validator, unit, and end-to-end coverage for the new fields, while porting database tests to the current schema enums. Latest changes: The newest commits make create-action description arguments non-nullable again and remove explicit-null create assertions from the Functions and Storage end-to-end tests.
Fix with agent prompt### Issue 1
tests/unit/Platform/DescriptionParamTest.php:127
**Avoid regex in the endpoint-discovery test**
This test uses `preg_match` here and again at line 131 to parse endpoint source, but `AGENTS.md` explicitly forbids adding regular expressions. This can also silently miss declarations as their formatting changes; string-based parsing would follow the repository rule.
### Issue 2
src/Appwrite/Platform/Modules/Functions/Http/Functions/Update.php:198
**Cover an explicit null on function update**
The function update e2e case checks that omitting `description` preserves the saved value, but never sends `description: null`. Since this new nullable input is meant to preserve the value too, an explicit-null assertion here would catch regressions in this handler; the bucket test exercises a different action.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.📂 Walkthrough · 8
⏳ Still open from earlier reviews · 2
🔇 Filtered out · 1Findings Hansi considered but did not post.
Merged |
![hansi-codes[bot]](https://avatars.githubusercontent.com/in/5046327?s=60&v=4){kind=small}
|
|
||
| // Only the optional ones: Utopia's null-skips-validation path is reached | ||
| // through the `optional` flag. | ||
| if (!\preg_match("/->param\(\s*'description',.*,\s*true[\s,)]/U", $source)) { |
Contributor
There was a problem hiding this comment.
Avoid regex in the endpoint-discovery test
This test uses preg_match here and again at line 131 to parse endpoint source, but AGENTS.md explicitly forbids adding regular expressions. This can also silently miss declarations as their formatting changes; string-based parsing would follow the repository rule.
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/unit/Platform/DescriptionParamTest.php
Line: 127
Comment:
**Avoid regex in the endpoint-discovery test**
This test uses `preg_match` here and again at line 131 to parse endpoint source, but `AGENTS.md` explicitly forbids adding regular expressions. This can also silently miss declarations as their formatting changes; string-based parsing would follow the repository rule.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.🟡 Minor · maintainability · Reply if this doesn't apply.
| $runtime = $function->getAttribute('runtime'); | ||
| } | ||
|
|
||
| $description ??= $function->getAttribute('description', ''); |
Contributor
There was a problem hiding this comment.
Cover an explicit null on function update
The function update e2e case checks that omitting description preserves the saved value, but never sends description: null. Since this new nullable input is meant to preserve the value too, an explicit-null assertion here would catch regressions in this handler; the bucket test exercises a different action.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/Appwrite/Platform/Modules/Functions/Http/Functions/Update.php
Line: 198
Comment:
**Cover an explicit null on function update**
The function update e2e case checks that omitting `description` preserves the saved value, but never sends `description: null`. Since this new nullable input is meant to preserve the value too, an explicit-null assertion here would catch regressions in this handler; the bucket test exercises a different action.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.🟡 Minor · testing · Reply if this doesn't apply.
Security rulesNo new WARNING or ERROR findings from security rules. 19 existing findings tracked in
|
![tenki-reviewer[bot]](https://avatars.githubusercontent.com/in/2900641?s=60&v=4){kind=small}
✨ Benchmark resultsComparing
Per-scenario breakdown & investigation detailsMetrics below reflect the current branch (after). Δ P95 compares against the base.
Top API waits (after) |
Three independent failures, all in the test/config surface rather than the
endpoint code.
tests/unit/Platform/DescriptionParamTest.php used the doc-comment
`@dataProvider descriptionEndpoints`. PHPUnit is pinned 12.* and PHPUnit 12
reads metadata only from attributes, so the provider was never bound and the
test was invoked with no arguments:
ArgumentCountError: Too few arguments to function
testActionAcceptsAnExplicitNullDescription(), 0 passed ... and exactly 2 expected
Converted to #[DataProvider('descriptionEndpoints')], matching how the rest of
tests/unit/ declares providers. The provider now binds and the case runs for
all 112 discovered endpoints.
Rector (AssertComparisonToSpecificMethodRector) also flagged the subclass
guard in the same file: assertTrue($a !== $b) is now assertNotSame($b, $a),
which asserts the same thing in the form the rule requires.
tests/e2e/.../VectorsDB/DatabasesBase.php::testCreateIndexDescription was
written against Database::INDEX_HNSW_COSINE / INDEX_HNSW_DOT, which no longer
exist: main moved index types to the Utopia\Query\Schema\IndexType enum, and
the merge kept the pre-refactor constant names in the new test. That is why it
failed both at runtime (Error: Undefined constant) and under PHPStan
(classConstant.notFound). Switched to IndexType::HnswCosine->value and
IndexType::HnswDot->value, which is what the sibling index tests in this same
file already use and what the endpoint's own type whitelist accepts, and
dropped the now-unused Utopia\Database\Database import. The assertions are
untouched.
app/config/sdks/parameters.lock.json was never updated for the new param, so
Tests\Unit\SDK\ParametersTest::testMethodsAreLocked failed on the first
mismatch. Regenerated the lock from the routes themselves, using the test's own
ordering logic, and verified the result differs from the committed file in
exactly one way: "description" appended to 222 platform/method entries. No
param is reordered, renamed or removed, and no method is added or dropped, so
no released positional call shifts.
Verified locally against the pinned toolchain: phpunit on both affected unit
files is green (114 tests, 10599 assertions), rector --dry-run is clean, and
pint --test passes.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…resh the SDK lock Drops the explicit-null handling I had added on top of #14355. That 500 is not specific to description: 211 optional Text params across main's create endpoints share the shape (entrypoint and commands on Functions/Create.php, for two), because Utopia skips validation for an explicit null and only falls back to the declared default when the key is absent. Fixing it for description alone would be inconsistent with the rest of the API, so it belongs in its own PR. What remains on top of #14355 is only what main requires: - tests/e2e/.../VectorsDB/DatabasesBase.php and the two validator unit tests used Database::INDEX_HNSW_* and the pre-enum attribute constants; main has moved to the IndexType/ColumnType enums, so they died resolving a constant before reaching their assertions. - app/config/sdks/parameters.lock.json carried no description entries, so ParametersTest::testMethodsAreLocked failed on every touched method. Regenerated, not hand-patched: 222 entries gain "description", no method added, removed or reordered. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…o-main # Conflicts: # tests/e2e/Services/Project/KeysBase.php
1 of 2 tasks
6 tasks
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
2 participants
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.
What does this PR do?
Gets resource descriptions live. The API params have existed on
feat/description-schemasince 14 Oct but never reachedmain, so production Cloudstill returns no
descriptionfield on functions, buckets and the rest.Why it was never live. The two PRs landed in the wrong order:
feat/description-schema(then just the collection-schema commit) intomainfeat/description-schema— ten hours after its base had already goneSo
maingot only33ee90d2f8("add the description attribute to the collectionschema"), and the 112
param('description')declarations stayed on a branch thatwas by then 7 commits ahead of
main. The onlyparam('description')onmaintoday is the long-standing one in
Projects/Http/Projects/Update.php.This PR merges the branch across and makes it mergeable.
Resolving the merge
git mergeconflicted in 51 files, 49 of them undersrc/Appwrite/Platform/Modules/Databases/, becausemainrefactored theattribute and column endpoints after the branch was cut — exactly the surface the
branch rewrites. Every conflict is resolved by keeping
main's structure andapplying only the branch's description addition on top, never by taking the
branch's whole file. The branch was stale on all of these:
main's current formDatabase::VAR_*ColumnType::*->value(andAttributeDefinition::storedType(...))$dbForProject->getAdapter()->getMaxUIDLength()$dbForProject->getMaxUidLength()$dbForProject->getAdapter()->get{Min,Max}DateTime()$dbForProject->getAdapter()->limits()->{min,max}DateTimenew Spatial(Database::VAR_LINESTRING)new Spatial(ColumnType::Linestring->value)Two files needed more than that:
src/Appwrite/Utopia/Database/Validator/Indexes.php—mainaddeduse Utopia\Query\Schema\IndexType;where the branch addeduse Utopia\Validator\Text;. Both are needed and both are kept.tests/e2e/Services/Databases/DatabasesBase.php— git had interleaved thebranch's five new description tests with three
main-only tests added in the sameregion (
testCreateCollectionInlineDoubleIgnoresSize,testCreateCollectionInlineInternalFiltersAreRefused,testCreateCollectionInlineEncryptedString). Since the branch's change to thisfile is purely additive (492 insertions, 0 deletions), the file is rebuilt from
main's version plus those five tests, ported tomain's enums and itsId::unique()spelling — the branch'sID::unique()resolves only by PHP'scase-insensitive alias lookup, and the file uses
Id::unique()334 times.The branch's two new validator tests are ported the same way, onto
ColumnType/IndexTypeinstead ofDatabase::VAR_*/Database::INDEX_*. Theconstant values are identical (
'varchar','key', ...), so this is consistencywith the surrounding files rather than a behaviour change.
Fixing the 500 on an explicit null
Every
descriptionparam is optional, and Utopia skips validation entirely foran optional param whose value is an explicit null:
It then hands that null to the action, because
getArguments()falls back to thedeclared default only when the key is absent from the request altogether:
So
{"description": null}arrives asnullwhatever the default is. The 58endpoints whose action declared a non-nullable
string $descriptionraised aTypeErrorand returned 500. A validator cannot turn this into a clean 400,because validation is skipped before the validator is ever consulted.
Fixed the way
mainalready handles this forscheduleinFunctions/Http/Functions/Create.php:The param declarations and defaults are left alone, so the generated specs and
SDKs do not change — only the action signatures and a coalesce. 34 signatures
cover all 58 endpoints; the other 24 are subclasses that re-declare the params and
reuse a parent's
action(), such asTablesDB\Create extends Databases\Create.This also makes create and update consistent. Null means "use what is already
there": on update that is the saved value, on create there is nothing saved yet so
it is the empty default. Only
''ever clears a description, on either path.The attribute and column update endpoints express the same rule as a guard inside
the shared
Attributes\Action::updateAttribute()helper(
if (!\is_null($description))) rather than a coalesce.Deliberately left out
expire, and webhookssecurity/httpUser/enabled. These update endpoints write their paramsunconditionally, so omitting one resets it to the declared default. They predate
this work and are not description-related, so they belong in their own PR.
Projects/Http/Projects/Update.php— the console project's own pre-existingdescription, which has the same non-nullable signature. Its nine sibling params(
logo,url,legalName, ...) share both that shape and the unconditionaloverwrite above, so fixing only
descriptionthere would be half a change. It isthe one documented exclusion in the new unit test.
mainoutside this surface —entrypointandcommandsonFunctions/Create.php, for instance, would 500 onan explicit null today. Out of scope here.
Test Plan
composer installdoes not run in this environment, so this was verified withstatic checks, the repo's own tooling, and by executing the new test's assertion
directly. Reported honestly, including what could not be run.
Ran and passed:
php -lacross all 144 changed files — 0 failures.pint --test --config pint.jsonon all 144 —{"tool":"pint","result":"passed"}.DescriptionParamTestreflects over every endpoint declaring an optional
descriptionparam. Runningthat reflection against all 112 endpoints (0 load errors):
src/reverted to the merge commit: FAIL, 58 failingmaincontent reverted. Every one of the 395 hunks in this PR was audited;the 59 that do not mention "description" are all pure additions — 53
use Utopia\Validator\Text;, 5use Utopia\Validator\Nullable;, and one newupdateVariable()e2e helper. Zero removals.Database::VAR_,getAdapter()->getMaxUIDLength()andgetAdapter()->get{Min,Max}DateTime()appear in 0 files under the touchedtrees, confirming no refactor was undone.
DatabasesBase.phpis492 insertions, 0 deletions against
main— exactly the branch's additivediff, with all three
main-only tests intact.git merge-tree: all74 non-conflicted branch-touched files are byte-identical to a clean merge.
param('description')count is 113 — the branch's 112 plus the pre-existingProjects/Update.phpone, which survives (92 Databases, 4 Sites, 4 Project,4 Functions, 2 each Webhooks/Storage/Organization/Messaging).
git merge-tree origin/main HEADexits 0: merges cleanly into currentmain,including the
packages/queueandpackages/usagecommits that landed while thiswas being prepared.
Could not run here:
vendor/, and the one sibling checkout with one installedpredates
Utopia\Validator\Validator, so the validator tests cannot be loadedagainst it. The reflection assertion above runs because PHP resolves
usestatements lazily, so action signatures can be read without the validator classes.
StorageCustomServerTest::testUpdateBucketDescriptionandFunctionsCustomServerTestassert create-with-null →201+''andupdate-with-null → keeps the saved value.
.github/workflows/specs.ymlgenerates them in CI. This PR changes no param declaration, so the specs should be
unaffected either way.
Related PRs and Issues
feat/description-schemaafter its base had already been merged tomainfeat/description-schemaintomainten hours earlier, carrying only the collection-schema commitfix/sdk-param-order, already on the branchChecklist
The
descriptionparams are new API metadata, and the branch carries the referencedocs for them. No spec files are committed to this repo —
.github/workflows/specs.ymlgenerates them — and the 500 fix deliberately changes no declaration, only action
signatures, so it does not move the specs.
🤖 Generated with Claude Code