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

[SIP-15A] Enforcing ISO 8601 date/timestamp formats #7702

Merged
merged 1 commit into from
Oct 17, 2019
Merged

[SIP-15A] Enforcing ISO 8601 date/timestamp formats #7702

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
17 changes: 13 additions & 4 deletions superset/assets/src/datasource/DatasourceEditor.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,14 +97,23 @@ function ColumnCollectionTable({
<Field
fieldKey="python_date_format"
label={t('Datetime Format')}
descr={
descr={/* Note the fragmented translations may not work. */
<div>
{t('The pattern of the timestamp format, use ')}
{t('The pattern of timestamp format. For strings use ')}
<a href="https://docs.python.org/2/library/datetime.html#strftime-strptime-behavior">
{t('python datetime string pattern')}
</a>
{t(` expression. If time is stored in epoch format, put \`epoch_s\` or
\`epoch_ms\`.`)}
{t(' expression which needs to adhere to the ')}
<a href="https://en.wikipedia.org/wiki/ISO_8601">
{t('ISO 8601')}
Copy link
Member

Choose a reason for hiding this comment

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

none of this i18n stuff really works, but since it was bad before I don't see it worthwhile to fix now (or how best to fix it while still embedding links). Maybe we should add a TODO here to do the i18n the right way in the future?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

</a>
{t(` standard to ensure that the lexicographical ordering
coincides with the chronological ordering. If the
timestamp format does not adhere to the ISO 8601 standard
you will need to define an expression and type for
transforming the string into a date or timestamp. Note
currently time zones are not supported. If time is stored
in epoch format, put \`epoch_s\` or \`epoch_ms\`.`)}
</div>
}
control={<TextControl />}
Expand Down
2 changes: 2 additions & 0 deletions superset/connectors/sqla/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,8 @@ def dttm_sql_literal(self, dttm: DateTime) -> str:
return "'{}'".format(dttm.strftime(tf))
else:
s = self.table.database.db_engine_spec.convert_dttm(self.type or "", dttm)

# TODO(john-bodley): SIP-15 will explicitly require a type conversion.
return s or "'{}'".format(dttm.strftime("%Y-%m-%d %H:%M:%S.%f"))


Expand Down
34 changes: 29 additions & 5 deletions superset/connectors/sqla/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
# pylint: disable=C,R,W
"""Views used by the SqlAlchemy connector"""
import logging
import re

from flask import flash, Markup, redirect
from flask_appbuilder import CompactCRUDMixin, expose
Expand All @@ -27,6 +28,7 @@
from flask_babel import gettext as __
from flask_babel import lazy_gettext as _
from wtforms.ext.sqlalchemy.fields import QuerySelectField
from wtforms.validators import Regexp

from superset import appbuilder, db, security_manager
from superset.connectors.base.views import DatasourceModelView
Expand Down Expand Up @@ -99,12 +101,17 @@ class TableColumnInlineView(CompactCRUDMixin, SupersetModelView): # noqa
),
"python_date_format": utils.markdown(
Markup(
"The pattern of timestamp format, use "
"The pattern of timestamp format. For strings use "
'<a href="https://docs.python.org/2/library/'
'datetime.html#strftime-strptime-behavior">'
"python datetime string pattern</a> "
"expression. If time is stored in epoch "
"format, put `epoch_s` or `epoch_ms`."
"python datetime string pattern</a> expression which needs to "
'adhere to the <a href="https://en.wikipedia.org/wiki/ISO_8601">'
"ISO 8601</a> standard to ensure that the lexicographical ordering "
"coincides with the chronological ordering. If the timestamp "
"format does not adhere to the ISO 8601 standard you will need to "
"define an expression and type for transforming the string into a "
"date or timestamp. Note currently time zones are not supported. "
"If time is stored in epoch format, put `epoch_s` or `epoch_ms`."
),
True,
),
Expand All @@ -121,6 +128,24 @@ class TableColumnInlineView(CompactCRUDMixin, SupersetModelView): # noqa
"python_date_format": _("Datetime Format"),
"type": _("Type"),
}
validators_columns = {
"python_date_format": [
# Restrict viable values to epoch_s, epoch_ms, or a strftime format
# which adhere's to the ISO 8601 format (without time zone).
Regexp(
Copy link
Member Author

Choose a reason for hiding this comment

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

@dpgaspar this FAB validator works but it doesn't seem to show the message. I've successfully used validators in Superset on other PRs and I was wondering whether there's a FAB issue displaying messages when the form field isn't on the primary tab.

re.compile(
r"""
^(
epoch_s|epoch_ms|
(?P<date>%Y(-%m(-%d)?)?)([\sT](?P<time>%H(:%M(:%S(\.%f)?)?)?))?
)$
""",
re.VERBOSE,
),
message=_("Invalid date/timestamp format"),
)
]
}

add_form_extra_fields = {
"table": QuerySelectField(
Expand Down Expand Up @@ -303,7 +328,6 @@ class TableModelView(DatasourceModelView, DeleteMixin, YamlExportMixin): # noqa
"template_params": _("Template parameters"),
"modified": _("Modified"),
}

edit_form_extra_fields = {
"database": QuerySelectField(
"Database",
Expand Down