Skip to content
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

[21.09] Skip non JSON-encodable values in params_to_strings() #13826

Open
wants to merge 3 commits into
base: release_21.09
Choose a base branch
from

Conversation

nsoranzo
Copy link
Member

@nsoranzo nsoranzo commented Apr 28, 2022

Fix the error reported in #13455 , which was due to BioMart sending some binary empty strings as values, e.g. for the hsapiens_gene_ensembl__filter.chromosomal_region__file key.

N.B.: the BioMart tool still doesn't work after this fix (probably BioMart's fault), but at least Galaxy doesn't fail with the mysterious: "Error executing tool id 'biomart': Object of type bytes is not JSON serializable"

Also, cherry-pick commits from dev to fix test failures.

How to test the changes?

(Select all options that apply)

License

Fix the error reported in galaxyproject#13455 ,
which was due to BioMart sending some binary empty strings as values, e.g. for
the `hsapiens_gene_ensembl__filter.chromosomal_region__file` key.

N.B.: the BioMart tool still doesn't work after this fix (probably BioMart's fault), but
at least Galaxy doesn't fail with the mysterious:
"Error executing tool id 'biomart': Object of type bytes is not JSON serializable"
@github-actions github-actions bot added this to the 22.01 milestone Apr 28, 2022
@nsoranzo nsoranzo mentioned this pull request Apr 28, 2022
davelopez and others added 2 commits April 28, 2022 15:29
Co-authored-by: Nicola Soranzo <nicola.soranzo@gmail.com>
@nsoranzo
Copy link
Member Author

The failing Selenium tests are unrelated.

value = dumps(value, sort_keys=True)
except Exception as e:
log.warning(f"Error while serializing value {value} for key {key}: {e}")
continue
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems risky, don't you want to raise here ?

@mvdbeek
Copy link
Member

mvdbeek commented May 5, 2022

I think the issue is that we're not properly receiving the multi-part request from biomart in the legacy routes. The error message is of course a little unfortunate, but that's better than running a tool with incomplete data. If we want to special case something for biomart we should probably do that in

def biomart(self, trans, tool_id="biomart", **kwd):
, but I don't really understand the form data and what we're supposed to do with it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants