launchpad-reviewers team mailing list archive

[Guruprasad <mp+415804@xxxxxxxxxxxxxxxxxx>]

Ioana, I did a review pass and left some comments, questions, and observations. Since this is my first DB review, some of the comments and questions are for me to know more and not necessarily require changes to the patch. 😀

Diff comments:

> diff --git a/database/schema/patch-2210-42-0.sql b/database/schema/patch-2210-42-0.sql
> new file mode 100644
> index 0000000..2ad467e
> --- /dev/null
> +++ b/database/schema/patch-2210-42-0.sql
> @@ -0,0 +1,64 @@
> +-- 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;
> +
> +CREATE TABLE vulnerability (

We need an additional field 'date_made_public' to store the `PublicDateAtUSN` field as mentioned in the document that I created for the LP-544 task.

> +	id serial PRIMARY KEY,
> +	distribution integer REFERENCES Distribution NOT NULL,
> +	cve integer REFERENCES CVE,

A vulnerability is always associated with a CVE in the context of the security tracker epic, right? If yes, wouldn't it make sense to disallow `NULL` value?

> +	status integer,
> +	description text,
> +	notes text,

Colin, Ioana, since notes can have comments from some user with their IRC nickname, do we need a separate field to store the user reference?

> +	mitigation text,
> +	importance integer NOT NULL,
> +	importance_explanation text,
> +	private boolean DEFAULT false NOT NULL
> +);
> +
> +COMMENT ON TABLE vulnerability IS 'Expresses the notion of whether a CVE affects a distribution.';
> +COMMENT ON COLUMN vulnerability.distribution IS 'Indicates control by the pillar''s owner.';
> +COMMENT ON COLUMN vulnerability.cve IS 'Intentionally nullable, since we need to track vulnerabilities not associated with CVEs.';
> +COMMENT ON COLUMN vulnerability.status IS 'Indicates current status of the vulnerability.';
> +COMMENT ON COLUMN vulnerability.cve IS 'Overrides the cve description.';
> +COMMENT ON COLUMN vulnerability.notes IS 'Free-form notes; may need some formatting machinery.';
> +COMMENT ON COLUMN vulnerability.mitigation IS 'Explain why we''re ignoring something.';
> +COMMENT ON COLUMN vulnerability.importance IS 'Indicates work priority, not severity.';
> +COMMENT ON COLUMN vulnerability.importance_explanation IS 'Used to explain why our importance differs from somebody else''s CVSS score.';
> +COMMENT ON COLUMN vulnerability.private IS 'Indicates privacy of the vulnerability.';
> +
> +CREATE INDEX vulnerability__distribution__cve__idx

Do we expect to have queries that try to retrieve vulnerabilities for a distribution and optionally, a CVE? Since the CVE sequence is unique can we not just query using the sequence directly without requiring the distribution? I am just thinking out loud about the types of queries that this will help with.

> +    ON vulnerability (distribution, cve);
> +	
> +CREATE TABLE vulnerabilityactivity (
> +	id serial PRIMARY KEY,
> +	vulnerability integer REFERENCES Vulnerability NOT NULL,
> +	changer integer REFERENCES Person NOT NULL,
> +	date_changed timestamp without time zone NOT NULL,
> +	what_changed integer NOT NULL,
> +	old_value text,
> +	new_value text
> +);
> +
> +COMMENT ON TABLE vulnerabilityactivity IS 'Tracks changes to vulnerability rows.';
> +COMMENT ON COLUMN vulnerabilityactivity.vulnerability IS 'Indicates the vulnerability that the changes refer to.';
> +COMMENT ON COLUMN vulnerabilityactivity.changer IS 'Indicates the person that made the changes.';
> +COMMENT ON COLUMN vulnerabilityactivity.date_changed IS 'Indicates the date when the vulnerability details last changed.';
> +COMMENT ON COLUMN vulnerabilityactivity.what_changed IS 'Indicates what field changed for the vulnerability by means of an enum.';
> +COMMENT ON COLUMN vulnerabilityactivity.old_value IS 'Indicates the value prior to the change.';
> +COMMENT ON COLUMN vulnerabilityactivity.new_value IS 'Indicates the current value.';
> +
> +CREATE INDEX vulnerabilityactivity__vulnerability__changer__idx

Don't we need a separate index for 'changer' since this index will be used only when the 'WHERE' clause of a query combines both the fields? Colin mentioned this on Mattermost in a discussion about the indexes to create.

> FKs to Person always need an index to support person merging

> +    ON vulnerabilityactivity (vulnerability, changer);
> +	
> +CREATE TABLE bugvulnerability (

Colin, Ioana, do we need this table relating a bug and a vulnerability when a vulnerability is already related to a Cve, which is connected to a bug? I know this is just implementing the DB design, but wanted to be sure.

> +	bug integer REFERENCES Bug NOT NULL,
> +	vulnerability integer REFERENCES Vulnerability NOT NULL
> +);
> +
> +COMMENT ON TABLE bugvulnerability IS 'Links a vulnerability to the bug.';
> +
> +CREATE INDEX bugvulnerability__bug__vulnerability__idx
> +    ON bugvulnerability (bug, vulnerability);
> +
> +INSERT INTO LaunchpadDatabaseRevision VALUES (2210, 42, 0);


-- 
https://code.launchpad.net/~ilasc/launchpad/+git/launchpad/+merge/415804
Your team Launchpad code reviewers is requested to review the proposed merge of ~ilasc/launchpad:add-vulnerability-model into launchpad:db-devel.