Mccalluc/user files next steps #1772
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #1772 +/- ##
===========================================
+ Coverage 39.97% 41.71% +1.74%
===========================================
Files 382 383 +1
Lines 25171 26074 +903
Branches 1267 1354 +87
===========================================
+ Hits 10061 10876 +815
- Misses 15110 15198 +88
Continue to review full report at Codecov.
|
refinery/data_set_manager/utils.py
Outdated
| except Study.MultipleObjectsReturned: | ||
| logger.error('Expected only one study for %s', investigation) | ||
| logger.error('Expected only one Study for %s', investigation) |
There was a problem hiding this comment.
except (Study.DoesNotExist, Study.MultipleObjectsReturned) as e:
We tend to include e in the logger.
refinery/data_set_manager/utils.py
Outdated
| # but there's nothing more to do here. | ||
| except: | ||
| logger.error('Expected only one assay for %s', study) | ||
| logger.error('Expected at least one Assay for %s', study) |
There was a problem hiding this comment.
same as Jennifer's remarks above
refinery/data_set_manager/utils.py
Outdated
| assay_uuids | ||
| )) | ||
| ) | ||
| assert len(assay_uuids) > 0, 'At least one assay must be accessible' |
There was a problem hiding this comment.
I don't think assert is inline with what we've been doing. We generally have been using try/excepts, but i do enjoy its simplicity. Maybe it could be up for debate?
Presumably
AssertionError is raised? so at the very least that should be handled.
There was a problem hiding this comment.
Nothing will go wrong at the next line if assya_uuids == [], except that the solr query constructed is invalid: so it seems best to catch it here. That said, perhaps what we really want is a custom error, that can be caught a couple layers up.
There was a problem hiding this comment.
Just committed a fix I like: We're inside a method which is building the query string. rather than using errors, we just return None is there query string would be invalid, and then at the top level that can be handled.
refinery/data_set_manager/utils.py
Outdated
| @@ -806,10 +800,17 @@ def format_solr_response(solr_response): | |||
| solr_response_json = json.loads(solr_response) | |||
| except TypeError: | |||
| return "Error loading json." | |||
| assert solr_response_json['responseHeader']['status'] == 0, \ | |||
There was a problem hiding this comment.
@scottx611x : I would like to leave this, actually: If solr is returning error responses to us, then we really do have a problem, and letting it bubble up and manifest as a 500 is appropriate: It's not something we can or should recover from.
This assertion isn't necessary for my code, but it will make it easier for any developer in the future by pointing them in the right direction.
There was a problem hiding this comment.
@mccalluc It may be useful to return HTTP 500 response in a case like this but exceptions should not be left unhandled: "Care must be taken when handling exceptions to ensure proper application cleanup while maintaining useful error reporting." https://doughellmann.com/blog/2009/06/19/python-exception-handling-techniques/
There was a problem hiding this comment.
I'm not on the same page right now, but this line shouldn't block the PR. I'll revert it; meanwhile, I've filed #1789 to try to explain where I'm coming from. Sorry for the distraction.
refinery/data_set_manager/utils.py
Outdated
|
|
||
| # Reorganizes solr response into easier to digest objects. | ||
| order_facet_fields = solr_response_json.get('responseHeader').get( | ||
| 'params').get('fl').split(',') | ||
| order_facet_fields_joined = solr_response_json.get('responseHeader')\ |
There was a problem hiding this comment.
We've been generally avoiding \
There was a problem hiding this comment.
Also I think the chaining of get()s would be cleaner like so:
try:
order_facet_fields_joined = solr_response_json["response_header"]["params"]["fl"]
except KeyError as e:
# handle error
else:
# forge ahead
refinery/user_files_manager/views.py
Outdated
| params, | ||
| user_uuid=request.user.uuid) | ||
| try: | ||
| solr_params = generate_solr_params_for_user( |
There was a problem hiding this comment.
Minor styling:
solr_params = generate_solr_params_for_user(
params,
user_uuid=request.user.uuid
)
|
@scottx611x : see if this addresses everything? |
|
@scottx611x : I think that one line was the last remaining problem? Let me know if you see anything else. |
This continues the work on user file listing begun in #1749. This PR replaces #1759. This is generally about better or more graceful error handling.