Skip to content

Feature: schema proposals #6836

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 21 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@
"@graphql-codegen/urql-introspection": "3.0.0",
"@graphql-eslint/eslint-plugin": "3.20.1",
"@graphql-inspector/cli": "4.0.3",
"@graphql-inspector/core": "^6.0.0",
"@manypkg/get-packages": "2.2.2",
"@next/eslint-plugin-next": "14.2.23",
"@parcel/watcher": "2.5.0",
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,179 @@
import { type MigrationExecutor } from '../pg-migrator';

/**
* This migration establishes the schema proposal tables.
*/
export default {
name: '2025.05.29T00-00-00.schema-proposals.ts',
run: ({ sql }) => [
{
name: 'create schema_proposal tables',
query: sql`
CREATE TYPE
schema_proposal_stage AS ENUM('DRAFT', 'OPEN', 'APPROVED', 'IMPLEMENTED', 'CLOSED')
;
/**
* Request patterns include:
* - Get by ID
* - List target's proposals by date
* - List target's proposals by date, filtered by author/user_id and/or stage (for now)
*/
CREATE TABLE IF NOT EXISTS "schema_proposals"
(
id UUID PRIMARY KEY DEFAULT uuid_generate_v4 ()
, created_at TIMESTAMPTZ NOT NULL DEFAULT NOW()
, updated_at TIMESTAMPTZ NOT NULL DEFAULT NOW()
, title VARCHAR(72) NOT NULL
, stage schema_proposal_stage NOT NULL
, target_id UUID NOT NULL REFERENCES targets (id) ON DELETE CASCADE
-- ID for the user that opened the proposal
, user_id UUID REFERENCES users (id) ON DELETE SET NULL
-- The original schema version that this proposal referenced. In case the version is deleted,
-- set this to null to avoid completely erasing the change... This should never happen.
, diff_schema_version_id UUID NOT NULL REFERENCES schema_versions (id) ON DELETE SET NULL
)
;
CREATE INDEX IF NOT EXISTS schema_proposals_list ON schema_proposals (
target_id
, created_at DESC
)
;
CREATE INDEX IF NOT EXISTS schema_proposals_list_by_user_id ON schema_proposals (
target_id
, user_id
, created_at DESC
)
;
CREATE INDEX IF NOT EXISTS schema_proposals_list_by_stage ON schema_proposals (
target_id
, stage
, created_at DESC
)
;
CREATE INDEX IF NOT EXISTS schema_proposals_list_by_user_id_stage ON schema_proposals (
target_id
, user_id
, stage
, created_at DESC
)
;
-- For performance during schema_version delete
CREATE INDEX IF NOT EXISTS schema_proposals_diff_schema_version_id on schema_proposals (
diff_schema_version_id
)
;
-- For performance during user delete
CREATE INDEX IF NOT EXISTS schema_proposals_diff_user_id on schema_proposals (
user_id
)
;
/**
* Request patterns include:
* - Get by ID
* - List proposal's latest versions for each service
* - List all proposal's versions ordered by date
*/
CREATE TABLE IF NOT EXISTS "schema_proposal_versions"
(
id UUID PRIMARY KEY DEFAULT uuid_generate_v4 ()
, user_id UUID REFERENCES users (id) ON DELETE SET NULL
, created_at TIMESTAMPTZ NOT NULL DEFAULT NOW()
, schema_proposal_id UUID NOT NULL REFERENCES schema_proposals (id) ON DELETE CASCADE
, service_name text
, schema_sdl text NOT NULL
)
;
CREATE INDEX IF NOT EXISTS schema_proposal_versions_list_latest_by_distinct_service ON schema_proposal_versions(
schema_proposal_id
, service_name
, created_at DESC
)
;
CREATE INDEX IF NOT EXISTS schema_proposal_versions_schema_proposal_id_created_at ON schema_proposal_versions(
schema_proposal_id
, created_at DESC
)
;
/**
* Request patterns include:
* - Get by ID
* - List proposal's latest versions for each service
* - List all proposal's versions ordered by date
*/
/**
SELECT * FROM schema_proposal_comments as c JOIN schema_proposal_reviews as r
ON r.schema_proposal_review_id = c.id
WHERE schema_proposal_id = $1
ORDER BY created_at
LIMIT 10
;
*/
CREATE TABLE IF NOT EXISTS "schema_proposal_reviews"
(
id UUID PRIMARY KEY DEFAULT uuid_generate_v4 ()
, created_at TIMESTAMPTZ NOT NULL DEFAULT NOW()
-- null if just a comment
, stage_transition schema_proposal_stage NOT NULL
Comment on lines +115 to +116
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For me this wasn't clear. Is it just me or worth expanding a bit?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Every action on a proposal is a "review". E.g. move back to draft, move to open, approve, close...
I agree this is slightly odd conceptually but I explained the reason in a separate comment below. (TLDR; to keep chronological order of changes clean)

, user_id UUID REFERENCES users (id) ON DELETE SET NULL
, schema_proposal_id UUID NOT NULL REFERENCES schema_proposals (id) ON DELETE CASCADE
-- store the originally proposed version to be able to reference back as outdated if unable to attribute
-- the review to another version.
, original_schema_proposal_version_id UUID NOT NULL REFERENCES schema_proposal_versions (id) ON DELETE SET NULL
-- store the original text of the line that is being reviewed. If the base schema version changes, then this is
-- used to determine which line this review falls on. If no line matches in the current version, then
-- show as outdated and attribute to the original line.
, line_text text
Comment on lines +122 to +125
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could another way be to store a relation to the base schema version?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, and of course I'm open to adjusting this, but then youd need to calculate this text at runtime by diffing that version and the "original_schema_proposal_version". Otherwise you wouldn't know what the review is referencing and when the diff changes, the line number would likely reference a totally different line.
String matching is unfortunately the only way I could come up with that made sense.

E.g.
on a diff

line 10: - type FooUser {
line 11: + type FooUser implements User {

If I comment on line 11, and in the next version several lines get added above this, then I want my comment to still reference + type FooUser implements User {

And if this line is modified in later version and no longer has a match, then it the review will revert to being at its original line (11), be flagged as stale, and show a preview of what the line used to be -- which is "line_text".
So I believe this column is essential if we want to be performant and accurate.

-- used in combination with the line_text to determine what line in the current version this review is attributed to
, original_line_num INT
-- the coordinate closest to the reviewed line. E.g. if a comment is reviewed, then
-- this is the coordinate that the comment applies to.
-- note that the line_text must still be stored in case the coordinate can no
-- longer be found in the latest proposal version. That way a preview of the reviewed
-- line can be provided.
, schema_coordinate text
)
;
CREATE INDEX IF NOT EXISTS schema_proposal_reviews_schema_proposal_id ON schema_proposal_reviews(
schema_proposal_id
, created_at ASC
)
;
-- For performance on user delete
CREATE INDEX IF NOT EXISTS schema_proposal_reviews_user_id ON schema_proposal_reviews(
user_id
)
;
-- For performance on schema_proposal_versions delete
CREATE INDEX IF NOT EXISTS schema_proposal_reviews_original_schema_proposal_version_id ON schema_proposal_reviews(
original_schema_proposal_version_id
)
;
/**
* Request patterns include:
* - Get by ID
* - List a proposal's comments in order of creation, grouped by review.
*/
CREATE TABLE IF NOT EXISTS "schema_proposal_comments"
(
id UUID PRIMARY KEY DEFAULT uuid_generate_v4 ()
, user_id UUID REFERENCES users (id) ON DELETE SET NULL
, body TEXT NOT NULL
, created_at TIMESTAMPTZ NOT NULL DEFAULT NOW()
, updated_at TIMESTAMPTZ NOT NULL DEFAULT NOW()
, schema_proposal_review_id UUID REFERENCES schema_proposal_reviews (id) ON DELETE CASCADE
)
Comment on lines +156 to +164
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we bake in more semantics here rather than just free form? For example maybe a category enum.

type: kudos | confused | disagree | idea

The thinking is that structured semantics here would allow some interesting UX downstream, such as us being able to present the user with triaged comments e.g. "blocking" | "optional" etc. to reduce review process overhead.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a great idea but it might belong on the review object and is better suited to a v2.

For more context -- One constraint of the current table design is that every comment must be tied to a review. This is because I opted to use sql and I wanted to avoid complicated merging + pagination logic in the UI to display multiple sets or chronological data at the same time.
If using nosql then this could all be in the same table and have different structures.

;
CREATE INDEX IF NOT EXISTS schema_proposal_comments_list ON schema_proposal_comments(
schema_proposal_review_id
, created_at ASC
)
;
-- For performance on user delete
CREATE INDEX IF NOT EXISTS schema_proposal_comments_user_id ON schema_proposal_comments(
user_id
)
;
`,
},
],
} satisfies MigrationExecutor;
1 change: 1 addition & 0 deletions packages/migrations/src/run-pg-migrations.ts
Original file line number Diff line number Diff line change
Expand Up @@ -167,5 +167,6 @@ export const runPGMigrations = async (args: { slonik: DatabasePool; runTo?: stri
await import('./actions/2025.05.15T00-00-00.contracts-foreign-key-constraint-fix'),
await import('./actions/2025.05.15T00-00-01.organization-member-pagination'),
await import('./actions/2025.05.28T00-00-00.schema-log-by-ids'),
await import('./actions/2025.05.29T00.00.00.schema-proposals'),
],
});
2 changes: 2 additions & 0 deletions packages/services/api/src/create.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ import {
SchemaPolicyServiceConfig,
} from './modules/policy/providers/tokens';
import { projectModule } from './modules/project';
import { proposalsModule } from './modules/proposals';
import { schemaModule } from './modules/schema';
import { ArtifactStorageWriter } from './modules/schema/providers/artifact-storage-writer';
import { provideSchemaModuleConfig, SchemaModuleConfig } from './modules/schema/providers/config';
Expand Down Expand Up @@ -88,6 +89,7 @@ const modules = [
collectionModule,
appDeploymentsModule,
auditLogsModule,
proposalsModule,
];

export function createRegistry({
Expand Down
11 changes: 11 additions & 0 deletions packages/services/api/src/modules/proposals/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import { createModule } from 'graphql-modules';
import { resolvers } from './resolvers.generated';
import typeDefs from './module.graphql';

export const proposalsModule = createModule({
id: 'proposals',
dirname: __dirname,
typeDefs,
resolvers,
providers: [],
});
Loading