fix: Handle empty lists in TAXII status responses

[maybe-sybr]

These elements of the response are optional, so we should handle
situations when they aren't provided at all.

[maybe-sybr]

In the commit message, when I say these elements are optional, I'm referring to section 4.3.1 of the TAXII 2.1 spec:

... MAY contain the status of individual objects within the request (i.e. whether they are still pending, completed and failed, or completed and succeeded).

Let me know if there are other considerations I've missed.

[codecov-commenter]

Codecov Report

Merging #81 into master will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master      #81   +/-   ##
=======================================
  Coverage   94.89%   94.89%           
=======================================
  Files           8        8           
  Lines        1784     1784           
=======================================
  Hits         1693     1693           
  Misses         91       91           

Impacted Files	Coverage Δ
taxii2client/v20/__init__.py	90.14% <100.00%> (ø)
taxii2client/v21/__init__.py	91.54% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 355b9a3...0bdf84e. Read the comment docs.

[maybe-sybr]

Technically these don't need the or None at all any more, but handling it like this also deals with situations when the element might be otherwise falsey and cause confusion. I've not seen that occur though, so happy to simplify this if something things it's pointless.

[clenk]

Hi @maybe-sybr, thanks for the PR. Is there a specific bug you ran into that this will fix? Leaving them as [] should still handle the case where those elements are optional since it's falsy. I'm hesitant to change it to None, since that would be a breaking change, unless there's a specific issue with it.

[maybe-sybr]

Hi @clenk. Yep, this manifests when I use a custom backend in the cti-taxii-server which doesn't populate the successes, failures and pendings but does count them (since the count elements are required). My status responses don't have successes and friends so self.successes end up being set to [] in the diffed init method and the conditionals fail since len([]) != self.success_count for any non-trivial submission made to the server.

You mentioned that changing the default to None would be breaking - I presume that's because people are likely to have used patterns like for pending_id in self.pendings: ... or similar, yes? That makes sense, I can definitely strip that out of the diff. The actual meat of the change is in changing those conditional to not blow up when those elements aren't passed in. I think it would actually be better to do something like this though:

def __init__(.., successes=None, ..):
  self._successes = successes
@property
def successes(self):
  if self._successes is None: return []
  return self._successes

and use _successes for the checks against success_count and friends.

Any preference? I'll amend the PR when I see a reply. Thanks!

[clenk]

@maybe-sybr that makes sense. Yeah, patterns like what you mentioned is what I had in mind about breaking. I think we should take out that part. Also, can you add your success_count, et al fix to the v20/__init__.py file as well to be consistent in both versions? Thanks!

Forgive me for being obtuse (maybe I need coffee) but what is the advantage of your _successes approach? I think I prefer the simplicity of the current implementation.

[maybe-sybr]

Using a private member and exposing it with a property would let us use None rather than a falsey empty list. That'd be relevant if we cared to error out on responses which look like:

{
  "success_count": <non-zero>,
  "successes": [],
}

rather than omitting the successes element entirely as they should if they choose not to populate it. The difference is between having an empty list of successes vs having so successes element at all, which might be semantically important depending on your reading of the spec. The 2.1 spec doesn't seem to be too specific but it seems sensible that if any successes element (and friends) is provided, it must be a list with success_count number of elements and can only be empty if success_count == 0.

I'll get an amended diff for you shortly and will take the successes or [] approach for now - we can amend it to use the private members and accessors later if you end up liking that idea.

[maybe-sybr]

@clenk - sorry about the delay, got caught up with other stuff yesterday. I've just pushed an updated diff which is pretty minimal and should be fine if we're cool with not exploding on responses like the one I mocked in my previous comment. I'm happy to move forward with this diff.

[clenk]

This looks good to me, thank you @maybe-sybr!