Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
wants to merge 5 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
43d0fdf
#62 Added MV link proto, working with local domain
Sep 24, 2021
f34932c
#62 Added ModelViewerUrlProvider, integrated into html renderer
Sep 30, 2021
af58a70
#62 Updated api endpoint to new version, changed placement of class.
Nov 4, 2021
82e3d53
Merge branch 'master' into issues/62/MV-link
Nov 4, 2021
b82b4e7
#62 Moved import statement to start of file
Nov 4, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 3 additions & 3 deletions source/incqueryserver-jupyter/iqs_jupyter/analysis_extensions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,9 @@ def _monkey_patch_analysis_result_repr_html(self : iqs_client.AnalysisResult, he
param_headers = " ".join(["<th>{}</th> ".format(html.escape(param)) for param in pattern_params])
match_rows = "\n".join([
"<tr><th>{}</th><td>{}</td>{}</tr>\n".format(row_index, self.matches[row_index].message, " ".join([
" <td>{}</td>".format(_cell_to_html(_dict_to_element(argument_value.value)))
" <td><a href=\"{}\">{}</a></td>".format(
_dict_to_element(argument_value.value).url,
_cell_to_html(_dict_to_element(argument_value.value)))
Comment on lines +148 to +150
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see my comments at query_execution_response_to_html

for argument_value in self.matches[row_index].matching_elements
])) for row_index in range(len(self.matches))
])
Expand All @@ -170,8 +172,6 @@ def _monkey_patch_analysis_result_repr_html(self : iqs_client.AnalysisResult, he
</div>
'''.format(header_report, style, param_headers, match_rows)



def _do_monkey_patching():
iqs_client.AnalysisResults._repr_html_ = _monkey_patch_analysis_results_repr_html
iqs_client.AnalysisResult._repr_html_ = _monkey_patch_analysis_result_repr_html
Expand Down
10 changes: 9 additions & 1 deletion source/incqueryserver-jupyter/iqs_jupyter/authentication.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@
from iqs_jupyter import api_composition
from iqs_client_extension import ApiClientWithOIDC

import requests


def connect(
address=defaults.default_IQS_address,
Expand Down Expand Up @@ -58,7 +60,6 @@ def connect(

return IQSClient(configuration, use_oidc)


class IQSClient:
def __init__(
self,
Expand All @@ -71,6 +72,13 @@ def __init__(
api_composition.decorate_iqs_client(self, root_configuration, ApiClient)
self.jupyter_tools = ext_point.IQSJupyterTools(self)

# Acquiring Model Viewer base link from host through internal API
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"]))
Comment on lines +76 to +79
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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




class IQSConnectorWidget:
def __init__(
Expand Down
4 changes: 1 addition & 3 deletions source/incqueryserver-jupyter/iqs_jupyter/core_extensions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,8 +70,6 @@ def _recognize_typed_element_in_compartment_descriptor(
ext_point.element_dict_recognizers.append(_recognize_typed_element_in_compartment_descriptor)




## monkey patch section

def _monkey_patch_element_in_compartment_descriptor_repr_html(self):
Expand Down Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

])) for row_index in range(len(match_list))
])

Expand Down
22 changes: 18 additions & 4 deletions source/incqueryserver-jupyter/iqs_jupyter/helpers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,16 +31,30 @@ def cell_to_html(cell):
else:
return str(cell)

def dict_to_element(dict_or_attribute_value, url_provider=None):
def dict_to_element(dict_or_attribute_value):
if isinstance(dict_or_attribute_value, dict):
for recognizer in ext_point.element_dict_recognizers:
recognized = recognizer(dict_or_attribute_value)
if recognized:
if url_provider is not None:
recognized.url = url_provider(recognized)
for url_provider in ext_point.url_providers:
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']))
Comment on lines +40 to +41
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Why was simple field assignment replaced by setattr?

  2. 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.

  3. 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.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your comments!

  1. There is no url field of elements in the case of query execution
  2. 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.
  3. We will have to enhance the html representation to handle multiple links for an element

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. 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?
  2. 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.
  3. Yes, but that HTML representation is generated here, so it would make sense to have it as part of this PR.

return recognized
# not recognized as an element, treated as raw dict
# not recognized as an element, treated as raw dict
return dict_or_attribute_value
else:
return dict_or_attribute_value

# def dict_to_element(dict_or_attribute_value, url_provider=None):
# if isinstance(dict_or_attribute_value, dict):
# for recognizer in ext_point.element_dict_recognizers:
# recognized = recognizer(dict_or_attribute_value)
# if recognized:
# if url_provider is not None:
# recognized.url = url_provider(recognized)
# return recognized
# # not recognized as an element, treated as raw dict
# return dict_or_attribute_value
# else:
# return dict_or_attribute_value

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please remove commented out older version

20 changes: 18 additions & 2 deletions source/incqueryserver-jupyter/iqs_jupyter/tool_extension_point.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,24 @@ def __init__(
self._iqs = iqs


# connector-specific extensions may add custom ways to convert a dict to an element representation;
# connector-specific extensions may add custom ways to convert a dict to an element representation;
# parse a dict and return the constructed element or None if not the right format
# default is ElementInCompartmentDescriptor
# default is ElementInCompartmentDescriptor
element_dict_recognizers : List[Callable[[dict], Optional[Any]]] = []


url_providers: List[Callable[[Any], Optional[Any]]] = []

class ModelViewerUrlProvider:
def __init__(self, iqs, mv_address=None):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

self.mv_address = mv_address
self.iqs = iqs

def __call__(self, element):
if "compartmentURI" in element and "relativeElementID" in element:
# Creating ModelViewer Link
compURI = element['compartmentURI']
relElementID = element['relativeElementID']
return f"{self.mv_address}?compartmentURI={compURI}&elementId={relElementID}"
else:
return None