markabrahams / node-net-snmp

JavaScript implementation of the Simple Network Management Protocol (SNMP)
211 stars 97 forks source link

Recently-introduced API inconsistency #159

Closed derrell closed 3 years ago

derrell commented 3 years ago

Recently, not-accessible index columns are no longer returned by queries. This appears to be true of the getTableRowCells method, too. What this means is that what's returned by getTableRowCells no longer matches what was passed to addTableRow or what would be necessary to pass to addTableRow to recreate that row.

Possible solutions:

  1. At the expense of a major version bump, I suspect that the best option is to pass the column array, without the index, to addTableRow, and add a new parameter that accepts the index value(s).
  2. An alternative might be to inject index values into what's returned by getTableRowCells.
  3. A further alternative might be to ignore the whole problem and accept the inconsistency.

I'm currently assuming (3) will be the end result, here, and am accommodating the inconsistency in my code by externally (in my code) implementing (2)... but that works in my case because I know that the index is always a single column, and it's always the very first column. The generic solution implemented in index.js would be more complicated.

markabrahams commented 3 years ago

Yep - those calls deal are dealing with two different things: addTableRow needs the entire row index and values, whereas getTableRowCells just returns what's in the table values, excluding things like foreign index fields, or (as we more recently introduced) "non-accessible" index fields. The complexity lies in that a table's row values sometimes include partial or full index information, but sometimes doesn't. Given this complexity, I didn't design it with a requirement that these two things needed to match.

I'm not sure that there's a clear set of outright advantages with (1), (2) or (3) - like many things, there are pros and cons to each.

For example, for your preferred (1), what do you do with row-included indexes like ifIndex in ifTable? Do you require a duplicate index number in the index parameter and the values parameter? This would probably seem odd and confusing to a library user, and opens the door to inconsistencies and having extra logic to deal with them e.g. if a user includes a different ifIndex value in the index parameter than they do to the ifIndex value in the row.

If there was a clear, superior method to the status quo (3), then I'd consider a breaking change to move to it, but at the moment, I'm not convinced that there is?

derrell commented 3 years ago

An alternative to eliding entirely those not-accessible index values, you could fill them with undefined in the result of getTableRowCells. That would retain the consistency of length between what's passed to addTableRow and what's returned by getTableRowCells.

If you don't like that idea, I don't have any better solutions at the moment. Feel free to close this if you feel the best solution is to ignore this. I don't have a strong opinion here, except that inconsistencies annoy me and I strive to avoid them when possible.

markabrahams commented 3 years ago

Yep - that would even up the lengths of those two. Although the downside of this is that it would introduce an inconsistency between the number of the columns in the actual MIB table row and the number of values in the getTableRowCells array. Having said that, this particularly inconsistency might not be that important to anyone!

Let's sit on this for a few days of contemplation before concluding/closing. Some divine inspiration may yet strike and guide us here! :-)

markabrahams commented 3 years ago

After some contemplation, and in the absence of a clear apparent winner, I'm inclined to keep the current behaviour here. The are inconsistencies any way you skin it, as the values in a MIB row and the set of index/row information required for creation are - at least in some cases - inherently inconsistent in length.