Closed maybe-sybr closed 3 years ago
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.
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.
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.
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!
@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.
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.
@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.
These elements of the response are optional, so we should handle situations when they aren't provided at all.