#62 MV Link generation from IQS client & API call #65

banko-marton · 2021-11-04T12:10:13Z

FIxes #62

Changes:

Obtain ModelViewer solution link from IQS connection
Created an URL provider which uses this link to create MV links for elements
Modified report generation to create links for elements to be viewed in MV

bergmanngabor

Thank you very much, not bad!

I do have a number of questions and comments though, let us discuss them.

bergmanngabor · 2021-11-04T16:25:03Z

source/incqueryserver-jupyter/iqs_jupyter/helpers.py

+                    if url_provider(dict_or_attribute_value['element']) is not None: # maybe we should set url param to None - else
+                        setattr(recognized, 'url', url_provider(dict_or_attribute_value['element']))


Why was simple field assignment replaced by setattr?

Will there be more than one URL provider (e.g. open in MV, open in Cameo Collab, open in Desktop Tool, etc.)?
If yes, then they will just trumple over each other, and the last one in the list will overwrite the other URLs.
I think if you truly want extensibility here, there you should allow multiple URLs assigned to a single element.

Also, if there are multiple link providers, we will need a way to distinguish these links on the UI (e.g "Open in Desktop App", "Open in Model Viewer", etc.), so each url provider entry should also come with a distinctive label or something.

Minor: avoid evaluating url_provider twice; save the return value to a local var that you then reuse.

Thank you for your comments!

There is no url field of elements in the case of query execution

Later url field can be a list, or a "domain" to "url" map in order to support multiple urls for an element.
At this point it is important to note that in IQS 2021.5.0 (coming in December) will be an API endpoint for providing
links to original service, IncQuery Desktop and Model Viewer.

We will have to enhance the html representation to handle multiple links for an element

In Python, you can just add fields to most objects by simple assignment - is there a reason why it would not work in this case? The last time I tested the original url_provider solution it still worked - but that was long ago, is there a reason the assignment needed to be replaced? Did the generated Python client change in a way that now the protocol data objects are no longer dict-backed?

Yes, I agree; I think dict would be the especially best. Anyway, either have a single url provider logic hardcoded for the time being, or have it as an extensible list, but then properly handle the list having multiple elements.

Yes, but that HTML representation is generated here, so it would make sense to have it as part of this PR.

bergmanngabor · 2021-11-04T16:50:49Z

source/incqueryserver-jupyter/iqs_jupyter/helpers.py

+#         return dict_or_attribute_value
+#     else:
+#         return dict_or_attribute_value
+


please remove commented out older version

bergmanngabor · 2021-11-04T16:54:31Z

source/incqueryserver-jupyter/iqs_jupyter/core_extensions.py

@@ -250,7 +248,7 @@ def _monkey_patch_query_execution_response_to_html(self):
    param_headers = " ".join(["<th>{}</th> ".format(html.escape(param)) for param in pattern_params])
    match_rows = "\n".join([
        "<tr><th>{}</th>{}</tr>\n".format(row_index," ".join([
-            " <td>{}</td>".format(_cell_to_html(match_list[row_index][param])) for param in pattern_params
+            " <td><a href=\"{}\">{}</a></td>".format(_cell_to_html(match_list[row_index][param]).url, _cell_to_html(match_list[row_index][param])) for param in pattern_params


This link.generating logic should be within _cell_to_html, not here.

Also, what if a given cell is not a model element, and thus has no url? Do we still generate an <a> tag with empty href?

_cell_to_html(match_list[row_index][param]).url

Wait, what? Does _cell_to_html not return a string? Are you really accessing the url field of a (HTML) string? How does that make sense? Will that ever work? I think there is a copy-past bug here; contrast with _monkey_patch_analysis_result_repr_html.

bergmanngabor · 2021-11-04T16:55:53Z

source/incqueryserver-jupyter/iqs_jupyter/authentication.py

+        req = requests.get(f"http://{root_configuration.host}/internal-api/solution-configuration")
+
+        # Registering MV URL provider, providing links to MV link received from API call above
+        ext_point.url_providers.append(ext_point.ModelViewerUrlProvider(self, req.json()["modelViewerBaseURL"]))


is there a specific reason why we do not use a generated OpenAPI client in python, instead of manual json parsing?

internal-api endpoints are not part of the generated client.

OK, but then why is this considered internal API, if it is used by clients to prettify their HTML presentation of query results? Does not sound very internal to me.

(We have a demo label for parts of the API that are not considered stable)

bergmanngabor · 2021-11-04T16:58:13Z

source/incqueryserver-jupyter/iqs_jupyter/tool_extension_point.py

+url_providers: List[Callable[[Any], Optional[Any]]] = []
+
+class ModelViewerUrlProvider:
+    def __init__(self, iqs, mv_address=None):


This is a statically / globally registered url provider; it should not be aware of the address of the Model Viewer instance. (What if we connect to multiple IncQuery Suite deployments with different addresses)? I would have expected mv_address information to be encoded in, perhaps, the iqs instance.

bergmanngabor · 2021-11-04T17:01:48Z

source/incqueryserver-jupyter/iqs_jupyter/analysis_extensions.py

+            " <td><a href=\"{}\">{}</a></td>".format(
+                _dict_to_element(argument_value.value).url,
+                _cell_to_html(_dict_to_element(argument_value.value)))


see my comments at query_execution_response_to_html

banko-marton added 4 commits September 24, 2021 14:31

#62 Added MV link proto, working with local domain

43d0fdf

#62 Added ModelViewerUrlProvider, integrated into html renderer

f34932c

#62 Updated api endpoint to new version, changed placement of class.

af58a70

Merge branch 'master' into issues/62/MV-link

82e3d53

banko-marton requested review from bergmanngabor and FuzesiMate November 4, 2021 12:10

banko-marton self-assigned this Nov 4, 2021

#62 Moved import statement to start of file

b82b4e7

bergmanngabor requested changes Nov 4, 2021

View reviewed changes

FuzesiMate closed this Feb 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#62 MV Link generation from IQS client & API call #65

#62 MV Link generation from IQS client & API call #65

banko-marton commented Nov 4, 2021

bergmanngabor left a comment

bergmanngabor Nov 4, 2021

FuzesiMate Nov 5, 2021

bergmanngabor Nov 5, 2021

bergmanngabor Nov 4, 2021

bergmanngabor Nov 4, 2021

bergmanngabor Nov 4, 2021

FuzesiMate Nov 5, 2021

bergmanngabor Nov 5, 2021

bergmanngabor Nov 5, 2021

bergmanngabor Nov 4, 2021

bergmanngabor Nov 4, 2021

		if url_provider(dict_or_attribute_value['element']) is not None: # maybe we should set url param to None - else
		setattr(recognized, 'url', url_provider(dict_or_attribute_value['element']))

#62 MV Link generation from IQS client & API call #65

#62 MV Link generation from IQS client & API call #65

Conversation

banko-marton commented Nov 4, 2021

bergmanngabor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment