Support non-numeric values for Usage chart filter parameters #716

ryanrath · 2018-11-01T14:05:18Z

Description

Added the ability for Usage chart requests to provide non-numeric values for
'_filter' parameters. The new feature will attempt to translate these
non-numeric values into appropriate id values that can be used during chart
generation. Specifically this new feature is to support integration w/
Coldfront.

Motivation and Context

Tests performed

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project as found in the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

jpwhite4

Also you need to create an integration test to confirm that the change works properly.

jpwhite4 · 2018-11-21T14:26:47Z

docs/software-requirements.md

@@ -76,7 +76,7 @@ added with this command for CentOS 7:
    # yum install httpd php php-cli php-mysql php-gd php-mcrypt \
                  gmp-devel php-gmp php-pdo php-xml \
                  php-pear-MDB2 php-pear-MDB2-Driver-mysql \
-                  java-1.7.0-openjdk java-1.7.0-openjdk-devel \
+                  java-1.8.0-openjdk java-1.8.0-openjdk-devel \


I don't think this documentation change belongs in this pull request.

This has been reverted.

jpwhite4 · 2018-11-21T14:43:46Z

classes/DataWarehouse/Access/Usage.php

+         * @return int[]|array all current queries return an int array. If no rows are found then
+         * an empty array is returned.
+         */
+        $lookupValue = function ($query, array $values) {


Note that the PHP PDO already has a function that will return the values of one column in an array.

$lookupvalue = function ($query, array $values) { $db = DB::factory('database'); $stmt = $db->prepare($query); $stmt->execute($values); return implode(',', $stmt->fetchAll(PDO::FETCH_COLUMN, 0) ); };

Very cool, thanks! This will be included in the next commit.

jpwhite4 · 2018-11-21T14:46:19Z

classes/DataWarehouse/Access/Usage.php

+    {
+
+        /**
+         * Anonymous function to prevent code duplication. $query is expected to minimally fulfill


I note that this anonymous function does not prevent code duplication since it only ever needs to be called once per call to translateFilterValues. In fact it results in more lines of code than simply having the code inline at the bottom of the switch statement and having the switch statement set the value of the $query.

Yeah... best intentions and all... Thanks for the catch. The code will be updated and actually simplified in the next set of commits.

jpwhite4 · 2018-11-21T14:56:40Z

classes/DataWarehouse/Access/Usage.php

+        };
+
+        if (!is_numeric($usageFilterValue)) {
+            $usageFilterValues = explode(',', $usageFilterValue);


The $usageFilterValue variable already went through an explode() on line 976. Therefore it cannot have any more commas to be exploded again and the bulk of this code is unnecessary. Please can you double check this.

Double checked, you are correct, this value doesn't need to be exploded again. Here is the function body w/ updates so far ( Just finishing up writing integration tests before committing ) :

// If the value is already a number then just return it. We do this because this function is // about tranlsating a non-numeric value to a numeric one ( i.e. an id ) if (is_numeric($usageFilterValue)) { return $usageFilterValue; } $query = null; switch ($usageFilterType) { case 'pi': // NOTE: At the time of writing there is no instance where multiple records have // the same username but different person_id values. $query = "SELECT DISTINCT sa.person_id AS value FROM modw.systemaccount sa WHERE sa.username = :value;"; break; case 'resource': $query = "SELECT id AS value FROM modw.resourcefact WHERE code = :value"; break; case 'project': $query = "SELECT account_id AS value FROM modw_cloud.account WHERE display = :value"; break; } if ($query !== null) { $db = DB::factory('database'); $stmt = $db->prepare($query); $stmt->execute(array(':value' => $usageFilterValue)); return implode(',', $stmt->fetchAll(PDO::FETCH_COLUMN, 0)); } return $usageFilterValue;

jpwhite4 · 2018-11-28T19:53:28Z

classes/DataWarehouse/Access/Usage.php

+            $stmt = $db->prepare($query);
+            $stmt->execute(array(':value' => $usageFilterValue));
+
+            return implode(',', $stmt->fetchAll(PDO::FETCH_COLUMN, 0));


I don't think you want to implode all results here. The rest of the code will only work if there is a single return value.

As per discussion earlier with @jpwhite4 and @ryanrath an upcoming commit will return an array of zero or more elements that can be iterated over to properly add the filters.

Update is in the latest version of the PR

jpwhite4 · 2018-11-28T21:36:03Z

What is the expected behavior of the code if a string value is supplied that does not match in the database. I.e if I ask for filtering by a PI that does not exist.

ryanrath · 2018-12-03T17:41:50Z

@smgallo Thoughts? What currently occurs is that that particular filter / value combo is ignored. So, resource_filter=frearson,robertson&pi_filter=superunknownpi is the same as resource_filter=frearson,robertson

smgallo · 2018-12-04T16:24:05Z

classes/DataWarehouse/Access/Usage.php

@@ -1156,14 +1155,38 @@ private function translateFilterValue($usageFilterType, $usageFilterValue)
        }

        if ($query !== null) {
+            $filterValueIsString = preg_match('/[\'"]+/', $usageFilterValue) === 1;


This regex will match quotes anywhere in the string not just those surrounding it. It should be anchored at the start of the string and shouldn't have the + (e.g., /^[\'"]/).

It should also be anchored at the end probably /^[\'"].*[\'"]$/

smgallo · 2018-12-04T16:26:13Z

classes/DataWarehouse/Access/Usage.php

+            $filterValueIsString = preg_match('/[\'"]+/', $usageFilterValue) === 1;
+
+            $value = $filterValueIsString
+                ? preg_replace('/[\'"]+/', '', $usageFilterValue)


This regex will trim any quote character from the string, which may not be what we want if we allow passing of a person's name in the future as it will remove apostrophes from anywhere in the string. We would be better served using trim($usageFilterValue, '\'"').

Nice, didn't know that PHP's trim allowed a char list to be trimmed. I'll update that.

smgallo · 2018-12-04T16:33:04Z

classes/DataWarehouse/Access/Usage.php

+            if ($rows !== false) {
+                $rowCount = count($rows);
+
+                if ($rowCount === 0 && !$filterValueIsString) {


Since we are now forcing the caller to specify whether the value is an integer id or a string that needs lookup in a dimension table, we can get rid of this logic. If the value is not enclosed in quotes we can return immediately.

smgallo · 2018-12-04T17:06:46Z

classes/DataWarehouse/Access/Usage.php

+            }
+
+            // If a string was provided but no id(s) were found then exception out.
+            throw new Exception("Invalid filter value detected");


Lets include the invalid filter in the error message:

Suggested change

throw new Exception("Invalid filter value detected");

throw new Exception(sprintf("Invalid filter value detected: %s", $usageFilterValue));

facepalm Ignore the most recent commit, added the type instead of the value.

ryanrath · 2018-12-04T20:24:30Z

Alrighty, @jpwhite4 @smgallo I think I've got things up to date w/ the comments on here / what we've talked about ( and updated w/ xdmod8.1 ). If you could take another look at things and let me know if there's any other changes you'd like to see before merging that would rock.

classes/DataWarehouse/Access/Usage.php

Added the ability for Usage chart requests to provide non-numeric values for '_filter' parameters. The new feature will attempt to translate these non-numeric values into appropriate id values that can be used during chart generation. Specifically this new feature is to support integration w/ Coldfront.

Per code review comments I've simplified this function. - Moved the `is_numeric` check to the top of the function and used it as a short circuit. - Removed the anonymous function and moved the code it contained to the bottom of the function, executed contingent on which `case`, if any, is hit.

This new test checks that the `_filter` variables accept non-id values (optionally, multiple comma delimited values). The test data includes the following: - Tests run as all users ( pub, cd, cs, pi, usr ) - Multiple Realms ( Jobs, Cloud ) - For the Jobs realm, all combinations of: ( including single, multiple, numeric & non-numeric values ) - 'resource', 'resource_filter' - 'pi', 'pi_filter' - For the Cloud realm: ( including single, multiple, numeric, and non-numeric values ) - 'project', 'project_filter'

As this function should only ever return one value the code has been updated so that this is the case.

- Updated `translateFilterValue` to return an array of values and the calling code to work with an array as opposed to a single value. This is to support the use case of a system account being associated with more than one person. - Updated `translateFilterValue` to only interpret `$usageFilterValue` as a string if it contains quotes ( single or double ). This allows us to identify when the calling code is passing us a value that is intended to be interpreted as a string not a number. - Updated Test Cases so that string values are quoted with both single / double quotes. - Added additional test cases that test what happens when a string value is provided that is not found in the db.

Refactored a portion of the `convertChartRequest` function and combined the two `for-each` loops that were populating the `$meFilters` variable. Now we have one loop that has the same functionality. Also a tiny comment update in the `translateFilterValue` function for the 'pi' filter type. Now that we always return an array the previous comment didn't really make sense anymore.

Now that we have a reliable test for what constitutes a "string" ( i.e. must be quoted ) we can simplify the internals of `translateFilterValue` by removing the vast majority of the if/elseif block. Also added a few comments to help provide context.

Added `$usageFilterType` to the message for the exception thrown in `translateFilterValue`.

Added the invalid `$usageFilterValue` to the exception message and updated the section in the tests that handled detecting when exceptions are thrown to handle this update.

Co-Authored-By: ryanrath <ryanrath@buffalo.edu>

ryanrath requested a review from smgallo November 1, 2018 14:05

ryanrath added this to the 8.1.0 milestone Nov 1, 2018

ryanrath added the enhancement Enhancement of the functionality of an existing feature label Nov 1, 2018

plessbd changed the base branch from xdmod8.0 to xdmod8.1 November 9, 2018 16:55

jpwhite4 requested changes Nov 21, 2018

View reviewed changes

ryanrath force-pushed the coldfront_integration_lite branch from 196b1ad to e3b250e Compare November 27, 2018 14:47

jpwhite4 reviewed Nov 28, 2018

View reviewed changes

smgallo suggested changes Dec 4, 2018

View reviewed changes

smgallo reviewed Dec 5, 2018

View reviewed changes

classes/DataWarehouse/Access/Usage.php Outdated Show resolved Hide resolved

smgallo previously approved these changes Dec 5, 2018

View reviewed changes

ryanrath and others added 14 commits December 10, 2018 09:52

Updates to support multiple string values

e0ad5c2

Style fix

bfd9fcd

Updating documentation

152dbb1

Style Fix

20926ef

Updates per @jpwhite4 comment

9f1e4d2

As this function should only ever return one value the code has been updated so that this is the case.

Simplified translateFilterValue

f149e41

Now that we have a reliable test for what constitutes a "string" ( i.e. must be quoted ) we can simplify the internals of `translateFilterValue` by removing the vast majority of the if/elseif block. Also added a few comments to help provide context.

Providing more informative exception info

e5d01de

Added `$usageFilterType` to the message for the exception thrown in `translateFilterValue`.

Updating the translateFilterValue exception message

fb095a8

Added the invalid `$usageFilterValue` to the exception message and updated the section in the tests that handled detecting when exceptions are thrown to handle this update.

Update classes/DataWarehouse/Access/Usage.php

0f83768

Co-Authored-By: ryanrath <ryanrath@buffalo.edu>

ryanrath dismissed smgallo’s stale review via 0f83768 December 10, 2018 14:54

ryanrath force-pushed the coldfront_integration_lite branch from ab0fc2a to 0f83768 Compare December 10, 2018 14:54

jpwhite4 approved these changes Dec 10, 2018

View reviewed changes

ryanrath merged commit 6df963f into ubccr:xdmod8.1 Dec 10, 2018

smgallo changed the title ~~Add new functionality to Usage chart requests~~ Support non-numeric values for Usage chart filter parameters Apr 10, 2019

smgallo added the Category:Metric Explorer Metric Explorer / Usage label Apr 10, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support non-numeric values for Usage chart filter parameters #716

Support non-numeric values for Usage chart filter parameters #716

ryanrath commented Nov 1, 2018 •

edited by smgallo

Loading

jpwhite4 left a comment

jpwhite4 Nov 21, 2018

ryanrath Nov 27, 2018

jpwhite4 Nov 21, 2018

ryanrath Nov 27, 2018

jpwhite4 Nov 21, 2018

ryanrath Nov 27, 2018

jpwhite4 Nov 21, 2018

ryanrath Nov 28, 2018

jpwhite4 Nov 28, 2018

smgallo Dec 4, 2018

ryanrath Dec 4, 2018

jpwhite4 commented Nov 28, 2018

ryanrath commented Dec 3, 2018

smgallo Dec 4, 2018

ryanrath Dec 4, 2018

smgallo Dec 4, 2018

ryanrath Dec 4, 2018

smgallo Dec 4, 2018

smgallo Dec 4, 2018

ryanrath Dec 4, 2018

ryanrath commented Dec 4, 2018

	throw new Exception("Invalid filter value detected");
	throw new Exception(sprintf("Invalid filter value detected: %s", $usageFilterValue));

Support non-numeric values for Usage chart filter parameters #716

Support non-numeric values for Usage chart filter parameters #716

Conversation

ryanrath commented Nov 1, 2018 • edited by smgallo Loading

Description

Motivation and Context

Tests performed

Types of changes

Checklist:

jpwhite4 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpwhite4 commented Nov 28, 2018

ryanrath commented Dec 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanrath commented Dec 4, 2018

ryanrath commented Nov 1, 2018 •

edited by smgallo

Loading