Skip to content

FIREFLY-2076: Fix period in IDs causing problems for several methods #81

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

Closed
jaladh-singhal wants to merge 1 commit into from
Closed

FIREFLY-2076: Fix period in IDs causing problems for several methods #81

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions firefly_client/fc_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,17 @@ def gen_item_id(item):
return None


def validate_id(id_value, id_name):
"""
Raise ValueError if an ID (table, chart, or plot) contains a period ('.').
Firefly uses these IDs as lodash object paths, so a period splits the ID and
breaks the related UI (e.g. a table's charts and coverage image don't show).
"""
if id_value and '.' in id_value:
raise ValueError(f"{id_name} '{id_value}' must not contain a period ('.'); "
f"use another character such as '_' instead")


def create_image_url(image_source):
def is_url(url): return urllib.parse.urlparse(url).scheme != ''

Expand Down
20 changes: 17 additions & 3 deletions firefly_client/firefly_client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,10 @@
except ImportError:
from range_values import RangeValues
try:
from .fc_utils import debug, warn, dict_to_str, create_image_url, ensure3, gen_item_id,\
from .fc_utils import debug, warn, dict_to_str, create_image_url, ensure3, gen_item_id, validate_id,\
DebugMarker, ALL, ACTION_DICT, LO_VIEW_DICT
except ImportError:
from fc_utils import debug, warn, dict_to_str, create_image_url, ensure3, gen_item_id,\
from fc_utils import debug, warn, dict_to_str, create_image_url, ensure3, gen_item_id, validate_id,\
DebugMarker, ALL, ACTION_DICT, LO_VIEW_DICT
try:
from ._server_compat import MIN_SERVER_VERSION, FIREFLY_VERSION_KEY, is_server_compatible
Expand Down Expand Up @@ -881,6 +881,7 @@ def show_fits_image(self, file_input=None, file_on_server=None, url=None,
it will be ignored if `file_input` is also passed.
plot_id : `str`, optional
The ID you assign to the image plot. This is necessary to further control the plot.
It must not contain a period ('.').
viewer_id : `str`, optional
The ID you assign to the viewer (or cell) used to contain the image plot. If grid view is used for
display, the viewer id is the cell id of the cell which contains the image plot.
Expand Down Expand Up @@ -915,6 +916,8 @@ def show_fits_image(self, file_input=None, file_on_server=None, url=None,
If more than one of these parameters are passed, precedence order is:
service specific `additional_params` > `file_input` (> `file_on_server` > `url`).
"""
validate_id(plot_id, 'plot_id')

# WebPlotRequest parameters
wp_request = {'plotGroupId': 'groupFromPython',
'GroupLocked': False}
Expand Down Expand Up @@ -972,6 +975,7 @@ def show_fits_3color(self, three_color_params, plot_id=None, viewer_id=None):

plot_id : `str`, optional
The ID you assign to the image plot. This is necessary to further control the plot.
It must not contain a period ('.').
viewer_id : `str`, optional
The ID you assign to the viewer (or cell) used to contain the image plot. If grid view is used for
display, the viewer id is the cell id of the cell which contains the image plot.
Expand All @@ -989,6 +993,7 @@ def show_fits_3color(self, three_color_params, plot_id=None, viewer_id=None):
item.update({'Title': item.get('Title') if 'Title' in item else '3 Color'})
if 'plotId' not in item and plot_id:
item.update({'plotId': plot_id})
validate_id(item.get('plotId'), 'plot_id')

payload = {'wpRequest': three_color, 'threeColor': True, 'useContextModifications': True}
warning = None
Expand Down Expand Up @@ -1033,6 +1038,7 @@ def show_table(self, file_input=None, file_on_server=None, url=None,
it will be ignored if `file_input` is also passed.
tbl_id : `str`, optional
A table ID. It will be created automatically if not specified.
It must not contain a period ('.').
title : `str`, optional
Title associated with the table.
page_size : `int`, optional
Expand Down Expand Up @@ -1103,6 +1109,7 @@ def show_table(self, file_input=None, file_on_server=None, url=None,
"""
has_file_input = bool(file_on_server) or bool(url) or bool(file_input)

validate_id(tbl_id, 'tbl_id')
if not tbl_id:
tbl_id = gen_item_id('Table')
if not title:
Expand Down Expand Up @@ -1165,6 +1172,7 @@ def fetch_table(self, file_on_server, tbl_id=None, title=None, page_size=1, tabl
Firefly has direct access to.
tbl_id : `str`, optional
A table ID. It will be created automatically if not specified.
It must not contain a period ('.').
title : `str`, optional
Title associated with the table.
page_size : `int`, optional
Expand All @@ -1181,6 +1189,7 @@ def fetch_table(self, file_on_server, tbl_id=None, title=None, page_size=1, tabl
out : `dict`
Status of the request, like {'success': True}.
"""
validate_id(tbl_id, 'tbl_id')
if not tbl_id:
tbl_id = gen_item_id('Table')
if not title:
Expand Down Expand Up @@ -1345,7 +1354,7 @@ def show_chart(self, group_id=None, **chart_params):
Parameters for the chart. The options are shown as below:

**chartId**: `str`, optional
The chart ID.
The chart ID. It must not contain a period ('.').
**data**: `list` of `dict`, optional
A list of data for all traces of the plot.ly chart. Firefly-specific keys: *tbl_id*,
*firefly* (for Firefly chart types).
Expand All @@ -1359,6 +1368,7 @@ def show_chart(self, group_id=None, **chart_params):

"""
chart_id = chart_params.get('chartId') if 'chartId' in chart_params else gen_item_id('Plotly')
validate_id(chart_id, 'chartId')
warning = None
if self.is_triview():
warning = 'group_id unnecessary and ignored in triview mode' if group_id else None
Expand Down Expand Up @@ -1522,6 +1532,7 @@ def show_hips(self, plot_id=None, viewer_id=None, hips_root_url=None, hips_image
----------
plot_id : `str`, optional
The ID you assign to the image plot. This is necessary to further control the plot.
It must not contain a period ('.').
viewer_id : `str`, optional
The ID you assign to the viewer (or cell) used to contain the image plot. If grid view is
used for display, the viewer id is the cell id of the cell which contains the image plot.
Expand Down Expand Up @@ -1554,6 +1565,7 @@ def show_hips(self, plot_id=None, viewer_id=None, hips_root_url=None, hips_image

payload = {'wpRequest': wp_request}

validate_id(plot_id, 'plot_id')
if not plot_id:
plot_id = gen_item_id('Image')

Expand Down Expand Up @@ -1583,6 +1595,7 @@ def show_image_or_hips(self, plot_id=None, viewer_id=None, image_request=None, h
----------
plot_id : `str`, optional
The ID you assign to the image plot. This is necessary to further control the plot.
It must not contain a period ('.').
viewer_id : `str`, optional
The ID you assign to the viewer (or cell) used to contain the image plot. If grid view is
used for display, the viewer id is the cell id of the cell which contains the image plot.
Expand All @@ -1607,6 +1620,7 @@ def show_image_or_hips(self, plot_id=None, viewer_id=None, image_request=None, h
if not image_request and not hips_request:
return

validate_id(plot_id, 'plot_id')
if not plot_id:
plot_id = gen_item_id('Image')
warning = None
Expand Down
Loading