Skip to content

fix(discover2): Various fixes #15315

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.

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

Merged
merged 20 commits into from
Oct 31, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,142 @@
import {DEFAULT_STATS_PERIOD} from 'app/constants';
import {defined} from 'app/utils';
import moment from 'moment';

const STATS_PERIOD_PATTERN = '^\\d+[hdmsw]?$';

function validStatsPeriod(input: string) {
return !!input.match(STATS_PERIOD_PATTERN);
}

const getStatsPeriodValue = (
maybe: string | string[] | undefined | null
): string | undefined => {
if (Array.isArray(maybe)) {
if (maybe.length <= 0) {
return undefined;
}

return maybe.find(validStatsPeriod);
}

if (typeof maybe === 'string' && validStatsPeriod(maybe)) {
return maybe;
}

return undefined;
};

// We normalize potential datetime strings into the form that would be valid
// if it were to be parsed by datetime.strptime using the format %Y-%m-%dT%H:%M:%S.%f
// This format was transformed to the form that moment.js understands using
// https://gist.github.com/asafge/0b13c5066d06ae9a4446
const normalizeDateTimeString = (
input: string | undefined | null
): string | undefined => {
if (!input) {
return undefined;
}

const parsed = moment.utc(input);

if (!parsed.isValid()) {
return undefined;
}

return parsed.format('YYYY-MM-DDTHH:mm:ss.SSS');
};

const getDateTimeString = (
maybe: string | string[] | undefined | null
): string | undefined => {
if (Array.isArray(maybe)) {
if (maybe.length <= 0) {
return undefined;
}

const result = maybe.find(needle => {
return moment.utc(needle).isValid();
});

return normalizeDateTimeString(result);
}

return normalizeDateTimeString(maybe);
};

const parseUtcValue = (utc: any) => {
if (typeof utc !== 'undefined') {
return utc === true || utc === 'true' ? 'true' : 'false';
}
return undefined;
};

const getUtcValue = (maybe: string | string[] | undefined | null): string | undefined => {
if (Array.isArray(maybe)) {
if (maybe.length <= 0) {
return undefined;
}

return maybe.find(needle => {
return !!parseUtcValue(needle);
});
}

maybe = parseUtcValue(maybe);

if (typeof maybe === 'string') {
return maybe;
}

return undefined;
};

interface Params {
start?: string | string[] | undefined | null;
end?: string | string[] | undefined | null;
period?: string | string[] | undefined | null;
statsPeriod?: string | string[] | undefined | null;
utc?: string | string[] | undefined | null;
[others: string]: string | string[] | undefined | null;
}

// Filters out params with null values and returns a default
// `statsPeriod` when necessary.
//
// Accepts `period` and `statsPeriod` but will only return `statsPeriod`
//
// TODO(billy): Make period parameter name consistent
export function getParams(params: Params): {[key: string]: string | string[]} {
const {start, end, period, statsPeriod, utc, ...otherParams} = params;

// `statsPeriod` takes precendence for now
let coercedPeriod = getStatsPeriodValue(statsPeriod) || getStatsPeriodValue(period);

const dateTimeStart = getDateTimeString(start);
const dateTimeEnd = getDateTimeString(end);

if (!(dateTimeStart && dateTimeEnd)) {
if (!coercedPeriod) {
coercedPeriod = DEFAULT_STATS_PERIOD;
}
}

// Filter null values
return Object.entries({
statsPeriod: coercedPeriod,
start: coercedPeriod ? null : dateTimeStart,
end: coercedPeriod ? null : dateTimeEnd,
// coerce utc into a string (it can be both: a string representation from router,
// or a boolean from time range picker)
utc: getUtcValue(utc),
...otherParams,
})
.filter(([_key, value]) => defined(value))
.reduce(
(acc, [key, value]) => ({
...acc,
[key]: value,
}),
{}
);
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,17 +2,20 @@ import {pick, pickBy, identity} from 'lodash';

import {defined} from 'app/utils';
import {getUtcToLocalDateObject} from 'app/utils/dates';
import {getParams} from 'app/components/organizations/globalSelectionHeader/getParams';

import {URL_PARAM} from 'app/constants/globalSelectionHeader';

// Parses URL query parameters for values relevant to global selection header
export function getStateFromQuery(query) {
let start = query[URL_PARAM.START] !== 'null' && query[URL_PARAM.START];
let end = query[URL_PARAM.END] !== 'null' && query[URL_PARAM.END];
const parsedParams = getParams(query);
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm seeing some issues stemming from the global selection header propagating down to discover2.

Invalid datetime selections can easily throw an error and break the app with no actual recovery flow.

I feel it's better to parse and validate the datetime selection early here; rather than doing it at the API payload generation everywhere.


let start = parsedParams.start;
let end = parsedParams.end;
let project = query[URL_PARAM.PROJECT];
let environment = query[URL_PARAM.ENVIRONMENT];
const period = query[URL_PARAM.PERIOD];
const utc = query[URL_PARAM.UTC];
const period = parsedParams.statsPeriod;
const utc = parsedParams.utc;

const hasAbsolute = !!start && !!end;

Expand Down
2 changes: 1 addition & 1 deletion src/sentry/static/sentry/app/views/events/events.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import SentryTypes from 'app/sentryTypes';
import parseLinkHeader from 'app/utils/parseLinkHeader';
import withOrganization from 'app/utils/withOrganization';

import {getParams} from './utils/getParams';
import {getParams} from 'app/components/organizations/globalSelectionHeader/getParams';
import EventsChart from './eventsChart';
import EventsTable from './eventsTable';

Expand Down
2 changes: 1 addition & 1 deletion src/sentry/static/sentry/app/views/events/index.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import PropTypes from 'prop-types';
import React from 'react';
import styled from 'react-emotion';

import {getParams} from 'app/views/events/utils/getParams';
import {getParams} from 'app/components/organizations/globalSelectionHeader/getParams';
import {t} from 'app/locale';
import BetaTag from 'app/components/betaTag';
import Feature from 'app/components/acl/feature';
Expand Down
55 changes: 0 additions & 55 deletions src/sentry/static/sentry/app/views/events/utils/getParams.tsx

This file was deleted.

49 changes: 37 additions & 12 deletions src/sentry/static/sentry/app/views/eventsV2/eventView.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {DEFAULT_PER_PAGE} from 'app/constants';
import {EventViewv1} from 'app/types';
import {SavedQuery as LegacySavedQuery} from 'app/views/discover/types';
import {SavedQuery, NewQuery} from 'app/stores/discoverSavedQueriesStore';
import {getParams} from 'app/components/organizations/globalSelectionHeader/getParams';

import {AUTOLINK_FIELDS, SPECIAL_FIELDS, FIELD_FORMATTERS} from './data';
import {
Expand Down Expand Up @@ -350,6 +351,8 @@ class EventView {
}

static fromLocation(location: Location): EventView {
const {start, end, statsPeriod} = getParams(location.query);

return new EventView({
id: decodeScalar(location.query.id),
name: decodeScalar(location.query.name),
Expand All @@ -358,9 +361,9 @@ class EventView {
tags: collectQueryStringByKey(location.query, 'tag'),
query: decodeQuery(location) || '',
project: decodeProjects(location),
start: decodeScalar(location.query.start),
end: decodeScalar(location.query.end),
statsPeriod: decodeScalar(location.query.statsPeriod),
start: decodeScalar(start),
end: decodeScalar(end),
statsPeriod: decodeScalar(statsPeriod),
environment: collectQueryStringByKey(location.query, 'environment'),
});
}
Expand Down Expand Up @@ -402,22 +405,30 @@ class EventView {
});
}

// normalize datetime selection

const {start, end, statsPeriod} = getParams({
start: saved.start,
end: saved.end,
statsPeriod: saved.range,
});

return new EventView({
fields,
id: saved.id,
name: saved.name,
query: queryStringFromSavedQuery(saved),
project: saved.projects,
start: saved.start,
end: saved.end,
start: decodeScalar(start),
end: decodeScalar(end),
statsPeriod: decodeScalar(statsPeriod),
sorts: fromSorts(saved.orderby),
tags: collectQueryStringByKey(
{
tags: (saved as SavedQuery).tags as string[],
},
'tags'
),
statsPeriod: saved.range,
environment: collectQueryStringByKey(
{
environment: (saved as SavedQuery).environment as string[],
Expand Down Expand Up @@ -845,17 +856,31 @@ class EventView {

const picked = pickRelevantLocationQueryStrings(location);

// normalize datetime selection

const normalizedTimeWindowParams = getParams({
start: this.start,
end: this.end,
period: decodeScalar(query.period),
statsPeriod: this.statsPeriod,
utc: decodeScalar(query.utc),
});

const sort = this.sorts.length > 0 ? encodeSort(this.sorts[0]) : undefined;
const fields = this.getFields();

// generate event query

const eventQuery: EventQuery & LocationQuery = Object.assign(picked, {
field: [...new Set(fields)],
sort,
per_page: DEFAULT_PER_PAGE,
query: this.getQuery(query.query),
});
const eventQuery: EventQuery & LocationQuery = Object.assign(
picked,
normalizedTimeWindowParams,
{
field: [...new Set(fields)],
sort,
per_page: DEFAULT_PER_PAGE,
query: this.getQuery(query.query),
}
);

if (!eventQuery.sort) {
delete eventQuery.sort;
Expand Down
2 changes: 1 addition & 1 deletion src/sentry/static/sentry/app/views/eventsV2/events.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import {Panel} from 'app/components/panels';
import EventsChart from 'app/views/events/eventsChart';
import getDynamicText from 'app/utils/getDynamicText';

import {getParams} from 'app/views/events/utils/getParams';
import {getParams} from 'app/components/organizations/globalSelectionHeader/getParams';

import Table from './table';
import Tags from './tags';
Expand Down
10 changes: 6 additions & 4 deletions src/sentry/static/sentry/app/views/eventsV2/table/tableView.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ class TableView extends React.Component<TableViewProps> {
_updateColumn = (columnIndex: number, nextColumn: TableColumn<keyof TableDataRow>) => {
const {location, eventView, tableData} = this.props;

if (!tableData) {
if (!tableData || !tableData.meta) {
return;
}

Expand All @@ -112,7 +112,7 @@ class TableView extends React.Component<TableViewProps> {
_deleteColumn = (columnIndex: number) => {
const {location, eventView, tableData} = this.props;

if (!tableData) {
if (!tableData || !tableData.meta) {
return;
}

Expand Down Expand Up @@ -143,7 +143,7 @@ class TableView extends React.Component<TableViewProps> {
_renderGridHeaderCell = (column: TableColumn<keyof TableDataRow>): React.ReactNode => {
const {eventView, location, tableData} = this.props;

if (!tableData) {
if (!tableData || !tableData.meta) {
return column.name;
}

Expand Down Expand Up @@ -178,9 +178,11 @@ class TableView extends React.Component<TableViewProps> {
dataRow: TableDataRow
): React.ReactNode => {
const {location, organization, tableData, eventView} = this.props;
if (!tableData) {

if (!tableData || !tableData.meta) {
return dataRow[column.key];
}

const hasLinkField = eventView.hasAutolinkField();
const forceLink =
!hasLinkField && eventView.getFields().indexOf(String(column.field)) === 0;
Expand Down
Loading