Blitz buildQuery params by will-moore · Pull Request #4950 · ome/openmicroscopy

will-moore · 2016-11-17T16:04:19Z

What this PR does

This is a subset of #4945 containing just the BlitzGateway and test changes.

This moves more logic for building queries to the BlitzGateway, to reduce duplication within api_query.py.

This means that the _getQueryString() for each BlitzGateway Wrapper object now returns a base query string, params and clauses. These are then augmented in conn.buildQuery(), which is used both by conn.getObjects() (and by the web /api_query.py - see #4945).

The params argument in
conn.getObjects("Project", params)
can now be an omero.sys.Parameters object (as before) OR can be a dictionary of parameters to support additional filtering.
E.g. {'page': 1, 'limit': 100, 'order_by': 'name', 'owner': 1, 'project': 5}

cc @chris-allan

will-moore · 2016-11-28T07:29:12Z

@chris-allan If you could have a look at this when you get time, that would be great, thanks. And #4930 too (lower priority). Cheers.

chris-allan · 2016-11-30T09:01:48Z

        return obj.getValue()

-    def _getQueryString(self):
+    def _getQueryString(self, opts=None):


As this is going to be used more extensively now and its return value has changed to a tuple the function needs pretty thorough documentation and we should definitely mark it for detailed review in the changelog. We have many examples where this method is overridden and/or its return value is used.

chris-allan · 2016-11-30T09:05:06Z

+        opts = None
+        inputParams = None
+
+        if isinstance(params, dict):


I'm really not sure about this. At a minimum it needs documentation and to detailed review in the changelog as above. While it certainly keeps the prototype is it not feasible to add opts as an additional keyword argument?

Understood. I wasn't sure about this either. I was just trying to avoid too many arguments, thinking that use of params would mostly be covered by opts instead (they cover mostly the same functionality). But it can get confusing in the implementation.
I'm happy to add opts as additional argument. Then there's the question of whether params or opts takes precedence (E.g. if you specify different pagination or owner in each)?

Adding opts as a keyword argument is certainly preferable from my perspective. @aleksandra-tarkowska would probably like the opportunity to comment.

I think you can choose one and document it as taking precedence. Whichever one is the subset of the other is probably the a good candidate but I can see arguments both ways. That is, if you have the possibility to configure 50 things with opts and have params only able to configure 10 the settings from params should probably override those set in params if a call were made where both were present.

where is the unit test?

chris-allan · 2016-11-30T09:06:49Z

-                "join fetch obj.details.group "
-                "join fetch obj.details.creationEvent")
+        query = ("select obj from Annotation obj "
+                 "join fetch obj.details.owner as owner "


Is loosing the details.group fetch here not a regression?

See ce55154 in original PR #4945 for when and why details.group was removed from these queries. cc @joshmoore

Cool, no problem. Just something we should document and probably schedule time to fix at a fundamental level. During work with omero-es I haven't noticed issues like this and I'm definitely loading multiple groups that are not the default group. So I guess it's a combination of factors?

/cc @aleksandra-tarkowska

@chris-allan See https://trac.openmicroscopy.org/ome/ticket/11362#comment:9 for how to reproduce.

could you add unit test as well

@aleksandra-tarkowska Unit tests for every _getQueryString() would be a lot to add to this PR, but maybe just for the ones that handle opts parameter.

why? we need one at least for parent. You could parametrize the test. In my opinion well written test is much better then the doc

chris-allan · 2016-11-30T09:07:10Z

-                "join fetch obj.details.group "
-                "join fetch obj.details.creationEvent join fetch obj.file")
+        query = ("select obj from FileAnnotation obj "
+                 "join fetch obj.details.owner as owner "
