qiime2 / q2-diversity

BSD 3-Clause "New" or "Revised" License
4 stars 45 forks source link

Special characters in metadata groups break links to graphs #317

Closed colinbrislawn closed 4 months ago

colinbrislawn commented 3 years ago

Bug Description A metadata subcategory called HF+ caused the generated .pdf and png files within the .qzv file to have different names, breaking the embedded figure and link on view.qiime2.org.

Screen Shot 2021-09-29 at 3 24 32 PM

Steps to reproduce the behavior See the attached file for a broken output: VIEWABLE_braycurtis-norm_feature-table_HF_Status_missingBoxPlot.zip

Expected behavior Links are preserved, through consistent file names or sanitized metadata categories.

Computation Environment

ebolyen commented 3 years ago

Thanks for the report! Looks like there's disagreement in URI encoding between JS and Python:

>>> urllib.parse.quote("+")
'%2B'

> encodeURI("+")
'+'

Which explains why this is happening. The visualizer seems to have escaped it with the expectation that q2view would do a similar escaping, which... normally it would, if not for the disagreement about +. Python's definitely wrong here, as + is one of the older "safe" characters to use in a URL.

thermokarst commented 3 years ago

Maybe we should transfer this issue to q2-diversity, that way we can address the quoting behavior there?

ebolyen commented 3 years ago

Actually, my memory is stale, q2view no longer URI encodes: https://github.com/qiime2/q2view/commit/ea899cc0945e730ca2796328403aa3d547fca280

ebolyen commented 3 years ago

Yeah perhaps, if q2view shouldn't do anything here.

It might make sense to attempt a search for the "python escaped" version of the file if we can't find it in the zip the first time. But I'm not sure if that's a good idea or a bad one.

thermokarst commented 3 years ago

I'm good with q2view remaining unchanged here - we know there have been issues with the quoting of various visualizations in q2-diversity (at my hand), so I'm good with pointing the blame there, lol.

colinbrislawn commented 4 months ago

I'm closing this in favor of #345