Jk/cumulus 3701 fix null rules (#3641)

* Update core deps to v20.12.2

* Update unit test to rmove .sort

.sort is intermittently failing to order as expected, causing this
test to fail in local test at an alarming failure rate.    Updating
test to be more explicit/remove sort, however this needs to be
investigated prior to closing thie branch/PR/ticket

* Update kinesis test to node v20

JSON.parse now throws a different error for the merged test case.

* Update knex test to handle new subdependency throwing an Aggregate
error

* Revert "Update unit test to rmove .sort"

This reverts commit adbad84.

* Update got

* Revert "Update got"

This reverts commit a19f9d1.

* Upgrade ava/nock, fix ingest provider tests

* Reapply "Update got"

This reverts commit e652516.

* Update ingest module for node 20

This update has a couple of controversial changes in it:

Updating got to v14 means we're using a pure ESM module given sindre's
stance on not supporting common exports.  That's being done due to
incompatible changes in node streams requires at least got v12

Additionally there's a probable outstanding issue in got
sindresorhus/got#2341 related to node v20/fs
readstreams/nock and/or msw incompatibility (as well as a possible
open source contrib)

* Remove httpClient test mock/fix

Updating the existing apache server to return 200 on an existing
endpoint is far better than the prior commit approach in that it's a
valid/useful unit test as a result, with the technical/less tidy
downside of requiring the unit test stack to be working.

* Update local test stack configuration to restrict connection to localhost

* Update send-pan test/dependencies

* Update lambdas/async operation to deploy with node 20

* Update common to export a helper to import ESM got module

* Add docstring to importGot

* Remove unneeded imports

* Minor PR review fixes

* Update CHANGELOG

* Update CHANGELOG

* Reconfigure CI to not use unsafe perms modification

* Fix broken package.json

* Fix extran. dep issue

* Make error message test less specific

* Update test to not rely on aggregate internal errors

* Update packages/ingest/test/test-HttpProviderClient.js

Co-authored-by: etcart <37375117+etcart@users.noreply.github.com>

* Move importGot -> importEsm, introduce static import definition

* Fix inadvertant test commit

* Update rules helpers to remove all null key values from rule JSON

* Update CHANGELOG

* Update helper test titles for consistency

* Minor refactor/rename

* Fix typing errors in original code, update PR accordingly

* Fix inadvertant move of rule validation

* Update packages/api/endpoints/rules.js

Co-authored-by: etcart <37375117+etcart@users.noreply.github.com>

* Remove custom null removal method in favor of omitDeepBy

* Remove duplicative call to omitDeepBy

* Update packages/api/lib/rulesHelpers.js

Co-authored-by: etcart <37375117+etcart@users.noreply.github.com>

---------

Co-authored-by: etcart <37375117+etcart@users.noreply.github.com>

CHANGELOG.md

@@ -189,6 +189,8 @@ operations (e.g. `PREFIX-AsyncOperationEcsLogs`).
 
 ### Fixed
 
+- **CUMULUS-3701**
+  - Updated `@cumulus/api` to no longer improperly pass PATCH/PUT null values to Eventbridge rules
 - **CUMULUS-3618**
   - Fixed `@cumulus/es-client` credentialing issue in instance where
     lambda/Fargate task runtime would exceed the timeout for the es-client. Added retry/credential
@@ -204,7 +206,7 @@ operations (e.g. `PREFIX-AsyncOperationEcsLogs`).
     API/etc wholesale
   - Addresses [CVE-2020-36604](https://github.com/advisories/GHSA-c429-5p7v-vgjp)
 - **CUMULUS-3673**
-  - Fixes Granules API so that paths containing a granule and/or collection ID properly URI encode the ID.  
+  - Fixes Granules API so that paths containing a granule and/or collection ID properly URI encode the ID.
 - **Audit Issues**
   - Addressed [CVE-2023-45133](https://github.com/advisories/GHSA-67hx-6x53-jw92) by
     updating babel packages and .babelrc

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

@@ -1,9 +1,12 @@
+//@ts-check
+
 'use strict';
 
 const cloneDeep = require('lodash/cloneDeep');
 const get = require('lodash/get');
 const isNil = require('lodash/isNil');
 const set = require('lodash/set');
+
 const {
   AddPermissionCommand,
   DeleteEventSourceMappingCommand,
@@ -590,8 +593,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. ');
@@ -609,12 +613,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;
 }
 
 /**
@@ -635,15 +640,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;
@@ -691,18 +695,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/;
@@ -711,7 +716,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,6 +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');
 
@@ -1758,6 +1760,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();
@@ -1825,6 +1867,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,