Bugfixes for Intermine (TargetMine) connectivity - #6
Conversation
…mes of lists to be exported. Lists were sometimes deleted twice, which resulted in a bug on the server side. Also, remove the obsolete scheme where lists are sometimes named with the 'Set:' prefix, sometimes not, and we checked for both names.
…at ivy jars can be overwritten if needed
…am fixes a bug related to unresolved identifiers
| <!-- Set up WEB-INF/lib with jars --> | ||
| <delete dir="${wardir}/WEB-INF/lib" failonerror="false" /> | ||
| <mkdir dir="${wardir}/WEB-INF/lib" /> | ||
| <ivy:retrieve pattern="${wardir}/WEB-INF/lib/[artifact]-[revision].[ext]" conf="runtime" type="bundle,jar"/> |
There was a problem hiding this comment.
Moving this "ivy:retrieve" to be sequentially prior to the "copy" command just below allows the latter to copy the mlib/ jars (on manual.classpath) over the ivy retrieved jars.
Thus, we retrieve intermine-ws-java and its dependencies into the classpath, and then overwrite its main jar with our custom one, while still keeping its dependencies.
yujiberra
left a comment
yujiberra
left a comment
There was a problem hiding this comment.
@jtnystrom
Thanks for making this PR!
If it's okay with you I'd like to approve this without testing it since I don't have a servlet container set up and I don't want to deal with that right now, and I'll be able to do testing once you set up an endpoint in the Scalatra app.
I can get a servlet container set up though if you'd be more comfortable that way.
For now I've just looked over the code, and I'd say it looks reasonable based on what you wrote in the PR body; I made one comment but it's not that important.
| * Delete pre-existing lists with the same name as the one about to be exported, if replacement was requested. | ||
| * Returns a list name that may be used if the export can proceed, or None if it cannot. | ||
| */ | ||
| private def validNameForExport(ls: ListService, name: Option[String], | ||
| replace: Boolean): Option[String] = { | ||
|
|
||
| var serverList = name.map(n => Option(ls.getList(n))).flatten | ||
| if (serverList != None && replace) { | ||
| ls.deleteList(serverList.get) | ||
| } | ||
| private def checkListNameForExport(service: ListService, name: String, replace: Boolean): Option[String] = { |
There was a problem hiding this comment.
I understand things are this way as an artifact of the old implementation, but my personal inclination would be to either split this into two methods, or to just move the logic into addEntrezList, because I feel it does two conceptually separate things:
- delete existing lists with the same name (if replace is true)
- check if
nameis usable- (if this were going to be separated out into a function it would probably make more sense with a boolean return value)
Definitely not a high priority though, so I wouldn't object to this being merged as-is.
There was a problem hiding this comment.
Thanks for your comments, see what you think of the latest changes.
…c to addEntrezList. Add comments. Change the tag for newly created lists from "toxygates" to "panomicon".
yujiberra
left a comment
yujiberra
left a comment
There was a problem hiding this comment.
@jtnystrom
Thanks for the change, I think the logic is easier to understand now.
Since you didn't respond to my other comment I'm going to assume you're okay with my not actually running/testing the code, so LGTM!
Yes I'm fine with that, thanks for reviewing! |
2 participants
We rely on the intermine-ws-java library to connect to InterMine instances such as TargetMine.
For some time, this library has had a bug where list creation on the server will fail if the list contains unresolved identifiers that are not strings (e.g. integers like Entrez genes). I believe that I have now fixed that bug. For more details on that problem, please see the PR that I opened in the upstream project: intermine/intermine-ws-java#7
It's not clear how long it'll take until that PR gets merged (if it gets merged), so for the time being I am adding a custom compiled jar that contains the bugfix to our Toxygates/mlib. If upstream accepts and publishes the bugfix, I will remove our custom jar again.
The antlr-2.7.6 dependency had been removed prematurely and I am restoring it here since intermine-ws-java still needs it.
Other minor bugfixes:
I tested that all Intermine related functions work by importing/exporting gene sets (gene lists) using the GWT application. The next step will be to implement a Scalatra API for the same functionality, as discussed.