Improve null handling for project_field and project_field_value (#17)

* update types and docs to reflect nullability

* update docs

* improve Joi validation

* put optional fields constraints in the database

* enhance docs

* filter should be allow(null) as well

* make project_field and project_field_value required for insert

* revert "make project_field and project_field_value required for insert"

* comment on Joi schema for project_field and project_field_value

* comment on Joi schema design

* remove useless export

Author: joao-paulo-parity

README.md

@@ -97,7 +97,11 @@ retrieved at any point by using the [listing endpoint](#service-api-list-rules).
 #### Unfiltered Rule <a name="service-api-unfiltered-rule"></a>
 
 If a Rule is specified with no filter, **any** issue associated with the
-[incoming events](#sync-events) will be registered to the board.
+[incoming events](#sync-events) will be registered to the board. As noted by
+[`IssueToProjectFieldRuleCreationInput`](./src/server/types.ts),
+`"project_field"` and `"project_field_value"` can be (both) nullable, in which
+case the issue would be created to the board but no custom field would be
+assigned to it.
 
 ```
 curl \

src/core.ts

@@ -172,7 +172,11 @@ export const syncIssue = async ({
 }: {
   issue: Issue
   graphql: typeof OctokitGraphQL
-  project: { number: number; targetField?: string; targetValue?: string }
+  project: {
+    number: number
+    targetField: string | null
+    targetValue: string | null
+  }
   organization: string
 }) => {
   const projectData = await fetchProjectData({

src/server/api.ts

@@ -280,20 +280,71 @@ export const setupApi = (
     },
   )
 
+  /*
+     For project_field and project_field_value, having the schema organized this
+     way yields better error messages than Joi.alternative()
+  */
+  const commonIssueToProjectFieldRuleCreationSchemaKeys = {
+    project_field: Joi.string()
+      .allow(null)
+      /*
+        We use ".." (the reference to the shared parent object of project_field
+        and project_field since they're both on the same level in the payload's
+        hierarchy) here because project_field and project_field_value value
+        depend on one another, but Joi doesn't support circular references
+      */
+      .when("..", {
+        switch: [
+          // If project_field_value is null, project_field should also be null
+          {
+            is: Joi.object()
+              .keys({ project_field_value: Joi.valid(null).required() })
+              .options({ allowUnknown: true }),
+            then: Joi.valid(null).required(),
+          },
+          {
+            // If project_field_value is a string, project_field should also be a string
+            is: Joi.object()
+              .keys({ project_field_value: Joi.string().required() })
+              .options({ allowUnknown: true }),
+            then: Joi.string().required(),
+          },
+        ],
+      }),
+    project_field_value: Joi.string()
+      .allow(null)
+      /*
+         We use ".." (the reference to the shared parent object of project_field
+         and project_field since they're both on the same level in the payload's
+         hierarchy) here because project_field and project_field_value value
+         depend on one another, but Joi doesn't support circular references
+      */
+      .when("..", {
+        switch: [
+          // If project_field is null, project_field_value should also be null
+          {
+            is: Joi.object()
+              .keys({ project_field: Joi.valid(null).required() })
+              .options({ allowUnknown: true }),
+            then: Joi.valid(null).required(),
+          },
+          {
+            // If project_field is a string, project_field_value should also be a string
+            is: Joi.object()
+              .keys({ project_field: Joi.string().required() })
+              .options({ allowUnknown: true }),
+            then: Joi.string().required(),
+          },
+        ],
+      }),
+    filter: Joi.string().allow(null),
+  }
+
   const issueToProjectFieldRuleCreationSchema = Joi.object<
-    Pick<
-      IssueToProjectFieldRule,
-      "project_number" | "project_field" | "project_field_value" | "filter"
-    >
+    Omit<IssueToProjectFieldRule, "id">
   >().keys({
+    ...commonIssueToProjectFieldRuleCreationSchemaKeys,
     project_number: Joi.number().required(),
-    project_field: Joi.string(),
-    project_field_value: Joi.string().when("project_field", {
-      is: Joi.exist(),
-      then: Joi.required(),
-      otherwise: Joi.forbidden(),
-    }),
-    filter: Joi.string(),
   })
 
   setupRoute(
@@ -315,23 +366,18 @@ export const setupApi = (
       }
 
       const { value: payload } = bodyValidation
-      const inputParams: DynamicQueryParam[] = []
-      for (const [column, value] of Object.entries(payload)) {
-        if (
-          value === null ||
-          (typeof value === "string" && value.length === 0)
-        ) {
-          continue
-        }
-        inputParams.push({ column, value })
-      }
-      if (inputParams.length === 0) {
+      const queryParams: DynamicQueryParam[] = Object.entries(payload).map(
+        ([column, value]) => {
+          return { column, value }
+        },
+      )
+      if (queryParams.length === 0) {
         return err(422, "Should provide at least one column")
       }
 
       const rowCount = await database.wrapForDynamicQuery(
         [
-          ...inputParams,
+          ...queryParams,
           { column: "github_owner", value: params.owner },
           { column: "github_name", value: params.name },
         ],
@@ -383,12 +429,10 @@ export const setupApi = (
   const issueToProjectFieldRuleUpdateSchema = Joi.object<
     ToOptional<Omit<IssueToProjectFieldRule, "id">>
   >().keys({
+    ...commonIssueToProjectFieldRuleCreationSchemaKeys,
     github_owner: Joi.string(),
     github_name: Joi.string(),
     project_number: Joi.number(),
-    project_field: Joi.string(),
-    project_field_value: Joi.string(),
-    filter: Joi.string(),
   })
   setupRoute(
     "patch",
@@ -413,22 +457,17 @@ export const setupApi = (
       }
 
       const { value: payload } = bodyValidation
-      const inputParams: DynamicQueryParam[] = []
-      for (const [column, value] of Object.entries(payload)) {
-        if (
-          value === null ||
-          (typeof value === "string" && value.length === 0)
-        ) {
-          continue
-        }
-        inputParams.push({ column, value })
-      }
-      if (inputParams.length === 0) {
+      const queryParams: DynamicQueryParam[] = Object.entries(payload).map(
+        ([column, value]) => {
+          return { column, value }
+        },
+      )
+      if (queryParams.length === 0) {
         return err(422, "Should provide at least one column")
       }
 
       const { rows, rowCount } = await database.wrapForDynamicQuery(
-        inputParams,
+        queryParams,
         async (client, { updateStatementParamsJoined, values }) => {
           const { rows, rowCount } = await client.query(
             `

src/server/event.ts

@@ -79,8 +79,8 @@ export const setupBotEvents = (ctx: Context) => {
             graphql: octokit.graphql,
             project: {
               number: rule.project_number,
-              targetField: rule.project_field,
-              targetValue: rule.project_field_value,
+              targetField: rule.project_field ?? null,
+              targetValue: rule.project_field_value ?? null,
             },
             organization: rule.github_owner,
           })

src/server/migrations/1645593356645_issue_to_project_field_rule.ts

@@ -1,14 +1,17 @@
 import type { MigrationBuilder } from "node-pg-migrate"
 
-const issueToProjectFieldRuleTable = "issue_to_project_field_rule"
+export const issueToProjectFieldRuleTable = "issue_to_project_field_rule"
+export const projectField = "project_field"
+export const projectFieldValue = "project_field_value"
+
 export const up = async (pgm: MigrationBuilder) => {
   pgm.createTable(issueToProjectFieldRuleTable, {
     id: { type: "SERIAL", primaryKey: true },
     github_owner: { type: "TEXT", notNull: true },
     github_name: { type: "TEXT", notNull: true },
     project_number: { type: "INT", notNull: true },
-    project_field: { type: "TEXT", notNull: true },
-    project_field_value: { type: "TEXT", notNull: true },
+    [projectField]: { type: "TEXT", notNull: true },
+    [projectFieldValue]: { type: "TEXT", notNull: true },
     filter: { type: "TEXT" },
   })
 }

src/server/migrations/1651166162160_field_becomes_optional.ts

@@ -1,8 +1,10 @@
 import type { MigrationBuilder } from "node-pg-migrate"
 
-const issueToProjectFieldRuleTable = "issue_to_project_field_rule"
-const projectField = "project_field"
-const projectFieldValue = "project_field_value"
+import {
+  issueToProjectFieldRuleTable,
+  projectField,
+  projectFieldValue,
+} from "./1645593356645_issue_to_project_field_rule"
 
 export const up = async (pgm: MigrationBuilder) => {
   pgm.alterColumn(issueToProjectFieldRuleTable, projectField, {
@@ -15,9 +17,9 @@ export const up = async (pgm: MigrationBuilder) => {
 
 export const down = async (pgm: MigrationBuilder) => {
   pgm.sql(`
-    DELETE FROM ${issueToProjectFieldRuleTable} 
-    WHERE 
-      ${projectField} IS NULL 
+    DELETE FROM ${issueToProjectFieldRuleTable}
+    WHERE
+      ${projectField} IS NULL
       OR ${projectFieldValue} IS NULL
   `)
   pgm.alterColumn(issueToProjectFieldRuleTable, projectField, {

src/server/migrations/1651425765498_issue-to-project-field-constraint.ts

@@ -0,0 +1,28 @@
+import type { MigrationBuilder } from "node-pg-migrate"
+
+import {
+  issueToProjectFieldRuleTable,
+  projectField,
+  projectFieldValue,
+} from "./1645593356645_issue_to_project_field_rule"
+
+const projectFieldBothNullableOrRequiredConstraint =
+  "project_field_both_nullable_or_required"
+
+export const up = async (pgm: MigrationBuilder) => {
+  pgm.addConstraint(
+    issueToProjectFieldRuleTable,
+    projectFieldBothNullableOrRequiredConstraint,
+    `CHECK (
+      (${projectField} is null AND ${projectFieldValue} is null) OR
+      (${projectField} is not null AND ${projectFieldValue} is not null)
+    )`,
+  )
+}
+
+export const down = async (pgm: MigrationBuilder) => {
+  pgm.dropConstraint(
+    issueToProjectFieldRuleTable,
+    projectFieldBothNullableOrRequiredConstraint,
+  )
+}

src/server/types.ts

@@ -27,8 +27,8 @@ export type Context = {
 
 type IssueToProjectFieldRuleCreationInput = {
   project_number: number
-  project_field: string
-  project_field_value: string
+  project_field: string | null
+  project_field_value: string | null
   filter?: string | null
 }
 type IssueToProjectFieldRuleUpdateInput =