Separate specifications#10849
Conversation
📝 WalkthroughWalkthroughThe PR splits the single Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention during review:
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-09-18T07:42:08.734ZApplied to files:
🧬 Code graph analysis (1)app/controllers/general.php (5)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🔇 Additional comments (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Security Scan Results for PRDocker Image Scan Results
Source Code Scan Results🎉 No vulnerabilities found! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
tests/e2e/General/UsageTest.php (1)
942-944: Function creation specs wiring looks correctUsing both
buildSpecificationandruntimeSpecificationin the payload aligns with the new Functions.Create contract; constants and field names look consistent. You might later want to either adjust the comment or the values to actually exercise differing specs, but that’s purely optional.tests/e2e/Services/Sites/SitesCustomServerTest.php (2)
36-72: Specs listing test is aligned with the new APIThe use of
buildSpecificationandruntimeSpecificationin creation and assertions matches the new schema and exercises both valid and invalid values. If you ever expect environments with a single spec configured, you may want to assert index1exists before using it, but with current defaults this is acceptable.
1292-1359: Runtime spec update and validation tests look consistentUpdating
runtimeSpecificationand then asserting both the stored value and the error messages for invalidbuildSpecification/runtimeSpecificationalign with how the validators are expected to behave. Only very minor nits: the comment “Change the function specs” is slightly misleading in a sites test, and the hard‑coded error message prefixes must stay in sync with the validator text, but that’s in line with existing patterns.src/Appwrite/Platform/Modules/Sites/Http/Sites/Create.php (1)
81-93: Site create: spec parameters are correct; minor wording nit
buildSpecificationandruntimeSpecificationare set up consistently with the compute base (default via plan‑awaregetDefaultSpecification,Specificationvalidator, injectedplan) and persisted under matching document keys. That part looks good.Only nit is the description string:
), 'Runtime specification for the function SSR executions.'which could be updated to drop “function” to avoid confusion in a sites context.
- ), 'Runtime specification for the function SSR executions.', true, ['plan']) + ), 'Runtime specification for SSR executions.', true, ['plan'])Also applies to: 120-121, 172-173
tests/e2e/Services/Functions/FunctionsCustomServerTest.php (1)
35-68: Guard against assuming at least two specs intestListSpecsThe test indexes both
specifications[0]andspecifications[1]but only asserts that index 0 exists. If config is ever reduced to a single specification, this will notice via a PHP notice rather than a clean assertion failure.Consider explicitly asserting there are at least two specs before using index 1, e.g.
assertGreaterThanOrEqual(2, $specifications['body']['specifications']);.src/Appwrite/Platform/Workers/Functions.php (1)
359-359: Use ofruntimeSpecificationhere matches new separationSwitching to
$function->getAttribute('runtimeSpecification', APP_COMPUTE_SPECIFICATION_DEFAULT)is consistent with using build specs for builds and runtime specs for executions; downstream usage of$spec(env vars, executor resources, metrics) remains valid.If you ever harden this path, consider guarding against unknown spec keys (e.g. falling back when
Config::getParam('specifications')lacks the given index) to avoid notices/fatal errors on bad data.src/Appwrite/Platform/Modules/Functions/Workers/Builds.php (1)
255-255: Build paths now consistently usebuildSpecificationBoth the build execution (
buildDeployment) and usage accounting (sendUsage) now derive$specfrombuildSpecification, which correctly separates build-time sizing from runtime sizing. The min-memory logic and metrics based on$spec['cpus']/$spec['memory']remain coherent.You may want to defensively handle unknown spec IDs (missing array key in
Config::getParam('specifications')) with a fallback or explicit error, to avoid hard failures if data is corrupted or misconfigured.Also applies to: 1357-1357
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
app/config/collections/projects.php(2 hunks)app/controllers/general.php(1 hunks)src/Appwrite/Platform/Modules/Functions/Http/Executions/Create.php(1 hunks)src/Appwrite/Platform/Modules/Functions/Http/Functions/Create.php(3 hunks)src/Appwrite/Platform/Modules/Functions/Http/Functions/Update.php(4 hunks)src/Appwrite/Platform/Modules/Functions/Workers/Builds.php(2 hunks)src/Appwrite/Platform/Modules/Sites/Http/Sites/Create.php(3 hunks)src/Appwrite/Platform/Modules/Sites/Http/Sites/Update.php(4 hunks)src/Appwrite/Platform/Workers/Functions.php(1 hunks)src/Appwrite/Utopia/Response/Model/Func.php(1 hunks)src/Appwrite/Utopia/Response/Model/Site.php(1 hunks)tests/e2e/General/UsageTest.php(1 hunks)tests/e2e/Services/Functions/FunctionsCustomServerTest.php(8 hunks)tests/e2e/Services/Sites/SitesCustomServerTest.php(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
src/Appwrite/Utopia/Response/Model/Site.php (1)
src/Appwrite/Utopia/Response/Model.php (1)
addRule(90-102)
tests/e2e/Services/Sites/SitesCustomServerTest.php (2)
tests/e2e/Services/Sites/SitesBase.php (7)
getSite(177-185)createSite(107-115)updateSite(117-125)setupDeployment(36-83)packageSite(239-251)getDeployment(187-195)cleanupSite(85-94)tests/e2e/Services/Functions/FunctionsBase.php (3)
setupDeployment(35-67)getDeployment(160-168)createExecution(297-305)
src/Appwrite/Platform/Modules/Sites/Http/Sites/Create.php (2)
src/Appwrite/GraphQL/Types/Mapper.php (1)
param(255-363)src/Appwrite/Platform/Modules/Compute/Base.php (1)
getDefaultSpecification(30-53)
src/Appwrite/Utopia/Response/Model/Func.php (1)
src/Appwrite/Utopia/Response/Model.php (1)
addRule(90-102)
src/Appwrite/Platform/Modules/Sites/Http/Sites/Update.php (2)
src/Appwrite/Platform/Modules/Compute/Base.php (1)
getDefaultSpecification(30-53)src/Executor/Executor.php (1)
deleteRuntime(143-164)
src/Appwrite/Platform/Modules/Functions/Http/Functions/Update.php (2)
src/Appwrite/Platform/Modules/Compute/Base.php (1)
getDefaultSpecification(30-53)src/Executor/Executor.php (1)
deleteRuntime(143-164)
src/Appwrite/Platform/Modules/Functions/Http/Functions/Create.php (1)
src/Appwrite/Platform/Modules/Compute/Base.php (1)
getDefaultSpecification(30-53)
tests/e2e/Services/Functions/FunctionsCustomServerTest.php (2)
tests/e2e/Services/Functions/FunctionsBase.php (3)
getFunction(150-158)cleanupFunction(69-78)createFunction(80-88)tests/e2e/Client.php (1)
Client(8-342)
🪛 PHPMD (2.15.0)
tests/e2e/Services/Sites/SitesCustomServerTest.php
3007-3007: Avoid unused local variables such as '$functionId'. (undefined)
(UnusedLocalVariable)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: scan
🔇 Additional comments (13)
src/Appwrite/Platform/Modules/Functions/Http/Functions/Create.php (1)
96-107: Function create: build/runtime spec parameters are wired correctlyThe introduction of
buildSpecificationandruntimeSpecificationmirrors the existing specification pattern: defaults viagetDefaultSpecification($plan), validation viaSpecification, proper DI ofplan, and the values are persisted under the expected keys on the function document. Signature updates and persistence look consistent with the rest of the module.Also applies to: 145-147, 241-243
src/Appwrite/Utopia/Response/Model/Site.php (1)
164-176: Site response model updated to expose both build and runtime specsAdding
buildSpecificationandruntimeSpecificationwith string type andAPP_COMPUTE_SPECIFICATION_DEFAULTdefaults cleanly reflects the new API surface. Descriptions for deployments vs SSR executions are clear; this matches how you’ve shaped the backend documents.src/Appwrite/Utopia/Response/Model/Func.php (1)
179-190: Function response model correctly splits build vs runtime specsExposing
buildSpecificationalongsideruntimeSpecification, both usingAPP_COMPUTE_SPECIFICATION_DEFAULTas default/example, matches the new persistence fields and clarifies the distinction between build and execution machine specs.src/Appwrite/Platform/Modules/Functions/Http/Functions/Update.php (3)
92-103: New build/runtime specification params are wired correctlyThe parameter definitions for
buildSpecificationandruntimeSpecificationcorrectly reusegetDefaultSpecification($plan)and theSpecificationvalidator with the same config/env inputs as elsewhere, and they’re marked as plan-dependent. This looks consistent with the rest of the compute module.
134-135: Action signature matches new parametersThe added
string $buildSpecificationandstring $runtimeSpecificationarguments line up with the new param definitions and downstream usage. No issues spotted with ordering or types.
286-287: Persisting build/runtime specs on the function document looks correctStoring
buildSpecificationandruntimeSpecificationalongside other function attributes in the update payload matches the new API surface and is consistent with the cold-start logic and tests.tests/e2e/Services/Functions/FunctionsCustomServerTest.php (3)
1378-1425: Runtime spec update and validation coverage looks solidThe
testUpdateSpecschanges exercisingruntimeSpecificationupdates (1 vCPU / 1GB then 1 vCPU / 512MB) and validating the resultingAPPWRITE_FUNCTION_CPUS/APPWRITE_FUNCTION_MEMORYvalues, plus the failure cases for invalidbuildSpecificationandruntimeSpecificationmessages, align well with the new semantics and error surface.Also applies to: 1442-1465
2047-2083: Response-format compatibility tests correctly cover new spec fieldsThe extended
testResponseFilterschecks that:
- 1.5.0 responses expose neither
specificationnor the newbuildSpecification/runtimeSpecificationfields,- 1.8.0 responses expose legacy
specificationbut not the new fields, and- latest responses drop
specificationand includebuildSpecificationandruntimeSpecification.This nicely protects backward-compat behavior across formats.
2233-2244: End-to-end build/runtime spec separation is well tested
testFunctionSpecificationsnow:
- sets
buildSpecificationtoS_2VCPU_2GBandruntimeSpecificationtoS_1VCPU_1GB,- asserts these values are echoed back in the function document,
- verifies build logs contain
2048:2(build-time resources), and- verifies runtime env reports
APPWRITE_FUNCTION_MEMORY=1024andAPPWRITE_FUNCTION_CPUS=1.This cleanly exercises the intended separation between build and runtime specs.
Also applies to: 2262-2263
app/config/collections/projects.php (2)
769-789: Functions: separated build/runtime specs look consistent
buildSpecificationandruntimeSpecificationare defined with identical shape, sane size (128), nullable, and defaulting toAPP_COMPUTE_SPECIFICATION_DEFAULT, which matches how they’re read elsewhere. No issues from the schema side.Please confirm there is a migration/patch that copies existing
specificationvalues into these two new attributes so old functions keep their configured specs after upgrade.
1204-1223: Sites: build/runtime spec attributes align with functionsThe new
buildSpecification/runtimeSpecificationattributes forsitesmirror the functions schema and are compatible with the wayBuildsandrouter()now read them. Looks correct.src/Appwrite/Platform/Modules/Functions/Http/Executions/Create.php (1)
167-167: Execution API correctly switched toruntimeSpecificationReading
$specfromruntimeSpecificationaligns this HTTP execution path with the worker and router, so all execution‑time CPU/memory and MB‑seconds metrics now consistently use the runtime spec instead of the build spec.app/controllers/general.php (1)
308-308: Router now uses runtime spec for proxied executionsUsing
$resource->getAttribute('runtimeSpecification', APP_COMPUTE_SPECIFICATION_DEFAULT)ensures the custom‑domain router aligns with the new build/runtime split: build paths usebuildSpecification, while proxied executions (functions and sites) rely on the runtime spec for env vars, executor resources, and MB‑seconds metrics.Please double‑check that all relevant resources (including older ones created before this change) have
runtimeSpecificationpopulated via defaults or migrations so routed traffic doesn’t silently revert to the global default spec.
✨ Benchmark results
⚡ Benchmark Comparison
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/Appwrite/Platform/Modules/Sites/Http/Sites/Update.php (1)
85-96: Clarify runtimeSpecification description for sitesThe new param wiring looks good, but the description still says “Runtime specification for the function SSR executions.” which is confusing in a sites endpoint.
Suggest aligning with the Site model wording:
- ), 'Runtime specification for the function SSR executions.', true, ['plan']) + ), 'Runtime specification for the site SSR executions.', true, ['plan'])(or simply “Runtime specification for SSR executions.” to match the model.)
src/Appwrite/Platform/Modules/Sites/Http/Sites/Create.php (1)
81-92: Site creation specs are wired correctly; adjust runtime descriptionThe new
buildSpecification/runtimeSpecificationparams use the expected default resolver and validator and are persisted into the site document consistently; no functional issues.Small wording tweak to avoid “function” terminology on a site endpoint:
- ), 'Runtime specification for the function SSR executions.', true, ['plan']) + ), 'Runtime specification for the site SSR executions.', true, ['plan'])(or match the model’s “Machine specification for SSR executions.”)
Also applies to: 119-120, 171-172
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
app/controllers/general.php(3 hunks)src/Appwrite/Platform/Modules/Functions/Http/Functions/Update.php(4 hunks)src/Appwrite/Platform/Modules/Sites/Http/Sites/Create.php(3 hunks)src/Appwrite/Platform/Modules/Sites/Http/Sites/Update.php(4 hunks)src/Appwrite/Utopia/Request/Filters/V21.php(2 hunks)src/Appwrite/Utopia/Response/Filters/V21.php(1 hunks)src/Appwrite/Utopia/Response/Model/Site.php(1 hunks)tests/e2e/Services/Sites/SitesCustomServerTest.php(6 hunks)tests/resources/sites/astro/src/pages/specs.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/e2e/Services/Sites/SitesCustomServerTest.php
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: atharvadeosthale
Repo: appwrite/appwrite PR: 10486
File: src/Appwrite/Utopia/Request/Filters/V21.php:25-33
Timestamp: 2025-09-18T07:42:08.734Z
Learning: In Appwrite's V21 request filter for backward compatibility, clients will never send both old (`version`) and new (`type`/`reference`) parameters simultaneously - they use either the old 1.8.0 format or the new 1.8.1+ format, not a mix of both.
📚 Learning: 2025-09-18T07:42:08.734Z
Learnt from: atharvadeosthale
Repo: appwrite/appwrite PR: 10486
File: src/Appwrite/Utopia/Request/Filters/V21.php:25-33
Timestamp: 2025-09-18T07:42:08.734Z
Learning: In Appwrite's V21 request filter for backward compatibility, clients will never send both old (`version`) and new (`type`/`reference`) parameters simultaneously - they use either the old 1.8.0 format or the new 1.8.1+ format, not a mix of both.
Applied to files:
app/controllers/general.phpsrc/Appwrite/Utopia/Response/Filters/V21.phpsrc/Appwrite/Utopia/Request/Filters/V21.php
🧬 Code graph analysis (7)
app/controllers/general.php (3)
src/Appwrite/Utopia/Response/Filters/V20.php (1)
V20(8-26)src/Appwrite/Utopia/Request/Filters/V21.php (1)
V21(7-51)src/Appwrite/Utopia/Response/Filters/V21.php (1)
V21(9-51)
src/Appwrite/Utopia/Response/Model/Site.php (1)
src/Appwrite/Utopia/Response/Model.php (1)
addRule(90-102)
src/Appwrite/Utopia/Response/Filters/V21.php (2)
src/Appwrite/Utopia/Response.php (1)
Response(159-983)src/Appwrite/Utopia/Request/Filters/V21.php (2)
V21(7-51)parse(10-25)
src/Appwrite/Utopia/Request/Filters/V21.php (1)
src/Appwrite/Utopia/Response/Filters/V21.php (1)
parse(11-30)
src/Appwrite/Platform/Modules/Sites/Http/Sites/Update.php (2)
src/Appwrite/Platform/Modules/Compute/Base.php (1)
getDefaultSpecification(30-53)src/Executor/Executor.php (1)
deleteRuntime(143-164)
src/Appwrite/Platform/Modules/Sites/Http/Sites/Create.php (2)
src/Appwrite/GraphQL/Types/Mapper.php (1)
param(255-363)src/Appwrite/Platform/Modules/Compute/Base.php (1)
getDefaultSpecification(30-53)
src/Appwrite/Platform/Modules/Functions/Http/Functions/Update.php (2)
src/Appwrite/Platform/Modules/Compute/Base.php (1)
getDefaultSpecification(30-53)src/Executor/Executor.php (1)
deleteRuntime(143-164)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: E2E Service Test (Migrations)
- GitHub Check: E2E Service Test (Messaging)
- GitHub Check: E2E Service Test (VCS)
- GitHub Check: E2E Service Test (Storage)
- GitHub Check: E2E Service Test (Webhooks)
- GitHub Check: E2E Service Test (Proxy)
- GitHub Check: E2E Service Test (Account)
- GitHub Check: E2E Service Test (Projects)
- GitHub Check: E2E Service Test (Health)
- GitHub Check: E2E Service Test (Functions)
- GitHub Check: E2E Service Test (Realtime)
- GitHub Check: E2E Service Test (Databases/TablesDB)
- GitHub Check: E2E Service Test (FunctionsSchedule)
- GitHub Check: E2E Service Test (Console)
- GitHub Check: E2E Service Test (Site Screenshots)
- GitHub Check: E2E Service Test (Dev Keys)
- GitHub Check: Unit Test
- GitHub Check: E2E General Test
- GitHub Check: Benchmark
- GitHub Check: scan
🔇 Additional comments (8)
app/controllers/general.php (3)
32-33: LGTM! Import additions for new response filters.The imports for
ResponseV20andResponseV21are correctly added and follow the existing pattern for other version-specific response filters.
310-310: LGTM! Correct use ofruntimeSpecificationfor execution context.The change from
specificationtoruntimeSpecificationis appropriate here since this code path handles runtime execution and needs the runtime resource specifications for CPU/memory allocation. The fallback toAPP_COMPUTE_SPECIFICATION_DEFAULTensures backward compatibility.
1053-1058: LGTM! Response filters added for backward compatibility.The addition of
ResponseV20andResponseV21filters ensures backward compatibility for clients using API versions before 1.8.0 and 1.9.0 respectively. The V21 filter converts the new dual-spec format (buildSpecificationandruntimeSpecification) back to the legacy singlespecificationfield for older clients.Please verify that the backward compatibility works correctly for older SDK clients (< 1.9.0) by testing:
- Creating/updating functions and sites with older SDKs
- Retrieving function and site documents with older SDKs
- Ensuring the
specificationfield appears correctly in responsesBased on learnings: The V21 response filter exposes
buildSpecificationas the backward-compatiblespecificationvalue. Ensure this provides the expected behavior for clients that previously used a singlespecificationfor both build and runtime contexts.tests/resources/sites/astro/src/pages/specs.js (1)
1-13: Simple specs endpoint looks correctReturns the expected env fields as JSON with appropriate content type; fine for the test use case.
src/Appwrite/Utopia/Request/Filters/V21.php (1)
9-25: Specs back-compat mapping for V21 requests looks soundFor create/update you only translate legacy
specificationintobuildSpecificationandruntimeSpecificationwhen it’s present, leaving newer payloads (that already send the split fields) untouched. This aligns with the non-mixed-format assumption and gives older clients identical values in both specs.Also applies to: 41-50
src/Appwrite/Utopia/Response/Model/Site.php (1)
164-175: Site spec fields are modeled consistently
buildSpecificationandruntimeSpecificationare exposed with string type, default/example fromAPP_COMPUTE_SPECIFICATION_DEFAULT, and clear descriptions; this matches how they’re validated and stored elsewhere.src/Appwrite/Platform/Modules/Sites/Http/Sites/Update.php (1)
243-252: Cold-start on spec changes now uses correct attributesThe spec-change block correctly compares both
runtimeSpecificationandbuildSpecificationagainst the stored values and callsdeleteRuntimeonly when either differs, while tolerating 404s from the executor. Persistence ofbuildSpecification/runtimeSpecificationinto the site document matches the parameter list. This addresses the earlier attribute name typo and aligns with the functions update behavior.Also applies to: 281-282
src/Appwrite/Platform/Modules/Functions/Http/Functions/Update.php (1)
92-103: Function spec parameters and cold-start handling look consistent
buildSpecification/runtimeSpecificationare validated and defaulted via the compute Specification validator, passed into the action, and persisted under matching attribute names. The cold-start section now compares both stored specs against the incoming values using the correct keys and only callsdeleteRuntimewhen something changes, while ignoring 404s from the executor. This is aligned with the site update logic and fixes the earlier attribute typo.Also applies to: 134-135, 245-263, 286-287
2 participants
What does this PR do?
Separates build and runtime specs
Test Plan
Updated tests
Related PRs and Issues
x
Checklist