Feat/filter control blocks

[ElijahSwiftIBM]

💡 Issue Reference

Issue: #5

💻 What does this address?

Control block filtering is a valuable enhancement. In basic cases it allows for simple validation that a control block containing the right data is collected, but in more complex cases, it allows users to grab information on just a singular relevant control block or collection of them in a large table.

📟 Implementation Details

The core control block class now has a new unordered map for filtration that is built on object creation. The filter is passed from block to block along with inclusion patterns and validated along the way. At the "end" of the line, the control block's value is validated against that of the filter. In the event of a match, the json data is returned, otherwise an empty json comes back. Large lists of control blocks like ASCB's and ASSB's were edited to filter out these null values, allowing for a true filter.

📋 Is there a test case?

I added the following tests in both the python and the shell test suites (the names in python are fare more descriptive)

test_cbxp_raises_cbxp_error_if_filter_uses_non_included_control_block
test_cbxp_raises_cbxp_error_if_filter_uses_unknown_key
test_cbxp_raises_cbxp_error_if_filter_dict_wrong_size (python only)
test_cbxp_raises_cbxp_error_if_filter_value_not_string (python only)
test_cbxp_raises_cbxp_error_if_no_filter_match
test_cbxp_can_use_basic_filter
test_cbxp_can_use_include_filter_with_generic_include
test_cbxp_can_use_include_filter_with_discrete_include

[lcarcaramo]

DCO signoff seems to be missing on all commits. Maybe consider squashing all of these commits into one commit that has a DCO signoff?

[lcarcaramo]

We should restrict filters to only "repeated" control blocks. Maybe we can have a boolean class attribute to keep track of this?

const bool repeated_;

[ElijahSwiftIBM]

Should we? What if we have a script design to regularly run and throw flags if a certain value is found in a singular control block like the PSA? I see a much STRONGER benefit to using them in repeated control blocks, but I'm not convinced that they should be DISALLOWED from others.

[lcarcaramo]

I think the use case for singleton control blocks is completely separate from the concept of filtering, which is meant to filter items in an array or list of items. If somebody wants to check the value of something in a singleton control block they can just do a comparison.

if psa["psapsa"] == "PSA ":
    // do something

The point of filtering is saving the user from having to manually construct their own logic for looping through and filtering repeated control block data rather than checking if a value in a single control block meets some condition.

[lcarcaramo]

Should no matches result in an error or should we simply just return an empty JSON string and then the user can decide what to do with it from there?

[ElijahSwiftIBM]

So I thought that returning an empty JSON with no context might somehow happen inadvertently in some other edge or error case. This is why I had it return an empty JSON, but have the CLI and Python interfaces treat it like an "error" anyway, rather than flagging this as an error in the main code. I am open to changes here, though.

[lcarcaramo]

Based on how things work in jq, I am leaning towards, not giving an error, but instead just returning null or None in Python.

$ ./dist/cbxp -i cvt psa | jq '.junk.junk'
null

[lcarcaramo]

Change 55 to 255.

[lcarcaramo]

Change to filters_string since we can have multiple like with includes_string.

[lcarcaramo]

Change to createOptionsList() to better indicate that this is used to process an options list (i.e., filters or include patterns). We should also change variable names and debug messages accordingly to make this more clear.

[lcarcaramo]

Should we have one "options map" that contains both the filters and include patterns?

We could maybe have a cbxp_options_t struct in the control_block.hpp header.

typedef struct {
  std::vector<std::string> include_patterns;
  std::vector<std::string> filters;
} cbxp_options_t;

Then we can build a class attribute options_map_ with the following structure:

std::unordered_map<std::string, cbxp_options_t> options_map_;

Then the logic of ControlBlock::createFilterMap() and ControlBlock::createIncludeMap() can be mostly left the same with the only exception being that they populate the same options_map_ structure. We can then have one protected ControlBlock::createOptionsMap() function that calls ControlBlock::createIncludeMap() and ControlBlock::createFilterMap() to build the options_map_ structure. Those two functions can then be made private.

Now when we need to loop through these options, we can do something like this. A benefit of this is that we no longer be at risk of running into a scenario where "assb" for example is present in the include map, but somehow not present in the filters map, and overall I think this would enable better encapsulation as well.

  for (const auto& [include, cbxp_options] : options_map_) {
    if (include == "assb") {
      ascb_json["ascbassb"] = CBXP::ASSB(cbxp_options)
                                  .get(p_ascb->ascbassb);
      if (ascb_json["ascbassb"].is_null()) {
        return {};
      }
    }

[lcarcaramo]

There currently seems to be a bug with the string comparison logic here. If I try to use a filters like the following, I always get an empty list back:

./dist/cbxp -i assb -f 'assb.assbjbni=JOB     ' ascb
./dist/cbxp -i assb -f assb.assbjbni=JOB ascb
./dist/cbxp -f 'assbjbni=JOB     ' assb

[lcarcaramo]

I figured it out. The problem is that the match function converts everything to lowercase. I think we should avoid doing that because we may end up running into data that is case sensitive at some point.

[lcarcaramo]

How long can an integer value be? My guess is that we can have integers longer than 32-bits. Given this, we should probably treat everything as int64_t for both the try an except. I don't know if we should try to handle figure out whether we are working with a signed or unsigned value, but maybe for now treating everything as uint64_t makes sense?

[lcarcaramo]

Does it make sense to create a cbxp_filter_t struct here to split the operation and value?

typedef struct {
  std::string operation;
  std::string value;
} cbxp_filter_t;

[lcarcaramo]

Call this ControlBlock:addCurrentFilter() for better clarity since this function adds items to the current_filters_ map?

[lcarcaramo]

Shouldn't we be requiring each filter to be its own string and not allow filters to contain commas like include patterns?

[lcarcaramo]

If we are doing a hex comparison, we should require the value to be prepended with 0x to indicate that the value is a hex value.

[lcarcaramo]

I think a better way of describing these tests would be tests for hex comparisons, no?

[lcarcaramo]

Rename to next_assb and use same naming convention for other repeated control blocks.

[lcarcaramo]

Rename this to compare()?