[Platform API] Refactor database schema and improve#2254
Open
Thushani-Jayasekera wants to merge 39 commits into
Open
[Platform API] Refactor database schema and improve#2254Thushani-Jayasekera wants to merge 39 commits into
Thushani-Jayasekera wants to merge 39 commits into
Conversation
- Updated the schema for organizations, projects, applications, artifacts, rest_apis, subscription_plans, subscriptions, gateways, gateway_custom_policies, gateway_custom_policy_usages, gateway_tokens, deployments, deployment_status, and association_mappings tables to replace VARCHAR(40) with UUID for uuid and related foreign key fields. - Added new indexes for improved query performance on various tables, including api_keys, gateway_custom_policy_usages, and events.
…40) for primary keys - Introduced a new skill for comprehensive database schema design, modification, and review processes. - Updated the schema definitions for organizations, projects, applications, artifacts, rest_apis, subscription_plans, subscriptions, and gateways to replace UUID with VARCHAR(40) for primary keys and related foreign keys. - Added created_by and updated_by fields to relevant tables for better tracking of changes.
Thushani-Jayasekera
requested review from
AnuGayan,
Arshardh,
CrowleyRajapakse,
HeshanSudarshana,
HiranyaKavishani,
Krishanx92,
PasanT9,
RakhithaRR,
Tharsanan1,
VirajSalaka,
ashera96,
chamilaadhi,
dushaniw,
hisanhunais,
malinthaprasan,
pubudu538,
renuka-fernando,
senthuran16,
tgtshanika,
tharikaGitHub and
tharindu1st
as code owners
Contributor
|
Warning Review limit reached
More reviews will be available in 49 minutes and 45 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughThis PR rewrites the database schemas and repository contracts toward org-scoped, artifact-typed entities with audit metadata and binary payload storage. It removes DevPortal publication paths across handlers, services, repositories, models, client code, scopes, and OpenAPI paths. It also propagates actor identities through mutating handlers and services, adds audit recording, and updates related tests and utility mappings to use 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
platform-api/src/internal/database/schema.postgres.sql (1)
177-188: 🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy liftAlign
gateway_custom_policieswith repository queries.
platform-api/src/internal/repository/custom_policy.go:35-120still inserts/selectsdisplay_nameand does not provide the new requiredhandle; this DDL will break those paths. Update the repository/model contract or keep compatible columns.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@platform-api/src/internal/database/schema.postgres.sql` around lines 177 - 188, The gateway_custom_policies table schema now includes a handle column but the custom_policy.go repository code at lines 35-120 is still using display_name for insert and select operations, creating a mismatch. Either update the repository methods and model in custom_policy.go to use the new handle column instead of display_name, or add display_name back to the schema as a compatible column if handle is meant to be a new required field alongside display_name. Ensure the DDL and repository queries agree on what columns are being inserted and selected.
🧹 Nitpick comments (2)
.agents/skills/db-schema-review/SKILL.md (2)
471-490: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSpecify language for the reference table code block.
Add ```sql to the reference column width table at line 471 for consistency with other code blocks and to satisfy markdown linting rules.
🔧 Proposed fix
-\`\`\` +\`\`\`sql VARCHAR(40) — uuid, all FK columns (organization_uuid, project_uuid, gateway_uuid, …)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.agents/skills/db-schema-review/SKILL.md around lines 471 - 490, The reference column width table in the SKILL.md file is missing the SQL language specifier in its markdown code fence. Locate the code block starting with the VARCHAR(40) line that documents the column width standards and add "sql" after the opening triple backticks (changing ``` to ```sql) to properly specify the language for the code block, ensuring consistency with other code blocks in the document and satisfying markdown linting requirements.
28-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSpecify language for bash code blocks.
Markdown linters flag missing language specifiers on fenced code blocks. Add ```bash to lines 28 and 110:
🔧 Proposed fixes
+bash
Always read both before writing any DDL
cat internal/database/schema.postgres.sql
```diff+bash
find . -name "schema.postgres.sql" -o -name "schema.sqlite.sql" | grep internal/database</details> Also applies to: 110-111 <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In @.agents/skills/db-schema-review/SKILL.md around lines 28 - 30, Add the bash
language specifier to the markdown code blocks in the SKILL.md file. On line 28,
change the code block opening fromtobash for the block containing the
cat command that reads the database schema file. Similarly, on line 110, add
bash to the code block opening for the block containing the find command that
searches for schema files. This ensures markdown linters recognize these as bash
code blocks.</details> <!-- cr-comment:v1:b879f729d26af253c4a039fe --> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.Inline comments:
In @.agents/skills/db-schema-review/SKILL.md:
- Around line 134-158: The "Workflow" section with Steps 0–3 (Step 0 — Locate
the schemas through Step 3 — Report) is a duplicate of the existing "Workflow B"
section that uses clearer A/B naming conventions. Remove the entire duplicate
Workflow section from lines 134–158, keeping only the "Workflow B" section which
provides the same review process with better naming clarity and eliminates
maintenance overhead and reader confusion.In
@platform-api/src/internal/database/schema.postgres.sql:
- Around line 65-73: The artifacts table is missing the name and version fields
that are required to enforce global uniqueness constraints. Add name VARCHAR and
version VARCHAR columns to the artifacts table (CREATE TABLE IF NOT EXISTS
artifacts), and add a UNIQUE constraint on (name, version, organization_uuid) to
ensure that artifact identities remain globally unique across all artifact kinds
within an organization, replacing or supplementing the existing UNIQUE(uuid,
organization_uuid) constraint as needed.- Around line 192-197: The gateway_custom_policy_usages table schema has been
updated to use artifact_uuid instead of api_uuid, but the repository code in
custom_policy.go (lines 170-240) still references the old api_uuid column.
Update all SQL queries and method signatures in the custom_policy.go file that
read from or write to the gateway_custom_policy_usages table to replace api_uuid
with artifact_uuid. This includes updating any parameter names, variable names,
and SQL column references in the affected queries and their corresponding caller
methods to maintain consistency with the schema change.- Around line 329-330: The foreign key constraint on project_uuid that
references projects(uuid) is currently using ON DELETE CASCADE, but since
project_uuid is nullable by design, change this constraint to ON DELETE RESTRICT
instead. This will prevent automatic cascading deletion of mcp_proxies records
when a project is deleted, allowing the application code to explicitly handle
the nullable relationship. Locate the second FOREIGN KEY line in the diff (the
one with project_uuid REFERENCES projects) and replace ON DELETE CASCADE with ON
DELETE RESTRICT.- Around line 250-252: Remove the CHECK constraints on the status and
status_desired columns from the deployment_status table definition.
Specifically, delete the lines containing CHECK (status IN ('DEPLOYED',
'DEPLOYING', 'UNDEPLOYED', 'UNDEPLOYING', 'FAILED', 'ARCHIVED')) and CHECK
(status_desired IN ('DEPLOYED', 'UNDEPLOYED')) from the DDL. Apply this same
removal across all schema variants (schema.postgres.sql, schema.mysql.sql, etc.)
to ensure consistency, as the application layer handles the validation of these
deployment status values.- Around line 391-393: The CHECK constraint for api_keys.status should be
removed from the schema file since status validation is handled at the
application layer, not at the database level. Locate the line containing CHECK
(status IN ('active', 'expired', 'revoked')) in the api_keys table definition
and delete it entirely, while keeping the FOREIGN KEY and UNIQUE constraints in
place.In
@platform-api/src/internal/database/schema.sql:
- Around line 65-96: The rest_apis table schema now requires handle, name, and
version as NOT NULL columns, but the insert and select operations in the api.go
repository are not providing or reading these fields from rest_apis. Update the
INSERT queries that write to rest_apis to explicitly include the handle, name,
and version columns with values, and update the SELECT queries to read these
fields from rest_apis instead of artifacts. Alternatively, if these fields
should remain on artifacts, move them back to the artifacts table in the schema
and remove them from the rest_apis table definition.- Around line 333-334: The foreign key constraint on project_uuid in the
mcp_proxies table currently uses ON DELETE CASCADE, which will cascade project
deletion into mcp_proxies. Since project_uuid is nullable by design and should
be handled explicitly by application code, change the ON DELETE CASCADE clause
for the FOREIGN KEY (project_uuid) REFERENCES projects(uuid) constraint to ON
DELETE RESTRICT instead. This ensures that projects cannot be deleted if they
are referenced by mcp_proxies records, allowing the application to handle the
nullable relationship properly.- Around line 249-251: Remove the two CHECK constraints from the
deployment_status table schema: the constraint that validates status values
against the list ('DEPLOYED', 'DEPLOYING', 'UNDEPLOYED', 'UNDEPLOYING',
'FAILED', 'ARCHIVED') and the constraint that validates status_desired against
('DEPLOYED', 'UNDEPLOYED'). Delete these constraint definitions entirely as
status validation must be handled at the application layer rather than in the
database schema. Apply this removal to all schema.*.sql file variants.- Around line 400-402: Remove the CHECK constraint on the status column in the
api_keys table definition within schema.sql. The CHECK constraint that restricts
status values to 'active', 'expired', or 'revoked' should be deleted since
api_keys.status validation is already handled at the application layer and
should not be enforced at the database level.In
@platform-api/src/internal/database/schema.sqlite.sql:
- Around line 329-330: The FOREIGN KEY constraint on project_uuid in the
mcp_proxies table currently uses ON DELETE CASCADE, but since project_uuid is
intentionally nullable, this should be changed to ON DELETE RESTRICT to allow
application code to explicitly handle the deletion logic. Locate the FOREIGN KEY
constraint that references projects(uuid) for the project_uuid column in the
mcp_proxies table definition and replace ON DELETE CASCADE with ON DELETE
RESTRICT for that specific constraint only, leaving the other FOREIGN KEY
constraint for uuid unchanged.- Around line 250-252: Remove the two CHECK constraints from the
deployment_status table in the schema file. Delete the line containing the CHECK
constraint that validates status against the list of allowed values (DEPLOYED,
DEPLOYING, UNDEPLOYED, UNDEPLOYING, FAILED, ARCHIVED) and the line containing
the CHECK constraint for status_desired. These validation rules should be
enforced at the application layer rather than in the database schema, as the
status lifecycle is managed by the application. Make sure to apply this change
across all schema variants mentioned in the comment.- Around line 391-393: Remove the CHECK constraint that validates
api_keys.status values from the api_keys table definition in schema.sqlite.sql.
Locate the line containing CHECK (status IN ('active', 'expired', 'revoked'))
and delete it entirely, as status validation should be managed at the
application layer rather than enforced via database constraints to maintain the
existing application-owned lifecycle contract.
Outside diff comments:
In@platform-api/src/internal/database/schema.postgres.sql:
- Around line 177-188: The gateway_custom_policies table schema now includes a
handle column but the custom_policy.go repository code at lines 35-120 is still
using display_name for insert and select operations, creating a mismatch. Either
update the repository methods and model in custom_policy.go to use the new
handle column instead of display_name, or add display_name back to the schema as
a compatible column if handle is meant to be a new required field alongside
display_name. Ensure the DDL and repository queries agree on what columns are
being inserted and selected.
Nitpick comments:
In @.agents/skills/db-schema-review/SKILL.md:
- Around line 471-490: The reference column width table in the SKILL.md file is
missing the SQL language specifier in its markdown code fence. Locate the code
block starting with the VARCHAR(40) line that documents the column width
standards and add "sql" after the opening triple backticks (changing ``` toconsistency with other code blocks in the document and satisfying markdown linting requirements. - Around line 28-30: Add the bash language specifier to the markdown code blocks in the SKILL.md file. On line 28, change the code block opening from ``` to ```bash for the block containing the cat command that reads the database schema file. Similarly, on line 110, add bash to the code block opening for the block containing the find command that searches for schema files. This ensures markdown linters recognize these as bash code blocks.🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID:
20d3e4b1-e6eb-4c60-b13e-ce5d36579fb0📒 Files selected for processing (4)
.agents/skills/db-schema-review/SKILL.mdplatform-api/src/internal/database/schema.postgres.sqlplatform-api/src/internal/database/schema.sqlplatform-api/src/internal/database/schema.sqlite.sql
Comment on lines
329
to
+330
| FOREIGN KEY (uuid) REFERENCES artifacts(uuid) ON DELETE CASCADE, | ||
| FOREIGN KEY (project_uuid) REFERENCES projects(uuid) ON DELETE CASCADE | ||
| FOREIGN KEY (project_uuid) REFERENCES projects(uuid) ON DELETE CASCADE, |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Use restrictive project deletes for MCP proxies.
project_uuid is nullable by design, but this FK cascades project deletion into mcp_proxies. Change it to ON DELETE RESTRICT so application code can handle the nullable relationship explicitly. Based on learnings, mcp_proxies.project_uuid is intentionally nullable and its FK should use ON DELETE RESTRICT.
Proposed adjustment
- FOREIGN KEY (project_uuid) REFERENCES projects(uuid) ON DELETE CASCADE,
+ FOREIGN KEY (project_uuid) REFERENCES projects(uuid) ON DELETE RESTRICT,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| FOREIGN KEY (uuid) REFERENCES artifacts(uuid) ON DELETE CASCADE, | |
| FOREIGN KEY (project_uuid) REFERENCES projects(uuid) ON DELETE CASCADE | |
| FOREIGN KEY (project_uuid) REFERENCES projects(uuid) ON DELETE CASCADE, | |
| FOREIGN KEY (uuid) REFERENCES artifacts(uuid) ON DELETE CASCADE, | |
| FOREIGN KEY (project_uuid) REFERENCES projects(uuid) ON DELETE RESTRICT, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@platform-api/src/internal/database/schema.postgres.sql` around lines 329 -
330, The foreign key constraint on project_uuid that references projects(uuid)
is currently using ON DELETE CASCADE, but since project_uuid is nullable by
design, change this constraint to ON DELETE RESTRICT instead. This will prevent
automatic cascading deletion of mcp_proxies records when a project is deleted,
allowing the application code to explicitly handle the nullable relationship.
Locate the second FOREIGN KEY line in the diff (the one with project_uuid
REFERENCES projects) and replace ON DELETE CASCADE with ON DELETE RESTRICT.
Source: Learnings
Comment on lines
333
to
+334
| FOREIGN KEY (uuid) REFERENCES artifacts(uuid) ON DELETE CASCADE, | ||
| FOREIGN KEY (project_uuid) REFERENCES projects(uuid) ON DELETE CASCADE | ||
| FOREIGN KEY (project_uuid) REFERENCES projects(uuid) ON DELETE CASCADE, |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Use restrictive project deletes for MCP proxies.
project_uuid is nullable by design, but this FK cascades project deletion into mcp_proxies. Change it to ON DELETE RESTRICT so application code can handle the nullable relationship explicitly. Based on learnings, mcp_proxies.project_uuid is intentionally nullable and its FK should use ON DELETE RESTRICT.
Proposed adjustment
- FOREIGN KEY (project_uuid) REFERENCES projects(uuid) ON DELETE CASCADE,
+ FOREIGN KEY (project_uuid) REFERENCES projects(uuid) ON DELETE RESTRICT,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| FOREIGN KEY (uuid) REFERENCES artifacts(uuid) ON DELETE CASCADE, | |
| FOREIGN KEY (project_uuid) REFERENCES projects(uuid) ON DELETE CASCADE | |
| FOREIGN KEY (project_uuid) REFERENCES projects(uuid) ON DELETE CASCADE, | |
| FOREIGN KEY (uuid) REFERENCES artifacts(uuid) ON DELETE CASCADE, | |
| FOREIGN KEY (project_uuid) REFERENCES projects(uuid) ON DELETE RESTRICT, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@platform-api/src/internal/database/schema.sql` around lines 333 - 334, The
foreign key constraint on project_uuid in the mcp_proxies table currently uses
ON DELETE CASCADE, which will cascade project deletion into mcp_proxies. Since
project_uuid is nullable by design and should be handled explicitly by
application code, change the ON DELETE CASCADE clause for the FOREIGN KEY
(project_uuid) REFERENCES projects(uuid) constraint to ON DELETE RESTRICT
instead. This ensures that projects cannot be deleted if they are referenced by
mcp_proxies records, allowing the application to handle the nullable
relationship properly.
Source: Learnings
Comment on lines
329
to
+330
| FOREIGN KEY (uuid) REFERENCES artifacts(uuid) ON DELETE CASCADE, | ||
| FOREIGN KEY (project_uuid) REFERENCES projects(uuid) ON DELETE CASCADE | ||
| FOREIGN KEY (project_uuid) REFERENCES projects(uuid) ON DELETE CASCADE, |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Use restrictive project deletes for MCP proxies.
project_uuid is nullable by design, but this FK cascades project deletion into mcp_proxies. Change it to ON DELETE RESTRICT so application code can handle the nullable relationship explicitly. Based on learnings, mcp_proxies.project_uuid is intentionally nullable and its FK should use ON DELETE RESTRICT.
Proposed adjustment
- FOREIGN KEY (project_uuid) REFERENCES projects(uuid) ON DELETE CASCADE,
+ FOREIGN KEY (project_uuid) REFERENCES projects(uuid) ON DELETE RESTRICT,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@platform-api/src/internal/database/schema.sqlite.sql` around lines 329 - 330,
The FOREIGN KEY constraint on project_uuid in the mcp_proxies table currently
uses ON DELETE CASCADE, but since project_uuid is intentionally nullable, this
should be changed to ON DELETE RESTRICT to allow application code to explicitly
handle the deletion logic. Locate the FOREIGN KEY constraint that references
projects(uuid) for the project_uuid column in the mcp_proxies table definition
and replace ON DELETE CASCADE with ON DELETE RESTRICT for that specific
constraint only, leaving the other FOREIGN KEY constraint for uuid unchanged.
Source: Learnings
…dempotent DDL - Updated the gateway_association_mappings table to use a composite PRIMARY KEY (organization_uuid, artifact_uuid, gateway_uuid) instead of a UNIQUE constraint for better replication support. - Added new indexes for artifact_uuid and gateway_uuid to improve query performance. - Ensured all CREATE TABLE and CREATE INDEX statements are idempotent across PostgreSQL, SQLite, and SQL Server schemas for consistent deployment.
- Introduced a new skill for managing database schema design rules specific to the WSO2 API Platform, covering table creation, modification, and review processes. - Created a comprehensive reference document outlining the rules (R1–R9) for schema design, including conventions for primary keys, organization scoping, and column types. - Implemented a script for generating structured JSON reports from schema review findings, enhancing the auditing process. - Removed the outdated db-schema-review skill to streamline schema management under the new API platform-specific guidelines.
- Changed the default value of data_version from 'v1.0' to '1.0' across multiple tables for consistency. - Reduced the length of the handle column from VARCHAR(255) to VARCHAR(40) in various tables to enforce uniformity and improve data integrity. - Updated related documentation to reflect these schema changes, ensuring alignment across PostgreSQL, SQLite, and SQL Server implementations.
…traints - Clarified the distinction between `handle` and `name` in the schema design rules, emphasizing that `name` does not require uniqueness within a project. - Updated SQL schema files to remove the `UNIQUE(organization_uuid, project_uuid, name)` constraint, ensuring that only `UNIQUE(organization_uuid, handle)` is enforced. - Adjusted related documentation to reflect these changes, enhancing clarity and consistency across PostgreSQL, SQLite, and SQL Server implementations.
…cription plan services - Enhanced audit logging by ensuring that UUIDs are used consistently for API key updates, revocations, and subscription plan operations. - Updated comments in the constants file for clarity regarding accepted values for managed_by fields. - Removed unused error constants to streamline error handling in the error.go file.
…integrity - Updated SQL queries in the API and deployment repositories to include a 'handle' column alongside 'name' for projects, ensuring consistent data insertion. - Changed the handling of properties and metadata from JSON strings to byte arrays across multiple repository methods, enhancing performance and consistency in data serialization. - Adjusted boolean fields for gateways and deployments to use integers for better compatibility with SQL data types. - Improved error handling and data unmarshalling processes to ensure robustness in data retrieval and manipulation.
|
|
||
| -- Subscription plans table (organization-scoped rate/billing plans) | ||
| CREATE TABLE IF NOT EXISTS subscription_plans ( | ||
| uuid VARCHAR(40) PRIMARY KEY, |
Contributor
There was a problem hiding this comment.
Don't we need a handle?
cc @malinthaprasan, @tharindu1st
Contributor
Author
There was a problem hiding this comment.
also api_keys, does not have a field for name(display name).
…ency - Renamed the 'plan_name' column to 'handle' and added a 'name' column in the subscription_plans table across all database schemas to improve clarity and consistency. - Updated related SQL queries, repository methods, and service logic to reflect the new column names, ensuring all references to subscription plans are aligned with the updated schema. - Enhanced validation and error handling in the subscription plan service to require 'handle' and 'name' fields, improving data integrity and user feedback.
…onsistency - Renamed the 'name' column to 'handle' in the websub_api_hmac_secrets table across all database schemas to improve clarity and consistency. - Updated related SQL queries, repository methods, and service logic to reflect the new column name, ensuring all references to HMAC secrets are aligned with the updated schema. - Adjusted the maximum length of the handle column to 40 characters for uniformity and improved data integrity.
…ormance - Simplified SQL queries across multiple repository files by removing unnecessary JOINs with the artifacts table, enhancing clarity and performance. - Standardized the retrieval of API and proxy data by directly querying their respective tables, ensuring consistent handling of fields such as UUID, handle, name, and organization UUID. - Introduced helper methods for scanning rows into model structures, reducing code duplication and improving maintainability. - Updated related methods to reflect these changes, ensuring all references to API and proxy data are aligned with the new query structure.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
platform-api/src/internal/integration/cascade_test.go (1)
58-59: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winUse the canonical REST artifact type in fixtures.
These seeds use
"rest_api", while the updated deployment code classifies REST artifacts as"RestApi"and this file uses"WebSubApi"for WebSub. Align the REST fixtures with the canonical artifact type.Proposed fix
- it.exec(t, `INSERT INTO artifacts (uuid, type, organization_uuid) VALUES (?, ?, ?)`, - g.apiArtifact, "rest_api", g.org) + it.exec(t, `INSERT INTO artifacts (uuid, type, organization_uuid) VALUES (?, ?, ?)`, + g.apiArtifact, "RestApi", g.org) @@ - it.exec(t, `INSERT INTO artifacts (uuid, type, organization_uuid) VALUES (?, ?, ?)`, - g.depArtifact, "rest_api", g.org) + it.exec(t, `INSERT INTO artifacts (uuid, type, organization_uuid) VALUES (?, ?, ?)`, + g.depArtifact, "RestApi", g.org)Also applies to: 72-73
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@platform-api/src/internal/integration/cascade_test.go` around lines 58 - 59, The REST fixture seeds are still using the legacy artifact type string, while the deployment and WebSub fixtures use the canonical casing. Update the INSERTs in cascade_test.go that seed REST artifacts to use the same canonical REST artifact type as the updated classification logic, and make sure both occurrences in the test setup stay consistent with the artifact type constants used elsewhere.platform-api/src/internal/repository/llm.go (1)
353-364: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReturn
updated_bywhere the repository writes it.Provider/proxy updates persist
updated_by, but get/list projections only scancreated_by, so returned models lose updater attribution.Proposed projection update pattern
- description, created_by, template_uuid, openapi_spec, model_list, configuration + description, created_by, updated_by, template_uuid, openapi_spec, model_list, configuration @@ - &p.Description, &p.CreatedBy, &p.TemplateUUID, &openAPISpec, &modelProvidersRaw, &configurationJSON, + &p.Description, &p.CreatedBy, &p.UpdatedBy, &p.TemplateUUID, &openAPISpec, &modelProvidersRaw, &configurationJSON,- project_uuid, description, created_by, provider_uuid, + project_uuid, description, created_by, updated_by, provider_uuid, @@ - &p.ProjectUUID, &p.Description, &p.CreatedBy, &p.ProviderUUID, + &p.ProjectUUID, &p.Description, &p.CreatedBy, &p.UpdatedBy, &p.ProviderUUID,Also applies to: 397-417, 480-483, 607-618, 645-665, 690-710, 735-755, 832-838
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@platform-api/src/internal/repository/llm.go` around lines 353 - 364, The llm repository projections are dropping updater attribution because the query/scan paths still only include created_by. Update the SELECT lists and matching row.Scan calls in the relevant LLM provider/proxy read methods (such as the provider lookup and list helpers) to also return and populate updated_by on model.LLMProvider wherever it is written by updates, keeping the column order and scan targets aligned.
🧹 Nitpick comments (1)
platform-api/src/internal/repository/websub_api_hmac_secret.go (1)
129-131: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename the delete argument to
handle.The SQL now deletes by
handle, but the method parameter is still namedname, which makes this handle/name migration easy to misuse.Proposed cleanup
-func (r *WebSubAPIHmacSecretRepo) Delete(artifactUUID, name string) error { +func (r *WebSubAPIHmacSecretRepo) Delete(artifactUUID, handle string) error { query := `DELETE FROM websub_api_hmac_secrets WHERE artifact_uuid = ? AND handle = ?` - result, err := r.db.Exec(r.db.Rebind(query), artifactUUID, name) + result, err := r.db.Exec(r.db.Rebind(query), artifactUUID, handle)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@platform-api/src/internal/repository/websub_api_hmac_secret.go` around lines 129 - 131, The Delete method in WebSubAPIHmacSecretRepo still uses a parameter named name even though the SQL filters by handle, which can cause misuse during the handle/name migration. Rename the Delete argument to handle and update the Exec call in Delete to pass handle so the method signature, query, and semantics all match.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
@.agents/skills/api-platform-db-schema-design-rules/scripts/generate-schema-report.js:
- Around line 61-85: The normalization step in generate-schema-report.js assumes
every finding has a valid object shape and string rule, so malformed entries can
crash the report and bad severities can affect counts. Update the findings
processing before the current normalised map to validate each entry, default or
reject invalid rule/severity values, and only pass sanitized findings into the
normalization logic. Keep the fixes localized around the normalised
transformation, the severity ordering, and the summary aggregation so the script
never sees unexpected entry shapes.
In @.agents/skills/api-platform-db-schema-design-rules/SKILL.md:
- Around line 261-264: The binary-payload guidance in the schema design rules
incorrectly includes metadata, which should remain TEXT by default. Update the
guidance in SKILL.md so the BYTEA/BLOB/VARBINARY(MAX) list references the binary
or large payload fields only, and explicitly remove metadata from that list
unless a documented repository-wide migration plan exists. Use the existing
schema guidance section around openapi_spec, model_list, content, configuration,
properties, manifest, policy_definition, and api_key_hashes to place the
correction.
- Around line 209-219: Remove the global uniqueness constraint from the template
table definition by updating the column declaration for handle so it is not
marked UNIQUE, while keeping the existing composite constraint
UNIQUE(organization_uuid, handle). Make this change in the schema snippet that
defines the handle, name, version, and related columns so handles remain unique
per organization only.
In `@platform-api/src/internal/database/schema.postgres.sql`:
- Line 243: The schema definition for the deployment metadata column was changed
to BYTEA, but it should remain TEXT to match the repository’s metadata storage
convention. Update the `metadata` field in the postgres schema definition back
to TEXT in the relevant schema file and keep the surrounding deployment schema
entries unchanged unless there is a repo-wide migration plan tied to `metadata`.
In `@platform-api/src/internal/database/schema.sqlite.sql`:
- Around line 118-119: The subscription plan table is missing a matching parent
key for the composite foreign key from subscriptions, so update the
subscription_plans definition to expose a UNIQUE or PRIMARY KEY constraint on
the exact referenced columns (uuid, organization_uuid). Make this change in the
subscription_plans schema block and apply the same fix to the other matching
occurrence so the subscriptions FK can resolve correctly; use the
subscription_plans and subscriptions table definitions as the anchors.
- Line 243: The schema change in the metadata column should be reverted because
repository convention keeps deployment metadata as TEXT. Update the relevant
SQLite schema definition for metadata back to TEXT in the schema file, and keep
the existing metadata storage type consistent with the rest of the repository
unless there is an approved migration plan.
- Line 35: The projects table is using an oversized handle column, so align the
SQLite schema with the project standard used by the handle validator and the
Postgres schema. Update the projects.handle definition in the schema setup to
use the same 40-character width as the rest of the codebase, keeping the schema
consistent across databases.
In `@platform-api/src/internal/handler/subscription_plan_handler.go`:
- Line 77: The StopOnQuotaReach field in subscription plan request binding is
currently accepted as any int, so add explicit validation in the request
handling paths for create/update using the subscription_plan_handler methods
that bind or map this field. Enforce that only 0 or 1 are allowed before passing
the value to the service, and reject any other integer values consistently
across the affected request structs and handlers.
In `@platform-api/src/internal/repository/artifact.go`:
- Around line 103-116: The handle-based lookup in
ArtifactRepo.GetAPIMetadataByHandle is ambiguous because it unions multiple type
tables using only handle and organization_uuid, so the returned metadata can
come from the wrong artifact type. Update the API to disambiguate by either
enforcing cross-type uniqueness at write time or requiring an explicit type
filter in the lookup path, and apply the same fix to the other union-based
lookup in the related method mentioned in the diff.
- Line 148: The query in the artifact repository still uses a dialect-specific
LIMIT 1, which breaks on SQL Server. Update the query in the artifact repository
method that builds this UNION-based lookup to use r.db.FetchFirstClause(1)
instead, so the same repository code remains compatible across PostgreSQL,
SQLite, and SQL Server while still guaranteeing a single row for QueryRow.
In `@platform-api/src/internal/repository/mcp.go`:
- Around line 164-173: `MCPProxyRepo.List` is ignoring its `limit` and `offset`
arguments, so pagination is broken. Update the SQL in `List` to apply paging for
the organization query, and make sure the `limit`/`offset` parameters are passed
through with the existing `r.db.Rebind(query)` call so callers only receive the
requested page of MCP proxies.
In `@platform-api/src/internal/service/api.go`:
- Around line 560-565: The subscription plan lookup in GetByHandleAndOrg
currently returns raw repository errors for missing handles instead of the
domain-level ErrSubscriptionPlanNotFoundOrInactive. Update the error handling in
the subscription plan flow in api.go so sql.ErrNoRows from
s.subscriptionPlanRepo.GetByHandleAndOrg is translated into
constants.ErrSubscriptionPlanNotFoundOrInactive, while preserving other errors
unchanged; keep the existing inactive/nil plan check and use the same
handle-based error wrapping for the normalized not-found path.
In `@platform-api/src/internal/service/subscription_plan_service.go`:
- Around line 145-156: Update validation in UpdatePlan to match CreatePlan by
rejecting empty handle and name values before applying them to the existing
plan. In subscription_plan_service.go, add checks around the update.Handle and
update.Name branches so that if either pointer is non-nil but points to an empty
string, the method returns a validation error instead of assigning it. Keep the
existing duplicate-handle lookup and reuse the same service flow around
UpdatePlan and CreatePlan so both paths enforce the same non-empty rules.
---
Outside diff comments:
In `@platform-api/src/internal/integration/cascade_test.go`:
- Around line 58-59: The REST fixture seeds are still using the legacy artifact
type string, while the deployment and WebSub fixtures use the canonical casing.
Update the INSERTs in cascade_test.go that seed REST artifacts to use the same
canonical REST artifact type as the updated classification logic, and make sure
both occurrences in the test setup stay consistent with the artifact type
constants used elsewhere.
In `@platform-api/src/internal/repository/llm.go`:
- Around line 353-364: The llm repository projections are dropping updater
attribution because the query/scan paths still only include created_by. Update
the SELECT lists and matching row.Scan calls in the relevant LLM provider/proxy
read methods (such as the provider lookup and list helpers) to also return and
populate updated_by on model.LLMProvider wherever it is written by updates,
keeping the column order and scan targets aligned.
---
Nitpick comments:
In `@platform-api/src/internal/repository/websub_api_hmac_secret.go`:
- Around line 129-131: The Delete method in WebSubAPIHmacSecretRepo still uses a
parameter named name even though the SQL filters by handle, which can cause
misuse during the handle/name migration. Rename the Delete argument to handle
and update the Exec call in Delete to pass handle so the method signature,
query, and semantics all match.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8686e054-8b76-44cd-aba3-9c47b9821f35
📒 Files selected for processing (41)
.agents/skills/api-platform-db-schema-design-rules/SKILL.md.agents/skills/api-platform-db-schema-design-rules/references/api-platform-db-schema-rules.md.agents/skills/api-platform-db-schema-design-rules/scripts/generate-schema-report.jsplatform-api/src/internal/constants/constants.goplatform-api/src/internal/constants/error.goplatform-api/src/internal/database/schema.postgres.sqlplatform-api/src/internal/database/schema.sqlplatform-api/src/internal/database/schema.sqlite.sqlplatform-api/src/internal/database/schema.sqlserver.sqlplatform-api/src/internal/dto/gateway_internal.goplatform-api/src/internal/handler/gateway_internal.goplatform-api/src/internal/handler/subscription_handler.goplatform-api/src/internal/handler/subscription_plan_handler.goplatform-api/src/internal/handler/websub_api_hmac_secret.goplatform-api/src/internal/integration/cascade_test.goplatform-api/src/internal/integration/lifecycle_test.goplatform-api/src/internal/model/subscription_plan.goplatform-api/src/internal/model/subscription_plan_event.goplatform-api/src/internal/model/websub_api_hmac_secret.goplatform-api/src/internal/repository/api.goplatform-api/src/internal/repository/api_deployments_test.goplatform-api/src/internal/repository/api_test.goplatform-api/src/internal/repository/artifact.goplatform-api/src/internal/repository/deployment.goplatform-api/src/internal/repository/gateway.goplatform-api/src/internal/repository/interfaces.goplatform-api/src/internal/repository/llm.goplatform-api/src/internal/repository/mcp.goplatform-api/src/internal/repository/subscription_plan_repository.goplatform-api/src/internal/repository/webbroker_api.goplatform-api/src/internal/repository/websub_api.goplatform-api/src/internal/repository/websub_api_hmac_secret.goplatform-api/src/internal/service/api.goplatform-api/src/internal/service/apikey.goplatform-api/src/internal/service/gateway_internal.goplatform-api/src/internal/service/subscription_plan_service.goplatform-api/src/internal/service/webbroker_api.goplatform-api/src/internal/service/websub_api.goplatform-api/src/internal/service/websub_api_hmac_secret.goplatform-api/src/internal/service/websub_api_hmac_secret_test.goplatform-api/src/internal/utils/handle.go
💤 Files with no reviewable changes (1)
- platform-api/src/internal/constants/error.go
✅ Files skipped from review due to trivial changes (1)
- platform-api/src/internal/handler/gateway_internal.go
🚧 Files skipped from review as they are similar to previous changes (12)
- platform-api/src/internal/dto/gateway_internal.go
- platform-api/src/internal/constants/constants.go
- platform-api/src/internal/repository/api_deployments_test.go
- platform-api/src/internal/repository/api_test.go
- platform-api/src/internal/service/gateway_internal.go
- platform-api/src/internal/integration/lifecycle_test.go
- platform-api/src/internal/service/webbroker_api.go
- platform-api/src/internal/service/websub_api.go
- platform-api/src/internal/database/schema.sqlserver.sql
- platform-api/src/internal/repository/gateway.go
- platform-api/src/internal/repository/api.go
- platform-api/src/internal/database/schema.sql
Comment on lines
+61
to
+85
| const normalised = findings.map(f => { | ||
| const ruleKey = (f.rule || 'UNKNOWN').toLowerCase().replace(/[^a-z0-9]/g, '-'); | ||
| counters[ruleKey] = (counters[ruleKey] || 0) + 1; | ||
| const seq = String(counters[ruleKey]).padStart(3, '0'); | ||
| return { | ||
| id: `${ruleKey}-${seq}`, | ||
| severity: f.severity || 'MEDIUM', | ||
| rule: f.rule || 'UNKNOWN', | ||
| table: f.table || null, | ||
| column: f.column || null, | ||
| finding: f.finding || '', | ||
| fix: f.fix || '', | ||
| }; | ||
| }); | ||
|
|
||
| // Sort: HIGH → MEDIUM → LOW | ||
| const ORDER = { HIGH: 0, MEDIUM: 1, LOW: 2 }; | ||
| normalised.sort((a, b) => (ORDER[a.severity] ?? 3) - (ORDER[b.severity] ?? 3)); | ||
|
|
||
| // Collect unique rule groups checked | ||
| const ruleGroups = [...new Set(normalised.map(f => f.rule.split('-')[0]))].sort(); | ||
|
|
||
| // Summary counts | ||
| const summary = { HIGH: 0, MEDIUM: 0, LOW: 0 }; | ||
| for (const f of normalised) summary[f.severity] = (summary[f.severity] || 0) + 1; |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Validate each finding entry before normalizing.
map() assumes every element is a well-formed object with a string rule; a malformed entry will currently throw during normalization, and unexpected severities can leak into the summary. Reject or normalize invalid entries up front.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
@.agents/skills/api-platform-db-schema-design-rules/scripts/generate-schema-report.js
around lines 61 - 85, The normalization step in generate-schema-report.js
assumes every finding has a valid object shape and string rule, so malformed
entries can crash the report and bad severities can affect counts. Update the
findings processing before the current normalised map to validate each entry,
default or reject invalid rule/severity values, and only pass sanitized findings
into the normalization logic. Keep the fixes localized around the normalised
transformation, the severity ordering, and the summary aggregation so the script
never sees unexpected entry shapes.
Comment on lines
+209
to
+219
| handle VARCHAR(40) UNIQUE NOT NULL, | ||
| name VARCHAR(255) NOT NULL, | ||
| version VARCHAR(30) NOT NULL DEFAULT '1.0', | ||
| status VARCHAR(20) NOT NULL DEFAULT 'CREATED', | ||
| description VARCHAR(1023), | ||
| data_version VARCHAR(20) NOT NULL DEFAULT '1.0', | ||
| created_by VARCHAR(200), | ||
| created_at TIMESTAMPTZ DEFAULT CURRENT_TIMESTAMP, | ||
| updated_by VARCHAR(200), | ||
| updated_at TIMESTAMPTZ DEFAULT CURRENT_TIMESTAMP, | ||
| UNIQUE(organization_uuid, handle), |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Remove the global handle uniqueness from the template.
The table already has UNIQUE(organization_uuid, handle). Keeping handle VARCHAR(40) UNIQUE NOT NULL would make handles globally unique across organizations.
Proposed adjustment
- handle VARCHAR(40) UNIQUE NOT NULL,
+ handle VARCHAR(40) NOT NULL,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| handle VARCHAR(40) UNIQUE NOT NULL, | |
| name VARCHAR(255) NOT NULL, | |
| version VARCHAR(30) NOT NULL DEFAULT '1.0', | |
| status VARCHAR(20) NOT NULL DEFAULT 'CREATED', | |
| description VARCHAR(1023), | |
| data_version VARCHAR(20) NOT NULL DEFAULT '1.0', | |
| created_by VARCHAR(200), | |
| created_at TIMESTAMPTZ DEFAULT CURRENT_TIMESTAMP, | |
| updated_by VARCHAR(200), | |
| updated_at TIMESTAMPTZ DEFAULT CURRENT_TIMESTAMP, | |
| UNIQUE(organization_uuid, handle), | |
| handle VARCHAR(40) NOT NULL, | |
| name VARCHAR(255) NOT NULL, | |
| version VARCHAR(30) NOT NULL DEFAULT '1.0', | |
| status VARCHAR(20) NOT NULL DEFAULT 'CREATED', | |
| description VARCHAR(1023), | |
| data_version VARCHAR(20) NOT NULL DEFAULT '1.0', | |
| created_by VARCHAR(200), | |
| created_at TIMESTAMPTZ DEFAULT CURRENT_TIMESTAMP, | |
| updated_by VARCHAR(200), | |
| updated_at TIMESTAMPTZ DEFAULT CURRENT_TIMESTAMP, | |
| UNIQUE(organization_uuid, handle), |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.agents/skills/api-platform-db-schema-design-rules/SKILL.md around lines 209
- 219, Remove the global uniqueness constraint from the template table
definition by updating the column declaration for handle so it is not marked
UNIQUE, while keeping the existing composite constraint
UNIQUE(organization_uuid, handle). Make this change in the schema snippet that
defines the handle, name, version, and related columns so handles remain unique
per organization only.
Comment on lines
+261
to
+264
| BYTEA (Postgres) / BLOB (SQLite) / VARBINARY(MAX) (SQL Server) | ||
| — openapi_spec, model_list, content, configuration, properties, | ||
| manifest, policy_definition, metadata, api_key_hashes, | ||
| and any payload that can exceed a few hundred bytes |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Keep metadata out of the binary-payload guidance.
The skill should not direct future schema changes to convert metadata columns to BYTEA/BLOB.
Proposed adjustment
- manifest, policy_definition, metadata, api_key_hashes,
+ manifest, policy_definition, api_key_hashes,Based on learnings, metadata columns in these schema files are intentionally kept as TEXT unless a documented repository-wide migration plan exists.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| BYTEA (Postgres) / BLOB (SQLite) / VARBINARY(MAX) (SQL Server) | |
| — openapi_spec, model_list, content, configuration, properties, | |
| manifest, policy_definition, metadata, api_key_hashes, | |
| and any payload that can exceed a few hundred bytes | |
| BYTEA (Postgres) / BLOB (SQLite) / VARBINARY(MAX) (SQL Server) | |
| — openapi_spec, model_list, content, configuration, properties, | |
| manifest, policy_definition, api_key_hashes, | |
| and any payload that can exceed a few hundred bytes |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.agents/skills/api-platform-db-schema-design-rules/SKILL.md around lines 261
- 264, The binary-payload guidance in the schema design rules incorrectly
includes metadata, which should remain TEXT by default. Update the guidance in
SKILL.md so the BYTEA/BLOB/VARBINARY(MAX) list references the binary or large
payload fields only, and explicitly remove metadata from that list unless a
documented repository-wide migration plan exists. Use the existing schema
guidance section around openapi_spec, model_list, content, configuration,
properties, manifest, policy_definition, and api_key_hashes to place the
correction.
Source: Learnings
| content BYTEA NOT NULL, | ||
| metadata TEXT, -- JSON object as TEXT | ||
| created_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP, | ||
| metadata BYTEA, |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Keep deployment metadata as TEXT.
Changing metadata to BYTEA breaks the repository convention for metadata storage.
Proposed adjustment
- metadata BYTEA,
+ metadata TEXT,Based on learnings, metadata columns in these schema files are intentionally defined as TEXT unless a documented repository-wide migration plan exists.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| metadata BYTEA, | |
| metadata TEXT, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@platform-api/src/internal/database/schema.postgres.sql` at line 243, The
schema definition for the deployment metadata column was changed to BYTEA, but
it should remain TEXT to match the repository’s metadata storage convention.
Update the `metadata` field in the postgres schema definition back to TEXT in
the relevant schema file and keep the surrounding deployment schema entries
unchanged unless there is a repo-wide migration plan tied to `metadata`.
Source: Learnings
Contributor
Author
There was a problem hiding this comment.
what is the source of learning?
Contributor
There was a problem hiding this comment.
@Thushani-Jayasekera The source was a previously stored CodeRabbit learning for this repository.
I used this retrieved learning in the earlier comment:
- Learned from:
ShakyaPr - Repo:
wso2/api-platform - PR:
#2264 - File context:
platform-api/src/internal/database/schema.postgres.sql - Recorded at:
2026-06-24T13:47:37.190Z
The learning states that, in this repo, metadata columns in the SQL schema are intentionally kept as TEXT for consistency, including examples such as deployments and gateway_associations, unless there is a repository-wide documented migration plan.
So the comment was based on that prior repository-specific context, not on a new source from this PR alone. If you want, I can re-evaluate the comment strictly against the current PR context and withdraw it if this change is intentionally part of a broader storage-format migration.
🧠 Learnings used
Learnt from: Arshardh
Repo: wso2/api-platform PR: 841
File: platform-api/src/internal/database/schema.postgres.sql:345-423
Timestamp: 2026-02-01T19:10:45.800Z
Learning: In SQL schema files under the platform-api module, avoid relying on database-level foreign key cascades to handle orphaned LLM policies. Ensure llm_policies references only organization_uuid and do not configure ON DELETE CASCADE for related tables (e.g., llm_providers, llm_proxies). Orphan handling should be implemented at the application layer for this domain to preserve schema flexibility.
Learnt from: RakhithaRR
Repo: wso2/api-platform PR: 989
File: platform-api/src/internal/database/schema.postgres.sql:40-53
Timestamp: 2026-02-09T05:23:35.668Z
Learning: In platform-api/src/internal/database/schema.postgres.sql, ensure the artifacts table enforces a global unique constraint on (name, version, organization_uuid) across all artifact kinds (e.g., RestApi, LLMProvider, LLMProxy). This prevents different artifact kinds within the same organization from sharing the same name+version. If this constraint is not present, add a unique constraint like UNIQUE(name, version, organization_uuid) or a composite unique index, and ensure existing data respects it before applying to maintain data integrity.
Learnt from: CrowleyRajapakse
Repo: wso2/api-platform PR: 1324
File: platform-api/src/internal/database/schema.postgres.sql:52-56
Timestamp: 2026-03-11T03:28:23.264Z
Learning: In the wso2/api-platform repository, treat the initial platform-api schema (platform-api/src/internal/database/schema.postgres.sql and related files) as a first-time initialization. Do not apply ALTER TABLE migrations yet; use CREATE TABLE IF NOT EXISTS for new tables to initialize the schema. Once tables exist, migrate with ALTER TABLE and other DDL changes in separate migration steps.
Learnt from: RakhithaRR
Repo: wso2/api-platform PR: 1332
File: platform-api/src/internal/database/schema.sql:294-303
Timestamp: 2026-03-12T06:37:06.133Z
Learning: In platform-api/src/internal/database/schema.sql and its variants (schema.sqlite.sql, schema.postgres.sql), mcp_proxies.project_uuid is intentionally nullable. The FK uses ON DELETE RESTRICT, so the application must nullify or delete mcp_proxy rows referencing a project before the project can be deleted. Do not flag the nullable project_uuid as an issue in future reviews; ensure reviewers understand this is by design and the code paths handle the nullable relationship.
Learnt from: ShalkiWenushika
Repo: wso2/api-platform PR: 1342
File: platform-api/src/internal/database/schema.postgres.sql:307-323
Timestamp: 2026-03-15T16:47:37.764Z
Learning: In the wso2/api-platform repository (platform-api project), for the api_keys table defined in platform-api/src/internal/database/schema.postgres.sql and its variants (schema.sql, schema.sqlite.sql), do not suggest or enforce CHECK constraints on the status column. Status validation is handled at the application layer and should not be implemented via database constraints. Apply this guidance to review of all SQL files in platform-api/src/internal/database/*.sql and related schema files; if a proposal to add such constraints appears, it should be rejected or gated by automated validation as appropriate.
Learnt from: malinthaprasan
Repo: wso2/api-platform PR: 1373
File: platform-api/src/internal/database/schema.postgres.sql:177-186
Timestamp: 2026-03-17T01:47:33.039Z
Learning: In the wso2/api-platform repository, database schema migrations (e.g., ALTER TABLE for new columns like those added to deployment_status in platform-api/src/internal/database/schema.postgres.sql) are handled automatically by an external migration process, not from within the application code. Do not flag missing in-code ALTER TABLE migrations for existing tables as issues in future reviews. This guidance applies to all schema.postgres.sql files and similar migration definitions; rely on the external migration tooling and CI checks to validate and apply changes.
Learnt from: malinthaprasan
Repo: wso2/api-platform PR: 1373
File: platform-api/src/internal/database/schema.sql:194-197
Timestamp: 2026-03-17T08:17:18.902Z
Learning: Do not add CHECK constraints for deployment_status.status or deployment_status.status_desired in any schema.*.sql files (e.g., schema.sql, schema.sqlite.sql, schema.postgres.sql). Status validation is handled at the application layer. Do not propose adding these CHECK constraints in future reviews.
Learnt from: renuka-fernando
Repo: wso2/api-platform PR: 1888
File: platform-api/src/internal/database/schema.postgres.sql:148-148
Timestamp: 2026-05-07T06:15:52.679Z
Learning: In this repository’s platform-api database schema SQL files (e.g., schema.sql, schema.postgres.sql, schema.sqlite.sql), the `gateways.version` column uses a two-segment `major.minor` format such as "1.0" (not three-segment SemVer). The registration validation regex `^[0-9]{1,6}\.[0-9]{1,6}$` and version comparison via `extractMajorMinor()` normalize both sides to major.minor, and `legacyGatewayVersion = "1.0.0"` is only a transient in-memory fallback for manifest push (not persisted to the DB). Therefore, when reviewing, do not flag the DB DEFAULT (e.g., '1.0') or stored `gateways.version` values as inconsistent with SemVer expectations; they are expected to be major.minor.
Learnt from: ShakyaPr
Repo: wso2/api-platform PR: 2264
File: platform-api/src/internal/database/schema.postgres.sql:0-0
Timestamp: 2026-06-24T13:47:37.190Z
Learning: In wso2/api-platform, the `metadata` columns in the SQL schema are intentionally defined as `TEXT` (not `JSONB`) for consistency (e.g., in tables like `deployments` and `gateway_associations`). When reviewing changes to any of the SQL schema files (`schema.sql`, `schema.postgres.sql`, `schema.sqlite.sql`), do not suggest altering these `metadata` columns to `JSONB`; keep them as `TEXT` unless a repository-wide, explicitly documented migration/compatibility plan is provided.
Comment on lines
+103
to
+116
| func (r *ArtifactRepo) GetAPIMetadataByHandle(handle, orgUUID string) (*model.APIMetadata, error) { | ||
| query := ` | ||
| SELECT uuid, handle, name, version, type, organization_uuid | ||
| FROM ( | ||
| SELECT uuid, handle, name, version, 'RestApi' AS type, organization_uuid FROM rest_apis WHERE handle = ? AND organization_uuid = ? | ||
| UNION ALL | ||
| SELECT uuid, handle, name, version, 'LlmProxy' AS type, organization_uuid FROM llm_proxies WHERE handle = ? AND organization_uuid = ? | ||
| UNION ALL | ||
| SELECT uuid, handle, name, version, 'Mcp' AS type, organization_uuid FROM mcp_proxies WHERE handle = ? AND organization_uuid = ? | ||
| UNION ALL | ||
| SELECT uuid, handle, name, version, 'WebSubApi' AS type, organization_uuid FROM websub_apis WHERE handle = ? AND organization_uuid = ? | ||
| UNION ALL | ||
| SELECT uuid, handle, name, version, 'WebBrokerApi' AS type, organization_uuid FROM webbroker_apis WHERE handle = ? AND organization_uuid = ? | ||
| ) combined |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make handle resolution unambiguous across artifact types.
These UNION lookups resolve by handle and organization only. If two type tables can contain the same handle in an organization, callers may receive the wrong artifact/API metadata. Enforce cross-type uniqueness before insert, or require the artifact type in these lookup APIs.
Also applies to: 133-148
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@platform-api/src/internal/repository/artifact.go` around lines 103 - 116, The
handle-based lookup in ArtifactRepo.GetAPIMetadataByHandle is ambiguous because
it unions multiple type tables using only handle and organization_uuid, so the
returned metadata can come from the wrong artifact type. Update the API to
disambiguate by either enforcing cross-type uniqueness at write time or
requiring an explicit type filter in the lookup path, and apply the same fix to
the other union-based lookup in the related method mentioned in the diff.
…bles - Changed the default value of the 'version' column from '1.0' to 'v1.0' in multiple tables, including rest_apis, gateways, and llm_provider_templates, to ensure consistency in versioning format. - Added 'organization_uuid' to the llm_provider_templates table to enhance data integrity and relationships. - Updated related SQL schema files for PostgreSQL, SQLite, and SQL Server to reflect these changes, ensuring alignment across implementations.
- Introduced a new GroupID field in the LLMProviderTemplate model to enhance data organization. - Updated the Create, GetByID, and GetByUUID repository methods to handle the new GroupID field, ensuring it is included in SQL queries and defaulted to the ID if not provided. - Adjusted SQL queries in the List method to incorporate the GroupID, maintaining consistency across data retrieval operations.
AnuGayan
reviewed
Jun 25, 2026
AnuGayan
left a comment
AnuGayan
left a comment
Contributor
There was a problem hiding this comment.
Code review — DB schema refactor + DevPortal removal
Reviewed the full diff across all four DB backends and the Go repository/service/handler layers. The renames (api_uuid→artifact_uuid, display_name→handle) are applied consistently and the DevPortal removal is clean. I've left inline comments at each location; summary below.
🔴 Blockers
subscription_planslost the UNIQUE its composite FK needs — breaksCREATE TABLE subscriptionson Postgres/SQL Server and subscription inserts on SQLite (verified locally). All four schema files.- Raw
LIMIT 1inrepository/application.go(×2) — fails on SQL Server, plus lost deterministic UUID-preference ordering.
🟠 Major
- Nullable
created_by/updated_byscanned into plainstring→converting NULL to string(subscription_repository.goGetByID,project.go,gateway.go,custom_policy.go). - Binary columns (
configuration/openapi_spec/model_list) written/read as Gostringinllm.go/mcp.go/websub_api.go/webbroker_api.go— SQL ServerVARBINARY(MAX)portability hazard + inconsistent withgateway.go/deployment.go. schema.sqlserver.sqlkeepsCHECKconstraints (subscriptions/gateways/gateway_tokens) the other three backends dropped → divergent validation.schema.sqlleftencrypted_secretasTEXTwhile all other backends use a binary type.subscriptionsuniqueness re-keyed onto the nullableapplication_id→ loses subscriber dedup and behaves differently on SQL Server vs others.
🟡 Minor / Nits
projects.handleisVARCHAR(255)on mysql/sqlite/sqlserver butVARCHAR(40)on postgres (every other handle is 40).gateway.goGetActiveTokenByHashhardcodesstatus = 'active'instead of the constant.handler/webbroker_api.gokeeps dead commented-outPublishToDevPortal/UnpublishFromDevPortalstubs (deleted outright inwebsub_api.go).repository/api.goGetAPIAssociations/UpdateAPIAssociationretain an unusedassociationTypeparam.- Stale
DefaultDevPortalconfig remains inconfig/config.go+config/default_config.goafter its consumer was removed. platform-api/src/api/generated.gonot regenerated —openapi.yamlwas stripped of DevPortal/publication, but the committed generated code still contains those types; the nextmake generatewill diverge. Please runmake generateand commit it.
What's good
Careful cross-backend work (SQL Server multiple-cascade-path avoidance, idempotency guards, new supporting indexes), consistent renames end-to-end, correct SMALLINT boolean handling in gateway.go/api.go, and a clean DevPortal removal across server wiring, interfaces, constants, openapi.yaml, and roles.yaml.
| FOREIGN KEY (artifact_uuid) REFERENCES artifacts(uuid) ON DELETE CASCADE, | ||
| FOREIGN KEY (organization_uuid) REFERENCES organizations(uuid) ON DELETE CASCADE, | ||
| FOREIGN KEY (subscription_plan_uuid, organization_uuid) | ||
| REFERENCES subscription_plans(uuid, organization_uuid) ON DELETE RESTRICT, |
Contributor
There was a problem hiding this comment.
🔴 Blocker — composite FK references a column set with no matching UNIQUE constraint
Problem: subscriptions declares FOREIGN KEY (subscription_plan_uuid, organization_uuid) REFERENCES subscription_plans(uuid, organization_uuid), but subscription_plans now only has PRIMARY KEY(uuid) and UNIQUE(organization_uuid, handle). A composite FK requires the referenced columns to be covered by a PK/UNIQUE constraint — that constraint no longer exists.
How it happened: this refactor dropped UNIQUE(uuid, organization_uuid) from subscription_plans. The sibling artifacts table kept its equivalent (renamed to UNIQUE(organization_uuid, uuid)), which is why the subscriptions → artifacts composite FK still works — but the subscription_plans one was not preserved.
Impact (verified): PostgreSQL & SQL Server fail at CREATE TABLE subscriptions ("there is no unique constraint matching given keys for referenced table subscription_plans") — the schema cannot bootstrap. On SQLite, inserts into subscriptions that reference a plan fail with foreign key mismatch (reproduced locally).
How to fix: re-add UNIQUE(organization_uuid, uuid) to subscription_plans in all four schema files (order-independent for the FK match), or drop this composite FK and keep only the single-column subscription_plan_uuid FK with app-level org-consistency checks.
Contributor
Author
|
TODOs: |
…ty and consistency - Added a unique constraint on (organization_uuid, uuid) in the subscription_plans table to ensure uniqueness. - Updated the subscriptions table to enforce a unique index on (organization_uuid, artifact_uuid, application_id) for better subscription management. - Adjusted the handle column length in the projects table to 40 characters for uniformity. - Refactored repository methods across multiple files to include createdBy and updatedBy fields, improving tracking of record ownership. - Enhanced SQL queries to utilize byte arrays for JSON configurations, ensuring better performance and data handling.
…ta integrity - Removed the unique constraint on (organization_uuid, name) in the projects table across all SQL schema files to streamline uniqueness requirements. - Updated the subscription plan handler to validate the 'stopOnQuotaReach' field, ensuring it only accepts values 0 or 1. - Enhanced error handling in the subscription plan service to require non-empty values for 'handle' and 'name' fields during updates.
…larity - Renamed method calls in lifecycle_test.go from ExistsByNameAndOrg to ExistsByHandleAndOrg for better alignment with the updated repository method naming. - Enhanced LLM provider service initialization in llm_test.go by adding a noopAuditRepo to the service constructor, improving audit tracking capabilities.
- Added a new 'Handle' field to the Project model for better project identification. - Updated SQL queries and repository methods to include the 'Handle' field in project creation and retrieval processes. - Modified tests to accommodate the new 'Handle' field, ensuring consistency across project-related operations. - Implemented handle generation logic in the project service to prevent duplicates within the same organization.
- Added tests to confirm that throttle limits are correctly stored and retrieved in subscription plans. - Implemented checks for the proper handling of throttle limits during updates, ensuring limits are cleared as expected. - Enhanced error reporting in tests for better debugging and clarity.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
platform-api/src/internal/repository/api.go (1)
106-121: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCheck the
Scanerror before consuming scanned values.The
Scanerror is evaluated at Line 116, butcreatedBy/updatedByare already read and assigned at Lines 110-114. Move the error handling immediately afterScanso field assignment only runs on the success path.Proposed reordering
err := r.db.QueryRow(r.db.Rebind(query), apiUUID, orgUUID).Scan( &api.ID, &api.Handle, &api.Name, &api.Description, &api.Version, &createdBy, &updatedBy, &api.ProjectID, &api.OrganizationID, &api.LifeCycleStatus, &configJSON, &api.CreatedAt, &api.UpdatedAt) - api.Kind = constants.RestApi - api.CreatedBy = createdBy.String - if updatedBy.Valid { - api.UpdatedBy = updatedBy.String - } - if err != nil { if errors.Is(err, sql.ErrNoRows) { return nil, nil } return nil, err } + api.Kind = constants.RestApi + api.CreatedBy = createdBy.String + if updatedBy.Valid { + api.UpdatedBy = updatedBy.String + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@platform-api/src/internal/repository/api.go` around lines 106 - 121, The query result handling in the repository method that uses r.db.QueryRow(...).Scan should check the Scan error before reading any scanned values. Move the err handling for sql.ErrNoRows and other failures immediately after Scan, then assign api.Kind, api.CreatedBy, and api.UpdatedBy only on the success path so the method does not consume partially scanned data.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@platform-api/src/internal/repository/api.go`:
- Around line 106-121: The query result handling in the repository method that
uses r.db.QueryRow(...).Scan should check the Scan error before reading any
scanned values. Move the err handling for sql.ErrNoRows and other failures
immediately after Scan, then assign api.Kind, api.CreatedBy, and api.UpdatedBy
only on the success path so the method does not consume partially scanned data.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b016b640-37bf-4592-9c8a-2ecad15a03c5
📒 Files selected for processing (25)
platform-api/src/internal/database/schema.postgres.sqlplatform-api/src/internal/database/schema.sqlplatform-api/src/internal/database/schema.sqlite.sqlplatform-api/src/internal/database/schema.sqlserver.sqlplatform-api/src/internal/handler/subscription_plan_handler.goplatform-api/src/internal/integration/cascade_test.goplatform-api/src/internal/integration/lifecycle_test.goplatform-api/src/internal/model/project.goplatform-api/src/internal/repository/api.goplatform-api/src/internal/repository/application.goplatform-api/src/internal/repository/artifact.goplatform-api/src/internal/repository/custom_policy.goplatform-api/src/internal/repository/gateway.goplatform-api/src/internal/repository/interfaces.goplatform-api/src/internal/repository/llm.goplatform-api/src/internal/repository/mcp.goplatform-api/src/internal/repository/project.goplatform-api/src/internal/repository/subscription_repository.goplatform-api/src/internal/repository/webbroker_api.goplatform-api/src/internal/repository/websub_api.goplatform-api/src/internal/service/api.goplatform-api/src/internal/service/llm_test.goplatform-api/src/internal/service/organization.goplatform-api/src/internal/service/project.goplatform-api/src/internal/service/subscription_plan_service.go
🚧 Files skipped from review as they are similar to previous changes (21)
- platform-api/src/internal/model/project.go
- platform-api/src/internal/service/project.go
- platform-api/src/internal/service/subscription_plan_service.go
- platform-api/src/internal/repository/subscription_repository.go
- platform-api/src/internal/handler/subscription_plan_handler.go
- platform-api/src/internal/database/schema.postgres.sql
- platform-api/src/internal/repository/artifact.go
- platform-api/src/internal/service/organization.go
- platform-api/src/internal/repository/custom_policy.go
- platform-api/src/internal/repository/websub_api.go
- platform-api/src/internal/repository/webbroker_api.go
- platform-api/src/internal/service/llm_test.go
- platform-api/src/internal/repository/mcp.go
- platform-api/src/internal/repository/gateway.go
- platform-api/src/internal/database/schema.sql
- platform-api/src/internal/repository/llm.go
- platform-api/src/internal/repository/application.go
- platform-api/src/internal/database/schema.sqlserver.sql
- platform-api/src/internal/integration/cascade_test.go
- platform-api/src/internal/database/schema.sqlite.sql
- platform-api/src/internal/service/api.go
…tions - Updated the unique constraint in the subscription_plans table to enforce uniqueness on (uuid, organization_uuid) for improved data integrity. - Introduced a new integration test file to validate the full lifecycle of API and gateway operations, including creation, update, and listing functionalities. - Enhanced the database initialization process to drop and recreate the SQL Server test database, ensuring a clean state for each test run. - Improved error handling and cleanup procedures in the integration tests for better reliability and clarity.
- Updated the APIKeyRepo methods to store and retrieve API key hashes as byte arrays, improving data handling and performance. - Adjusted the Create, Update, and List methods to accommodate the new byte array format for API key hashes. - Ensured compatibility with existing functionality by converting byte arrays back to strings where necessary.
…tabase operations - Eliminated the 'UpdatedAt' field from the ApplicationAssociationTarget model and its associated database queries. - Updated SQL insert statements in the ApplicationRepo to reflect the removal of 'UpdatedAt' from application artifacts and API keys. - Adjusted the application service to ensure consistency with the updated model structure.
…stency with model changes
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
3 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.
Issue: #2279
https://claude.ai/code/artifact/a73f74e1-c55a-4804-89e4-46cf8f3c708f