Extension to GlideRecord: addQuery2, Throws an exception if you pass a bad column name or bad operator

treycarroll
Giga Guru

Have you ever been burned by the fact that GlideRecord's addQuery function silently tolerates bad column names?   I have. This query was nearly "career-altering" for me, when, in a sprint review meeting with senior leadership after 8 months of development, something "unexpected" happened.

try {

      var gr = new GlideRecord('change_task');

      gr.addQuery('parent_change', current.sys_id);

      gr.addQuery('short_description', 'CONTAINS', 'Build change');

      gr.query();

      while (gr.next()) {

              gr.state = 3;

              gr.update();

      }

} catch (e) {

      gs.logError('Error closing build change task for ' + current.number + ':' + e, 'Normal Change Workflow');

}

Can you spot the error?   'parent_change' is not a valid column name on the change_task table; it should be 'change_request'.     But the code is wrapped in a try/catch, so it will throw if you make a mistake like this, right?   Wrong!     It just ignores line 3 due to the bad column name.   The result is that ALL change tasks with "Build change" in the short description will be closed, not just the one that this workflow is running against.

In this particular situation, as the senior leaders were reviewing what we had built over a WebEx, a developer was following along on a different change.   When they clicked their 'Close Build Change Task' UI Action form button on their change, it also unexpectedly closed the "Build change" task on the change that leadership was stepping through.

The plug got pulled on the release and we were in hot water.     We found the bug, but the damage was done.   We had carefully done our testing.   What happened?     Since the button technically worked (closed the task upon request), this code had made it through extensive testing by a very competent QA department.

This fiasco led me create a replacement for addQuery that will throw an exception if the query is malformed:

GlideRecord.prototype.addQuery2 = function () {

      //@param colListWithDots is a string containing column names separated by dots.

      //@param table is the name of the table

      function checkDotWalking(colListWithDots, table) {

              //@param table is a table name

              //@param column is expected to be the name of a column on the table param

              //@param referenceTypeColumn boolean that signals that this column is a reference-type col

              // Returns an object with properties, success(bool), error (string), nextTable(string)

              // nextTable property is only populated if the current column is a reference-type column

              function checkCol(table, column, referenceTypeColumn) {

                      try {

                              var grCheckTable = GlideRecord(table);

                              grCheckTable.setLimit(1);

                              grCheckTable.query();

                              //Check for bad column name

                              if (!grCheckTable.isValidField(column)) {

                                      return {

                                              success: false,

                                              error: column + " is not a valid field name for table: " + table

                                      };

                              }

                              //Check for non reference-type column used for dot walking

                              if (referenceTypeColumn) {

                                      if (grCheckTable[column].sys_meta.type_description != 'reference') {

                                              return {

                                                      success: false,

                                                      error: 'Attempted dot walking on a column that is not a reference-type column:' + column

                                              };

                                      } else { //return succesful setting the nextTable, signal for continued dot walking

                                              return {

                                                      success: true,

                                                      nextTable: grCheckTable[column].sys_meta.reference

                                              }

                                      }

                              }

                              // We have already passed every possible error condition.   (Not a reference-type field. Reached the end of the dot walking)

                              return {

                                      success: true

                              }

                      } catch (e3) {

                              throw 'Error in checkCol():' + e3;

                      }

              }//end checkCol

              //@param queryColString is the value of the first param from a call to addQuery (Column name)

              //Returns an array filled with the tokens between non-alpha-chars (here 'dots')

              //Example: if passed 'aaa.bbb.ccc.ddd', would return ['aaa','bbb','ccc','ddd']

              function loadTokens(queryColString) {

                      try {

                              var matches = queryColString.match(/(\w+)/ig);

                              var tokens = [];

                              if (matches) {

                                      for (var i = 0; i < matches.length; i++) {

                                              tokens.push(matches[i]);

                                      }

                              }

                              return tokens;

                      } catch (e4) {

                              throw 'Error in loadTokens():' + e4;

                      }

              }

              try {

                      var tokens = loadTokens(colListWithDots);

                      if (tokens.length > 1) {//We only want to do this if there is more than one token

                              for (var i = 0; i < tokens.length - 1; i++) {//Stop iterating one before the end.   The last value is not a reference field

                                      var columnName = tokens[i];

                                      //Check the value to ensure that it is a valid col on that table (for a reference-type column)

                                      var result = checkCol(table, columnName, true);

                                      if (!result.success) {

                                              return result;

                                      }

                                      table = result.nextTable;

                              }

                              var finalColumnName = tokens[tokens.length - 1];

                              //Check the last value to ensure that it is a valid col on that table (but not that it is a reference-type column)

                              result = checkCol(table, finalColumnName, false);

                              if (!result.success) {

                                      return result;

                              }

                              return {

                                      success: true

                              };

                      }

              } catch (e2) {

                      throw 'Error in body of checkDotWalking():' + e2;

              }

      }

      try {

              //Cast params to strings

              var a = arguments[0] + '';

              var b = arguments[1] + '';

              var c = arguments[2] + '';

              //The first arg (column name) uses dot walking

              if (/\w\.\w/.test(a)) {

                      var result = checkDotWalking(a, this.sys_meta.name);

                      if (!result.success) {

                              throw result.error;

                      }

              } else if (!this.isValidField(a)) { //Throw an exception if the simple field name is bad (non-dot walking)

                      throw a + " is not a valid field name for table: " + this.sys_meta.name;

              }

              //3 parameter version

              if (arguments[2]) {

                      var validOperators = ">,>=,=,!=,<,<=,STARTSWITH,ENDSWITH,IN,CONTAINS,DOES NOT CONTAIN";

                      //Throw an exception if the operator is bad                      

                      if (validOperators.indexOf(b) == -1) {

                              throw b + " is not a valid operation.   Valid operations:" + validOperators;

                      }

                      this.addQuery(a, b, c);

                      return;

              }

              //2 parameter version

              this.addQuery(a, b);

              return;

      } catch (e1) {

              throw "Error:" + e1 + ". Source:GlideRecord.prototype.addQuery2";

      }

};

You can now wrap your GlideRecord queries in a try catch and respond to bad field names.

find_real_file.png

                                    find_real_file.png

It also throws when you foul up the operator.

find_real_file.png

find_real_file.png

Rewriting the initial "horror story" query from the beginning of the post to use addQuery2:

try {

      var gr = new GlideRecord('change_task');

      gr.addQuery2('parent_change', current.sys_id);//'parent_change' should be 'change_request'

      gr.addQuery2('short_description', 'CONTAINS', 'Build change');

      gr.query();

      while (gr.next()) {

              gr.state = 3;

              gr.update();

      }

} catch (e) {

      gs.logError('Error closing build change task for '   + current.number + ':' + e, 'Normal Change Workflow');

}

Now the query will produce this error:

Normal Change Workflow: Error closing build change task for CHG0003345:Error:parent_change is not a valid field name for table: change_task. Source:GlideRecord.prototype.addQuery2: no thrown error

Much better!

Updated to reduce code complexity in checking validOperators based on feedback from sabell2012

Message was edited by: Updated to throw an error based on bad dot walking.

19 REPLIES 19

sabell2012
Mega Sage
Mega Sage

Awesome addition Trey!   I have wanted this for a very long time!   What GlideRecord does right now is eats the error and throws out the not-understood query line.   This overcomes that and tosses you back what was wrong and does not run the query!


sergiu_panaite
ServiceNow Employee
ServiceNow Employee

Hi Trey,



Good article. Just want to mention that we already have a way of protecting from bad GlideRecord calls.


See here:



Performance considerations when using GlideRecord



and here:



GlideRecord - ServiceNow Wiki   - 8.9.1 Controlling invalid queries



Regards,


Sergiu


Hi Sergiu,



Thanks for the links, i appreciate your input.   (Your 2nd link gave me a Jive 404 unfortunately.     I think that the new community features are not all functional at this time.)



I have used the "no rows" approach you mention in the past, but was not satisfied with it.   It was this dissatisfaction that drove development of addQuery2, actually.,   What addQuery2 does is fundamentally different than using the "errors return no rows" approach.     The problem is that   sometimes "no rows" is a valid result for a query, so we can't use it as a marker for an error.



E.g.   "Show me all of the approvers for this RITM where the state is approved."     If there are no rows my workflow takes one path, while finding rows sends me down another.



In the case above, using the "no rows" approach sends you down an incorrect path (no approvers) in the WF and never informs you that you have a problem in your query.   (These bugs can be difficult to track down!)     However, if you use addQuery2, wrapped in a try / catch block, you will be able to log the error and then rethrow it so that your WF activity will clearly show that an error happened.



Also, as sabell2012 pointed out, addQuery2 has the added benefit of stopping the execution of the (now erroneously "widened" and potentially expensive) bad query.



We're finding that getting a very tight grip on logging errors anywhere in code in our instance is essential for system stability and reducing time to resolve for bugs.   Hopefully addQuery2 will benefit others pursuing this same goal.   I would highly recommend it for any critical systems, such as a change module for SOX or Medical systems.



Regards,



Trey Carroll


Excellent article Trey,


Any harm in using it as a coding best practise for every GlideRecord query we does?


sabell2012 and sergiu.panaite, any advice on this?