-
-
Notifications
You must be signed in to change notification settings - Fork 152
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix getting ResultsRange field #2523
Fix getting ResultsRange field #2523
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Bugerman58 for the fixture.
Although I don't get fully why this happens, I would suggest some changes to make the whole thing more readable/understandable.
Maybe you can log an informative message or add a comment why the attribute might be missing?
self.specification = None | ||
if self.analysisrequest: | ||
has_specification = hasattr(self.analysisrequest, "getSpecification") | ||
if self.analysisrequest and has_specification: | ||
self.specification = self.analysisrequest.getSpecification() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please refactor to a class property as follows:
@property
def specification(self):
spec = None
try:
spec = self.analysisrequest.getSpecification()
except AttributeError:
pass # or maybe log an error?
return spec
if not sample: | ||
if not sample or not hasattr(sample, "ResultsRange"): | ||
return {} | ||
|
||
# Search by keyword |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please refactor as follows:
...
rr = self.get_results_range_for(sample)
def get_results_range(self, sample):
rr = None
try:
sample.getResultsRange()
except AttributeError:
pass # or log an error?
return rr
This exception arises of creating report for 'Anaylsis' content type and every time when update query for this report. To find out the reason for this exception, we added logging for this object and found: ...
'<Products.ATContentTypes.tool.factory.TempFolder object at 0x7f6e3c3f5b50>': 'RequestContainer' object has no attribute 'getField'
... , and after that: ...
'<Products.ATContentTypes.tool.factory.TempFolder object at 0x7f6e3c3f5b50>': 'RequestContainer' object has no attribute 'getSpecification'
... |
def get_results_range(self, sample): | ||
results_range = None | ||
try: | ||
results_range = sample.ResultsRange |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please always use the field getter instead of accessing the attribute directly.
Maybe you can simplify the whole thing with a combination this:
def get_results_range_for(self, sample, analysis):
rr = {}
try:
field = sample.getField("ResultsRange", {})
rr = field.get(sample, search_by=analysis.getKeyword())
except AttributeError:
pass
return rr
Also use a more descriptive logging message. As you know that it is a sample, you could be more specific, e.g.
logger.error("Failed to get results range for sample '{}' and analysis '{}': "
"{}".format(sample.getId(), analysis.getKeyword(), str(e)))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, if at look on this code:
# Try with uid (this shouldn't be necessary)
service_uid = analysis.getServiceUID()
, getting the results range is solved by checking the existence of the Keyword
. Right?
Or to get results range by UID has the meaning?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. I'm not sure why we added this fallback. But maybe you can bake it in for backwards compatibility?
sorry for my carelessness (a large of commits) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thanks @Bugerman58. Much cleaner now!
Description of the issue/feature this PR addresses
by @toropok description
With DataBox report based on Analysis query we noticed that when
DefaultResultsRangeProvider
being called following exception raised immediately:Investigation shown that it comes from resultrangefield.py line 103: field = sample.getField("ResultsRange")
The DefaultResultsRangeProvider docstring states that it follows backward compatibility solution for PR: #1506
I've added couple
hasattr
checks to exclude exceptions riot in logs.Current behavior before PR
Exception thrown
Desired behavior after PR is merged
tomb silence in logs 🙂 everything goes nice and easy
--
I confirm I have tested this PR thoroughly and coded it according to PEP8
and Plone's Python styleguide standards.