Skip to content

fix(issues): Escape status tag on search #89434

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 2 commits into from
Apr 14, 2025
Merged
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
11 changes: 9 additions & 2 deletions static/app/components/events/eventTags/eventTagsTreeRow.tsx
Original file line number Diff line number Diff line change
@@ -17,7 +17,7 @@ import {t} from 'sentry/locale';
import {space} from 'sentry/styles/space';
import type {Event} from 'sentry/types/event';
import type {Project} from 'sentry/types/project';
import {generateQueryWithTag} from 'sentry/utils';
import {escapeIssueTagKey, generateQueryWithTag} from 'sentry/utils';
import {isEmptyObject} from 'sentry/utils/object/isEmptyObject';
import {isUrl} from 'sentry/utils/string/isUrl';
import useCopyToClipboard from 'sentry/utils/useCopyToClipboard';
@@ -150,7 +150,14 @@ function EventTagsTreeRowDropdown({
project?.highlightTags &&
// Skip tags already highlighted
highlightTagSet.has(originalTag.key);
const query = generateQueryWithTag({referrer}, originalTag);
const query = generateQueryWithTag(
{referrer},
{
...originalTag,
key: escapeIssueTagKey(originalTag.key),
}
);

const isProjectAdmin = hasEveryAccess(['project:admin'], {
organization,
project,
13 changes: 13 additions & 0 deletions static/app/utils.spec.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import {escapeIssueTagKey} from './utils';

describe('escapeIssueTagKey', () => {
it('should escape conflicting tag keys', () => {
expect(escapeIssueTagKey('status')).toBe('tags[status]');
expect(escapeIssueTagKey('message')).toBe('tags[message]');
});

it('should not escape environment and project', () => {
expect(escapeIssueTagKey('environment')).toBe('environment');
expect(escapeIssueTagKey('project')).toBe('project');
});
});
20 changes: 20 additions & 0 deletions static/app/utils.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
import type {Query} from 'history';

import type {EventTag} from 'sentry/types/event';
import {
type FieldKey,
ISSUE_EVENT_FIELDS_THAT_MAY_CONFLICT_WITH_TAGS,
} from 'sentry/utils/fields';
import {appendTagCondition} from 'sentry/utils/queryString';

export function intcomma(x: number): string {
@@ -92,6 +96,22 @@ export function isWebpackChunkLoadingError(error: Error): boolean {
);
}

/**
* If a tag conflicts with a reserved keyword, change it to `tags[key]:value`
*/
export function escapeIssueTagKey(key: string) {
Copy link
Member

Choose a reason for hiding this comment

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

We are doing this same thing in useFetchIssueTags.tsx, can you use this function there as well?

// Environment and project should be handled by the page filter
if (key === 'environment' || key === 'project') {
return key;
}

if (ISSUE_EVENT_FIELDS_THAT_MAY_CONFLICT_WITH_TAGS.has(key as FieldKey)) {
return `tags[${key}]`;
}

return key;
}

export function generateQueryWithTag(prevQuery: Query, tag: EventTag): Query {
const query = {...prevQuery};

7 changes: 7 additions & 0 deletions static/app/utils/fields/index.ts
Original file line number Diff line number Diff line change
@@ -106,6 +106,7 @@ export enum FieldKey {
STACK_PACKAGE = 'stack.package',
STACK_RESOURCE = 'stack.resource',
STACK_STACK_LEVEL = 'stack.stack_level',
STATUS = 'status',
TIMESTAMP = 'timestamp',
TIMESTAMP_TO_DAY = 'timestamp.to_day',
TIMESTAMP_TO_HOUR = 'timestamp.to_hour',
@@ -1703,6 +1704,11 @@ const EVENT_FIELD_DEFINITIONS: Record<AllEventFieldKeys, FieldDefinition> = {
kind: FieldKind.FIELD,
valueType: FieldValueType.NUMBER,
},
[FieldKey.STATUS]: {
desc: t('Status of the issue'),
kind: FieldKind.FIELD,
valueType: FieldValueType.STRING,
},
[FieldKey.TIMES_SEEN]: {
desc: t('Total number of events'),
kind: FieldKind.FIELD,
@@ -1993,6 +1999,7 @@ export const ISSUE_EVENT_FIELDS_THAT_MAY_CONFLICT_WITH_TAGS: Set<FieldKey> = new
FieldKey.STACK_MODULE,
FieldKey.STACK_PACKAGE,
FieldKey.STACK_STACK_LEVEL,
FieldKey.STATUS,
Copy link
Member Author

Choose a reason for hiding this comment

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

@malwilley i wasn't totally sure about this change and how these are used already

Copy link
Member

Choose a reason for hiding this comment

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

Yeah this is exactly what this was set was meant to solve so it belongs here

FieldKey.TIMESTAMP,
FieldKey.TITLE,
FieldKey.TRACE,
Original file line number Diff line number Diff line change
@@ -18,7 +18,7 @@ import {IconArrow, IconEllipsis, IconOpen} from 'sentry/icons';
import {t, tct} from 'sentry/locale';
import {space} from 'sentry/styles/space';
import type {Group, Tag, TagValue} from 'sentry/types/group';
import {percent} from 'sentry/utils';
import {escapeIssueTagKey, generateQueryWithTag, percent} from 'sentry/utils';
import {SavedQueryDatasets} from 'sentry/utils/discover/types';
import {isUrl} from 'sentry/utils/string/isUrl';
import useCopyToClipboard from 'sentry/utils/useCopyToClipboard';
@@ -251,8 +251,13 @@ function TagValueActionsMenu({
const {onClick: handleCopy} = useCopyToClipboard({
text: tagValue.value,
});
const key = tagValue.key ?? tag.key;
const query = {query: tagValue.query || `${key}:"${tagValue.value}"`};
const referrer = 'tag-details-drawer';
const key = escapeIssueTagKey(tagValue.key ?? tag.key);
const query = tagValue.query
? {
query: tagValue.query,
}
: generateQueryWithTag({referrer}, {key, value: tagValue.value});
const eventView = useIssueDetailsEventView({group, queryProps: query});
const [isVisible, setIsVisible] = useState(false);

10 changes: 5 additions & 5 deletions static/app/views/issueList/utils/useFetchIssueTags.tsx
Original file line number Diff line number Diff line change
@@ -21,12 +21,12 @@ import {
} from 'sentry/types/group';
import type {Organization} from 'sentry/types/organization';
import type {User} from 'sentry/types/user';
import {escapeIssueTagKey} from 'sentry/utils';
import {SEMVER_TAGS} from 'sentry/utils/discover/fields';
import {
FieldKey,
FieldKind,
IsFieldValues,
ISSUE_EVENT_FIELDS_THAT_MAY_CONFLICT_WITH_TAGS,
ISSUE_EVENT_PROPERTY_FIELDS,
ISSUE_FIELDS,
ISSUE_PROPERTY_FIELDS,
@@ -68,15 +68,15 @@ const renameConflictingTags = (tags: TagCollection): TagCollection => {
const renamedTags: TagCollection = {};

for (const [key, tag] of Object.entries(tags)) {
if (ISSUE_EVENT_FIELDS_THAT_MAY_CONFLICT_WITH_TAGS.has(key as FieldKey)) {
const newKey = `tags[${key}]`;
const newKey = escapeIssueTagKey(key);
if (key === newKey) {
renamedTags[key] = tag;
} else {
renamedTags[newKey] = {
...tag,
key: newKey,
name: newKey,
};
} else {
renamedTags[key] = tag;
}
}


Unchanged files with check annotations Beta

SpanIndexedResponse,
} from 'sentry/views/insights/types';
import {useInsightsEap} from '../../common/utils/useEap';

Check warning on line 16 in static/app/views/insights/http/queries/useSpanSamples.tsx

GitHub Actions / eslint

import statements should have an absolute path
interface UseSpanSamplesOptions<Fields> {
enabled?: boolean;
isMissingInstrumentationNode,
isSpanNode,
isTraceErrorNode,
isTraceNode,

Check warning on line 15 in static/app/views/performance/newTraceDetails/traceModels/traceTreeTestUtils.tsx

GitHub Actions / eslint

import statements should have an absolute path
isTransactionNode,
} from 'sentry/views/performance/newTraceDetails/traceGuards';