Jk/cumulus 3701 fix null rules (#3641)

CHANGELOG.md

@@ -6,6 +6,11 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/).
 
 ## Unreleased
 
+### Fixed
+
+- **CUMULUS-3701**
+  - Updated `@cumulus/api` to no longer improperly pass PATCH/PUT null values to Eventbridge rules
+
 ## [v18.2.0] 2023-02-02
 
 ### Migration Notes

packages/api/endpoints/rules.js

@@ -1,3 +1,5 @@
+//@ts-check
+
 'use strict';
 
 const router = require('express-promise-router')();
@@ -33,6 +35,10 @@ const schemas = require('../lib/schemas.js');
 
 const log = new Logger({ sender: '@cumulus/api/rules' });
 
+/**
+ * @typedef {import('@cumulus/types/api/rules').RuleRecord} RuleRecord
+ */
+
 /**
  * List all rules.
  *
@@ -133,7 +139,7 @@ async function post(req, res) {
  *
  * @param {object} params                   - params object
  * @param {object} params.res               - express response object
- * @param {object} params.oldApiRule        - API 'rule' to update
+ * @param {RuleRecord} params.oldApiRule    - API 'rule' to update
  * @param {object} params.apiRule           - updated API rule
  * @param {object} params.rulePgModel       - @cumulus/db compatible rule module instance
  * @param {object} params.knex              - Knex object
@@ -158,7 +164,7 @@ async function patchRule(params) {
     return invokeRerun(oldApiRule).then(() => res.send(oldApiRule));
   }
 
-  const apiRuleWithTrigger = await updateRuleTrigger(oldApiRule, apiRule, knex);
+  const apiRuleWithTrigger = await updateRuleTrigger(oldApiRule, apiRule);
   const apiPgRule = await translateApiRuleToPostgresRuleRaw(apiRuleWithTrigger, knex);
   log.debug(`rules.patchRule apiRuleWithTrigger: ${JSON.stringify(apiRuleWithTrigger)}`);
 

packages/api/lib/rulesHelpers.js

@@ -571,8 +571,9 @@ async function addRule(item, payload) {
 /**
  * Checks if record is valid
  *
- * @param {RuleRecord} rule - Rule to check validation
- * @returns {void}          - Returns if record is valid, throws error otherwise
+ * @param {any} rule - Object to validate as a Rule Record validation
+ * @param {any} rule     - Object to validate as a Rule Record validation
+ * @returns {RuleRecord} - Returns if record is valid, throws error otherwise
  */
 function validateRecord(rule) {
   const error = new Error('The record has validation errors. ');
@@ -590,12 +591,13 @@ function validateRecord(rule) {
     throw error;
   }
 
-  recordIsValid(omitDeepBy(rule, isNil), ruleSchema, false);
+  recordIsValid(rule, ruleSchema, false);
 
   if (rule.rule.type !== 'onetime' && !rule.rule.value) {
     error.message += `Rule value is undefined for ${rule.rule.type} rule`;
     throw error;
   }
+  return rule;
 }
 
 /**
@@ -616,15 +618,14 @@ async function invokeRerun(rule) {
 }
 
 /**
- * Updates rule trigger for rule
+ * Updates rule trigger for rule object
  *
  * @param {RuleRecord} original - Rule to update trigger for
- * @param {RuleRecord} updated  - Updated rule for rule trigger
+ * @param {Object} updated  - Updated rule for rule trigger
  * @returns {Promise<RuleRecord>}        - Returns new rule object
  */
 async function updateRuleTrigger(original, updated) {
-  let resultRule = cloneDeep(updated);
-  validateRecord(resultRule);
+  let resultRule = validateRecord(omitDeepBy(updated, isNil));
 
   const stateChanged = updated.state && updated.state !== original.state;
   const valueUpdated = updated.rule && updated.rule.value !== original.rule.value;
@@ -672,18 +673,19 @@ async function updateRuleTrigger(original, updated) {
 /**
  * Creates rule trigger for rule
  *
- * @param {RuleRecord} ruleItem - Rule to create trigger for
+ * @param {Object} ruleItem - Rule to create trigger for
  * @param {Object} testParams - Function to determine to use actual invoke or testInvoke
  * @returns {Promise<RuleRecord>} - Returns new rule object
  */
 async function createRuleTrigger(ruleItem, testParams = {}) {
-  let newRuleItem = cloneDeep(ruleItem);
+  const candidateRuleItem = omitDeepBy(ruleItem, isNil);
+
   // the default value for enabled is true
-  if (newRuleItem.state === undefined) {
-    newRuleItem.state = 'ENABLED';
+  if (candidateRuleItem.state === undefined) {
+    candidateRuleItem.state = 'ENABLED';
   }
 
-  const enabled = newRuleItem.state === 'ENABLED';
+  const enabled = candidateRuleItem.state === 'ENABLED';
   const invokeMethod = testParams.invokeMethod || invoke;
   // make sure the name only has word characters
   const re = /\W/;
@@ -692,7 +694,7 @@ async function createRuleTrigger(ruleItem, testParams = {}) {
   }
 
   // Validate rule before kicking off workflows or adding event source mappings
-  validateRecord(newRuleItem);
+  let newRuleItem = validateRecord(candidateRuleItem);
 
   const payload = await buildPayload(newRuleItem);
   switch (newRuleItem.rule.type) {

packages/api/tests/lib/rules/test-rulesHelpers.js

@@ -3,7 +3,8 @@
 const test = require('ava');
 const sinon = require('sinon');
 const omit = require('lodash/omit');
-
+const isObject = require('lodash/isObject');
+const cloneDeep = require('lodash/cloneDeep');
 const proxyquire = require('proxyquire');
 const fs = require('fs-extra');
 
@@ -1763,6 +1764,46 @@ test.serial('Creating a rule trigger for a scheduled rule succeeds', async (t) =
   });
 });
 
+test.serial('Creating a rule trigger for a scheduled rule with null values in the rule results in putRule being called with an object missing null values', async (t) => {
+  const rule = fakeRuleFactoryV2({
+    workflow,
+    rule: {
+      type: 'scheduled',
+      value: 'rate(1 min)',
+    },
+    state: 'ENABLED',
+    meta: {
+      visibilityTimeout: 100,
+      retries: 4,
+      someOtherValue: null,
+    },
+    queueUrl: null,
+  });
+
+  const cloudwatchStub = sinon.stub(awsServices, 'cloudwatchevents')
+    .returns({
+      putRule: () => ({
+        promise: () => Promise.resolve(),
+      }),
+      putTargets: (params) => {
+        const valueFunc = (obj) =>
+          ((obj && isObject(obj))
+            ? Object.values(obj).map(valueFunc).flat()
+            : [obj]);
+        const paramValues = params.Targets.map((target) => JSON.parse(target.Input)).map((x) => valueFunc(x)).flat();
+        t.false(paramValues.includes(null));
+        return {
+          promise: () => Promise.resolve(),
+        };
+      },
+    });
+  await createRuleTrigger(rule);
+  t.true(cloudwatchStub.called);
+  t.teardown(() => {
+    cloudwatchStub.restore();
+  });
+});
+
 test('buildPayload builds a lambda payload from the rule', async (t) => {
   const collectionPgModel = new CollectionPgModel();
   const providerPgModel = new ProviderPgModel();
@@ -1830,6 +1871,47 @@ test('buildPayload throws error if workflow file does not exist', async (t) => {
   );
 });
 
+test.serial('Updating a rule trigger for a scheduled rule with null values in the rule results in putRule being called with an object missing null values', async (t) => {
+  const rule = fakeRuleFactoryV2({
+    workflow,
+    rule: {
+      type: 'scheduled',
+      value: 'rate(1 min)',
+    },
+    state: 'ENABLED',
+    meta: {
+      visibilityTimeout: 100,
+      retries: 4,
+    },
+  });
+
+  const updatedRule = cloneDeep(rule);
+  updatedRule.queueUrl = null;
+  updatedRule.meta.someOtherValue = null;
+  const cloudwatchStub = sinon.stub(awsServices, 'cloudwatchevents')
+    .returns({
+      putRule: () => ({
+        promise: () => Promise.resolve(),
+      }),
+      putTargets: (params) => {
+        const valueFunc = (obj) =>
+          ((obj && isObject(obj))
+            ? Object.values(obj).map(valueFunc).flat()
+            : [obj]);
+        const paramValues = params.Targets.map((target) => JSON.parse(target.Input)).map((x) => valueFunc(x)).flat();
+        t.false(paramValues.includes(null));
+        return {
+          promise: () => Promise.resolve(),
+        };
+      },
+    });
+  await updateRuleTrigger(rule, updatedRule);
+  t.true(cloudwatchStub.called);
+  t.teardown(() => {
+    cloudwatchStub.restore();
+  });
+});
+
 test.serial('Updating a rule trigger with an "onetime" rule type returns updated rule', async (t) => {
   const fakeRule = fakeRuleFactoryV2({
     workflow,