launchpad-reviewers team mailing list archive

[Colin Watson <mp+426864@xxxxxxxxxxxxxxxxxx>]

Review: Approve db

This will also need a follow-up hot patch (targeted to master, and applied live by IS) to create a unique partial index on `AccessArtifact(vulnerability)` and to validate the replaced `has_artifact` constraint.  Compare `database/schema/archive/patch-2210-26-4.sql`.

Diff comments:

> diff --git a/database/schema/patch-2210-50-0.sql b/database/schema/patch-2210-50-0.sql
> new file mode 100644
> index 0000000..0c23462
> --- /dev/null
> +++ b/database/schema/patch-2210-50-0.sql
> @@ -0,0 +1,115 @@
> +-- Copyright 2022 Canonical Ltd.  This software is licensed under the
> +-- GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +SET client_min_messages=ERROR;
> +
> +ALTER TABLE Vulnerability
> +    ADD COLUMN access_policy integer,
> +    ADD COLUMN access_grants integer[];
> +
> +CREATE TABLE VulnerabilitySubscription (
> +    id serial PRIMARY KEY,
> +    person integer REFERENCES Person NOT NULL,
> +    vulnerability integer REFERENCES Vulnerability NOT NULL,
> +    date_created timestamp without time zone DEFAULT (CURRENT_TIMESTAMP AT TIME ZONE 'UTC') NOT NULL,
> +    creator integer REFERENCES Person NOT NULL

Please rename this to `subscribed_by`, to match most of the other `*Subscription` tables.

> +);
> +
> +COMMENT ON TABLE VulnerabilitySubscription IS '';

Can you come up with a non-empty table comment here?

> +COMMENT ON COLUMN VulnerabilitySubscription.person IS 'The person subscribing to the vulnerability.';
> +COMMENT ON COLUMN VulnerabilitySubscription.vulnerability IS 'The vulnerability being subscribed to.';
> +COMMENT ON COLUMN VulnerabilitySubscription.date_created IS 'The date when the subscription was created.';
> +COMMENT ON COLUMN VulnerabilitySubscription.creator IS 'The person who created the subscription.';
> +
> +CREATE INDEX vulnerabilitysubscription__person__idx
> +    ON VulnerabilitySubscription (person);

There should also be `CREATE UNIQUE INDEX vulnerabilitysubscription__person__vulnerability__key ON VulnerabilitySubscription (person, vulnerability)`, and since the index on just `(person)` would be a leftmost prefix of the columns in that unique index, you can drop this one.

I'm not sure why `BugSubscription` doesn't have a unique index on `(person, bug)`.  That might be an oversight.

> +
> +CREATE INDEX vulnerabilitysubscription__vulnerability__idx
> +    ON VulnerabilitySubscription (vulnerability);
> +
> +CREATE INDEX vulnerabilitysubscription__creator__idx
> +    ON VulnerabilitySubscription (creator);
> +
> +ALTER TABLE AccessArtifact
> +    ADD COLUMN vulnerability integer REFERENCES Vulnerability;
> +
> +
> +ALTER TABLE AccessArtifact DROP CONSTRAINT has_artifact;
> +ALTER TABLE AccessArtifact
> +    ADD CONSTRAINT has_artifact CHECK (
> +    (null_count(ARRAY[bug, branch, gitrepository, snap, specification, ocirecipe, vulnerability]) = 6)) NOT VALID;
> +
> +
> +CREATE OR REPLACE FUNCTION vulnerability_denorm_access(vulnerability_id integer)
> +    RETURNS void LANGUAGE plpgsql AS
> +$$
> +DECLARE
> +    info_type integer;
> +BEGIN
> +    SELECT
> +        -- information type: 1 = public
> +        COALESCE(Vulnerability.information_type, 1)
> +    INTO info_type
> +    FROM Vulnerability where id = vulnerability_id;
> +
> +    UPDATE Vulnerability
> +        SET access_policy = policies[1], access_grants = grants
> +        FROM
> +            build_access_cache(
> +                (SELECT id FROM Vulnerability WHERE vulnerability = vulnerability_id),
> +                info_type)
> +            AS (policies integer[], grants integer[])
> +        WHERE id = vulnerability_id;
> +END;
> +$$;
> +
> +CREATE OR REPLACE FUNCTION accessartifact_denorm_to_artifacts(artifact_id integer)
> +    RETURNS void
> +    LANGUAGE plpgsql
> +    AS $$
> +DECLARE
> +    artifact_row accessartifact%ROWTYPE;
> +BEGIN
> +    SELECT * INTO artifact_row FROM accessartifact WHERE id = artifact_id;
> +    IF artifact_row.bug IS NOT NULL THEN
> +        PERFORM bug_flatten_access(artifact_row.bug);
> +    END IF;
> +    IF artifact_row.branch IS NOT NULL THEN
> +        PERFORM branch_denorm_access(artifact_row.branch);
> +    END IF;
> +    IF artifact_row.gitrepository IS NOT NULL THEN
> +        PERFORM gitrepository_denorm_access(artifact_row.gitrepository);
> +    END IF;
> +    IF artifact_row.snap IS NOT NULL THEN
> +        PERFORM snap_denorm_access(artifact_row.snap);
> +    END IF;
> +    IF artifact_row.specification IS NOT NULL THEN
> +        PERFORM specification_denorm_access(artifact_row.specification);
> +    END IF;
> +    IF artifact_row.ocirecipe IS NOT NULL THEN
> +        PERFORM ocirecipe_denorm_access(artifact_row.ocirecipe);
> +    END IF;
> +    IF artifact_row.vulnerability IS NOT NULL THEN
> +        PERFORM vulnerability_denorm_access(artifact_row.vulnerability);
> +    END IF;
> +    RETURN;
> +END;
> +$$;
> +
> +COMMENT ON FUNCTION accessartifact_denorm_to_artifacts(artifact_id integer) IS
> +    'Denormalize the policy access and artifact grants to bugs, branches, git repositories, snaps, specifications, ocirecipe, and vulnerabilities.';

Maybe fix the oddly singular "ocirecipe" to "OCI recipes" while you're here.

> +
> +-- A trigger to handle vulnerability.information_type changes.
> +CREATE OR REPLACE FUNCTION vulnerability_maintain_access_cache_trig() RETURNS trigger
> +    LANGUAGE plpgsql as $$
> +BEGIN
> +    PERFORM vulnerability_denorm_access(NEW.id);
> +    RETURN NULL;
> +END;
> +$$;
> +
> +CREATE TRIGGER vulnerability_maintain_access_cache
> +    AFTER INSERT OR UPDATE OF information_type ON Vulnerability
> +    FOR EACH ROW EXECUTE PROCEDURE vulnerability_maintain_access_cache_trig();
> +
> +INSERT INTO LaunchpadDatabaseRevision VALUES (2210, 50, 0);

Following the recent rebaseline, please rename this patch to `patch-2211-02-0.sql`, and change this to `INSERT INTO LaunchpadDatabaseRevision VALUES (2211, 02, 0);`.

> diff --git a/database/schema/security.cfg b/database/schema/security.cfg
> index 56eb404..e23e78a 100644
> --- a/database/schema/security.cfg
> +++ b/database/schema/security.cfg
> @@ -2466,6 +2466,7 @@ public.vote                             = SELECT, UPDATE
>  public.votecast                         = SELECT, UPDATE
>  public.vulnerability                    = SELECT, UPDATE
>  public.vulnerabilityactivity            = SELECT, UPDATE
> +public.vulnerabilitysubscription        = SELECT, UPDATE, DELETE
>  public.webhook                          = SELECT, UPDATE
>  public.wikiname                         = SELECT, UPDATE
>  public.xref                             = SELECT, UPDATE

I think you'll also need to add `EXECUTE` permission for the new `vulnerability_denorm_access` function to the `[public]` section near the top, add `SELECT, INSERT, UPDATE, DELETE` permission for `vulnerabilitysubscription` to the `[launchpad_main]` section, and add `SELECT, UPDATE, DELETE` permission for `vulnerabilitysubscription` to the `[sharing-jobs]` section.



-- 
https://code.launchpad.net/~lgp171188/launchpad/+git/launchpad/+merge/426864
Your team Launchpad code reviewers is requested to review the proposed merge of ~lgp171188/launchpad:vulnerability-subscription-sql into launchpad:db-devel.