geneontology / amigo

AmiGO is the public interface for the Gene Ontology.
http://amigo.geneontology.org
BSD 3-Clause "New" or "Revised" License
29 stars 17 forks source link

Annotation filter GO class (direct) not working #718

Closed raymond91125 closed 1 week ago

raymond91125 commented 2 months ago

To reproduce: https://amigo.geneontology.org/amigo/term/GO:0008746 click on GO class (direct) shows Total annotations: 103 (89) NAD(P)+ transhydrogenase activity (9) NAD(P)+ transhydrogenase (AB-specific) activity (5) NAD(P)+ transhydrogenase (B-specific) activity click on any line shows Total annotations: 0

cmungall commented 2 months ago

Confirmed.

I think there is something in the strings that are causing issues - I thought at first parentheses, but the ones under here work:

https://amigo.geneontology.org/amigo/term/GO:0016671

But some like alcohol dehydrogenase (NAD ) activity, zinc-dependent don't https://amigo.geneontology.org/amigo/term/GO:0016616

very odd...

cmungall commented 2 months ago

oh, interesting. This is the solr query:

https://golr.geneontology.org/solr/select?defType=edismax&qt=standard&indent=on&wt=json&rows=10&start=0&fl=*,score&facet=true&facet.mincount=1&facet.sort=count&json.nl=arrarr&facet.limit=25&hl=true&hl.simple.pre=%3Cem%20class=%22hilite%22%3E&hl.snippets=1000&fq=document_category:%22annotation%22&fq=isa_partof_closure:%22GO:0008746%22&fq=annotation_class_label:%22NAD(P)+%20transhydrogenase%20activity%22&facet.field=aspect&facet.field=taxon_subset_closure_label&facet.field=type&facet.field=evidence_subset_closure_label&facet.field=regulates_closure_label&facet.field=isa_partof_closure_label&facet.field=annotation_class_label&facet.field=qualifier&facet.field=annotation_extension_class_closure_label&facet.field=assigned_by&facet.field=panther_family_label&q=*:*&packet=2&callback_type=search&json.wrf=jQuery21403115388578178395_1714613182562&_=1714613182564

Notice that + is translated ` (double space). This is because the+is a URL encoding of a space. The solution is to use the right URL escaping when constructing the SOLR urls. We should also check other characters that typically get encoded, e.g.:`.

kltm commented 1 month ago

I talked a little with @pkalita-lbl about proximate places to look for the issue. While adding a fix (encoding/decoding), there may also be consequences in the UI filters and bookmarking, in addition to the "main" issue of the query to the Solr server.

There are some oddities that have slipped into the code over the years in response to years of LBL testing (e.g. https://github.com/geneontology/amigo/issues?q=is%3Aissue+XSS+is%3Aclosed); the bug may be a consequence of that (or just a regular "oops" on my part). Either way, a fix should be XSS-safe.

The problem code is probably in the bbop-manager-golr package, at https://github.com/berkeleybop/bbop-manager-golr/blob/master/lib/manager.js . A possible place to start might be https://github.com/berkeleybop/bbop-manager-golr/blob/da53ce470ad6672b356f2ff0a8abf2bb3d3682fe/lib/manager.js#L945 which uses encodeURI(). One way to approach this would be by the tests in that package, although that may practically be a slow way forward.

kltm commented 1 month ago

The JS code is old, baroque, and quirky (ooo--prototype!). If any problems or confusions come up, it's probably faster to drag me in than directly wrestle with whatever was in my head at the time.

pkalita-lbl commented 3 weeks ago

Note to self: another good URI-encoding test case

kltm commented 3 weeks ago

Rolling into production in progress.

kltm commented 1 week ago

@pkalita-lbl Now testing on https://amigo-staging.geneontology.io

kltm commented 1 week ago

@pkalita-lbl Now testing on https://amigo.geneontology.org

kltm commented 1 week ago

Okay, (light) testing complete; fully in production.