mirror of
https://github.com/johannesjo/super-productivity.git
synced 2026-07-17 16:37:43 +00:00
* fix(gitlab): avoid request storm on connection test and search (#9034) searchIssueInProject$ paginated through every matching issue and then fired a paginated /notes request per issue via forkJoin. With an empty search term (used by testConnection) or a broad one, a project with thousands of issues produced thousands of rapid requests, which GitLab rate-limited with a 429 and surfaced as "connection failed". The fetched comments were never used: search results only need id + title, the comment count already rides along via user_notes_count, and full notes are re-fetched fresh in the detail view via getById$. Fetch only the first page for search (results are already ordered by updated_at) and drop the per-issue comment fan-out. This turns the connection test and each search from thousands of requests into one. Add gitlab-api.service.spec.ts asserting search issues a single first-page request, never follows x-next-page, and never requests notes. * fix(gitlab): make project pattern valid as a native input attribute (#9034) Formly writes templateOptions.pattern verbatim to the native <input pattern> attribute, and Chromium 146 compiles that attribute with the RegExp `v` flag. The field passed a RegExp object, so its `/…/i` stringification became the attribute value, and the unescaped `/` and `-` in the `[\w.%/-]` character class are reserved under `v` — Chromium logged "Invalid regular expression: Invalid character in character class" on every change-detection cycle (hundreds of times during the request storm from the connection test). Pass the regex `.source` string instead, escape `/` and `-` in the class (`[\w.%\/\-]`), and make the pattern flag-independent by writing the only case-sensitive literal as `%2[Ff]` so dropping the `i` flag preserves the lowercase `%2f` separator. Matching behaviour is unchanged (verified against all existing valid/invalid cases). Angular's Validators.pattern still enforces the same rule via the source string. Add a regression test that the source compiles under the `v` flag.
This commit is contained in:
parent
30139c7f88
commit
0cd069fc74
4 changed files with 200 additions and 28 deletions
|
|
@ -0,0 +1,140 @@
|
|||
import {
|
||||
HttpClientTestingModule,
|
||||
HttpTestingController,
|
||||
} from '@angular/common/http/testing';
|
||||
import { TestBed } from '@angular/core/testing';
|
||||
import { SnackService } from 'src/app/core/snack/snack.service';
|
||||
import { GitlabApiService } from './gitlab-api.service';
|
||||
import { GitlabCfg } from '../gitlab.model';
|
||||
import { DEFAULT_GITLAB_CFG } from '../gitlab.const';
|
||||
import { GitlabOriginalIssue, GitlabOriginalUser } from './gitlab-api-responses';
|
||||
import { SearchResultItem } from '../../../issue.model';
|
||||
|
||||
const USER: GitlabOriginalUser = {
|
||||
id: 1,
|
||||
username: 'u',
|
||||
name: 'U',
|
||||
state: 'active',
|
||||
avatar_url: '',
|
||||
web_url: '',
|
||||
};
|
||||
|
||||
const makeOriginalIssue = (iid: number): GitlabOriginalIssue => ({
|
||||
id: 1000 + iid,
|
||||
iid,
|
||||
project_id: 1,
|
||||
title: `Issue ${iid}`,
|
||||
description: 'desc',
|
||||
state: 'open',
|
||||
weight: 0,
|
||||
created_at: '2026-01-01T00:00:00.000Z',
|
||||
updated_at: '2026-01-02T00:00:00.000Z',
|
||||
closed_at: '',
|
||||
closed_by: '',
|
||||
labels: [],
|
||||
milestone: null,
|
||||
assignees: [],
|
||||
author: USER,
|
||||
assignee: USER,
|
||||
user_notes_count: 7,
|
||||
merge_requests_count: 0,
|
||||
upvotes: 0,
|
||||
downvotes: 0,
|
||||
due_date: '',
|
||||
confidential: false,
|
||||
discussion_locked: false,
|
||||
web_url: `https://gitlab.com/group/sub/proj/-/issues/${iid}`,
|
||||
time_stats: {
|
||||
time_estimate: 0,
|
||||
total_time_spent: 0,
|
||||
human_time_estimate: '',
|
||||
human_total_time_spent: '',
|
||||
},
|
||||
task_completion_status: { count: 0, completed_count: 0 },
|
||||
has_tasks: false,
|
||||
task_status: '',
|
||||
_links: {
|
||||
self: `https://gitlab.com/api/v4/projects/1/issues/${iid}`,
|
||||
notes: `https://gitlab.com/api/v4/projects/1/issues/${iid}/notes`,
|
||||
award_emoji: '',
|
||||
project: '',
|
||||
},
|
||||
references: {
|
||||
short: `#${iid}`,
|
||||
relative: `#${iid}`,
|
||||
full: `group/sub/proj#${iid}`,
|
||||
},
|
||||
moved_to_id: 0,
|
||||
});
|
||||
|
||||
describe('GitlabApiService', () => {
|
||||
let service: GitlabApiService;
|
||||
let httpMock: HttpTestingController;
|
||||
|
||||
const cfg: GitlabCfg = {
|
||||
...DEFAULT_GITLAB_CFG,
|
||||
isEnabled: true,
|
||||
project: 'group/sub/proj',
|
||||
token: 'token',
|
||||
scope: 'all',
|
||||
};
|
||||
|
||||
beforeEach(() => {
|
||||
TestBed.configureTestingModule({
|
||||
imports: [HttpClientTestingModule],
|
||||
providers: [
|
||||
GitlabApiService,
|
||||
{
|
||||
provide: SnackService,
|
||||
useValue: jasmine.createSpyObj('SnackService', ['open']),
|
||||
},
|
||||
],
|
||||
});
|
||||
service = TestBed.inject(GitlabApiService);
|
||||
httpMock = TestBed.inject(HttpTestingController);
|
||||
});
|
||||
|
||||
afterEach(() => httpMock.verify());
|
||||
|
||||
describe('searchIssueInProject$', () => {
|
||||
it('fetches only the first page and never requests per-issue notes/comments (#9034)', () => {
|
||||
let result: SearchResultItem[] | undefined;
|
||||
service.searchIssueInProject$('bug', cfg).subscribe((r) => (result = r));
|
||||
|
||||
const reqs = httpMock.match(() => true);
|
||||
expect(reqs.length).toBe(1);
|
||||
const req = reqs[0].request;
|
||||
expect(req.method).toBe('GET');
|
||||
expect(req.url).toContain('/issues');
|
||||
expect(req.url).toContain('page=1');
|
||||
expect(req.url).toContain('per_page=100');
|
||||
expect(req.url).not.toContain('/notes');
|
||||
|
||||
// Respond WITH an x-next-page header — the service must NOT follow it, otherwise a
|
||||
// large project would trigger the request storm this fix removes.
|
||||
reqs[0].flush([makeOriginalIssue(1), makeOriginalIssue(2)], {
|
||||
// eslint-disable-next-line @typescript-eslint/naming-convention
|
||||
headers: { 'x-next-page': '2' },
|
||||
});
|
||||
|
||||
// No follow-up page request and no per-issue /notes request.
|
||||
httpMock.expectNone(() => true);
|
||||
expect(result?.length).toBe(2);
|
||||
expect(result?.[0].title).toBe('#group/sub/proj#1 Issue 1');
|
||||
expect(result?.[0].issueType).toBe('GITLAB');
|
||||
});
|
||||
|
||||
it('resolves to [] and sends no request when settings are invalid', () => {
|
||||
let result: SearchResultItem[] | undefined;
|
||||
let completed = false;
|
||||
service
|
||||
.searchIssueInProject$('bug', { ...cfg, project: null })
|
||||
.subscribe({ next: (r) => (result = r), complete: () => (completed = true) });
|
||||
|
||||
httpMock.expectNone(() => true);
|
||||
// EMPTY completes without emitting a value.
|
||||
expect(result).toBeUndefined();
|
||||
expect(completed).toBe(true);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
@ -7,7 +7,7 @@ import {
|
|||
HttpRequest,
|
||||
} from '@angular/common/http';
|
||||
import { parseUrl, stringifyUrl } from 'query-string';
|
||||
import { EMPTY, forkJoin, Observable, of } from 'rxjs';
|
||||
import { EMPTY, Observable } from 'rxjs';
|
||||
import { SnackService } from 'src/app/core/snack/snack.service';
|
||||
|
||||
import { GitlabCfg } from '../gitlab.model';
|
||||
|
|
@ -118,27 +118,20 @@ export class GitlabApiService {
|
|||
if (!this._isValidSettings(cfg)) {
|
||||
return EMPTY;
|
||||
}
|
||||
return this._sendIssuePaginatedRequest$(
|
||||
// NOTE: only the first page is fetched and issue comments are intentionally NOT
|
||||
// loaded here. The search dropdown only needs id + title (the comment count already
|
||||
// rides along via user_notes_count, and full notes are re-fetched in the detail view
|
||||
// via getById$). A broad or empty search term can match thousands of issues, so
|
||||
// paginating through all of them and then firing a /notes request per issue produced a
|
||||
// burst of thousands of requests that GitLab rate-limited with a 429 — see #9034.
|
||||
return this._sendIssueRequestFirstPage$(
|
||||
{
|
||||
url: `${this._apiLink(cfg)}/issues?search=${searchText}${this.getScopeParam(
|
||||
cfg,
|
||||
)}&order_by=updated_at${this.getCustomFilterParam(cfg)}`,
|
||||
},
|
||||
cfg,
|
||||
).pipe(
|
||||
mergeMap((issues: GitlabIssue[]) => {
|
||||
if (issues && issues.length) {
|
||||
return forkJoin([
|
||||
...issues.map((issue) => this.getIssueWithComments$(issue, cfg)),
|
||||
]);
|
||||
} else {
|
||||
return of([]);
|
||||
}
|
||||
}),
|
||||
map((issues: GitlabIssue[]) => {
|
||||
return issues ? issues.map(mapGitlabIssueToSearchResult) : [];
|
||||
}),
|
||||
);
|
||||
).pipe(map((issues) => issues.map(mapGitlabIssueToSearchResult)));
|
||||
}
|
||||
|
||||
getProjectIssues$(cfg: GitlabCfg): Observable<GitlabIssue[]> {
|
||||
|
|
@ -241,6 +234,21 @@ export class GitlabApiService {
|
|||
);
|
||||
}
|
||||
|
||||
// Fetches only the first page of issues (no x-next-page follow-up) and maps the raw
|
||||
// response body straight to GitlabIssue[]. Used for search, where fetching every page is
|
||||
// both unnecessary and a rate-limit risk on large projects (see searchIssueInProject$).
|
||||
private _sendIssueRequestFirstPage$(
|
||||
params: HttpRequest<string> | any,
|
||||
cfg: GitlabCfg,
|
||||
): Observable<GitlabIssue[]> {
|
||||
return this._sendPaginatedRequestImpl$(params, cfg, 1).pipe(
|
||||
map((res: any) => {
|
||||
const issues: GitlabOriginalIssue[] = res && res.body ? res.body : [];
|
||||
return issues.map((issue) => mapGitlabIssue(issue, cfg));
|
||||
}),
|
||||
);
|
||||
}
|
||||
|
||||
private _sendPaginatedRequest$(
|
||||
params: HttpRequest<string> | any,
|
||||
cfg: GitlabCfg,
|
||||
|
|
|
|||
|
|
@ -4,22 +4,34 @@ import {
|
|||
} from './gitlab-cfg-form.const';
|
||||
|
||||
describe('GITLAB_PROJECT_REGEX', () => {
|
||||
let projectPattern: RegExp;
|
||||
|
||||
beforeAll(() => {
|
||||
// Verify the form field is actually wired to the exported regex, so this
|
||||
// spec fails if someone swaps the field's `pattern` out from under it.
|
||||
// The form field is wired to the regex SOURCE STRING (not the RegExp object),
|
||||
// because Formly writes it verbatim to the native `<input pattern>` attribute.
|
||||
// Fail here if someone swaps the field's `pattern` out from under it.
|
||||
const projectField = GITLAB_CONFIG_FORM_SECTION.items!.find(
|
||||
(item) => item.key === 'project',
|
||||
);
|
||||
const pattern = projectField?.templateOptions?.pattern as RegExp;
|
||||
expect(pattern).toBe(GITLAB_PROJECT_REGEX);
|
||||
projectPattern = pattern;
|
||||
const pattern = projectField?.templateOptions?.pattern;
|
||||
expect(pattern).toBe(GITLAB_PROJECT_REGEX.source);
|
||||
});
|
||||
|
||||
// Angular's Validators.pattern uses `regex.test(value)` as-is for a RegExp
|
||||
// (no auto-anchoring), so the regex itself must be anchored at both ends.
|
||||
const isValid = (value: string): boolean => projectPattern.test(value);
|
||||
// Angular's Validators.pattern with a string wraps it in `^(?:…)$`; the source is
|
||||
// already anchored, so it is used as-is. Testing the exported RegExp is equivalent.
|
||||
const isValid = (value: string): boolean => GITLAB_PROJECT_REGEX.test(value);
|
||||
|
||||
// Regression guard for #9034: the source is written to the native `<input pattern>`
|
||||
// attribute, which Chromium compiles with the RegExp `v` flag. A value that is not
|
||||
// `v`-safe throws "Invalid regular expression" on every change-detection cycle.
|
||||
it('compiles as a native pattern attribute (RegExp `v` flag)', () => {
|
||||
expect(() => new RegExp(`^(?:${GITLAB_PROJECT_REGEX.source})$`, 'v')).not.toThrow();
|
||||
});
|
||||
|
||||
it('is flag-independent so it needs no `i` flag on the attribute', () => {
|
||||
expect(GITLAB_PROJECT_REGEX.flags).toBe('');
|
||||
// The only case-sensitive literal is the encoded separator; both cases accepted.
|
||||
expect(isValid('group%2Fproject')).toBe(true);
|
||||
expect(isValid('group%2fproject')).toBe(true);
|
||||
});
|
||||
|
||||
describe('valid project identifiers', () => {
|
||||
const validCases = [
|
||||
|
|
|
|||
|
|
@ -16,7 +16,16 @@ import {
|
|||
// about the segment chars (e.g. consecutive hyphens, which GitLab paths allow) to
|
||||
// avoid false-rejecting valid paths; the separator lookahead keeps the char class a
|
||||
// single unnested quantifier (no catastrophic backtracking).
|
||||
export const GITLAB_PROJECT_REGEX = /^(?:[1-9][0-9]*|(?=.*(?:\/|%2F))[\w.%/-]+)$/i;
|
||||
//
|
||||
// This pattern's `.source` is handed to Formly's `pattern` option, which both feeds
|
||||
// Angular's `Validators.pattern` AND is written verbatim to the native `<input pattern>`
|
||||
// attribute. Chromium compiles that attribute with the RegExp `v` flag, under which `/`
|
||||
// and `-` are reserved inside a character class and MUST be escaped — hence `[\w.%\/\-]`.
|
||||
// It must also stay flag-independent (the attribute cannot carry an `i` flag, and a
|
||||
// stringified `/…/i` RegExp is not a valid attribute value), so the only case-sensitive
|
||||
// literal, the encoded separator, is written explicitly as `%2[Ff]`. Getting this wrong
|
||||
// makes Chromium log "Invalid regular expression" on every change-detection cycle (#9034).
|
||||
export const GITLAB_PROJECT_REGEX = /^(?:[1-9][0-9]*|(?=.*(?:\/|%2[Ff]))[\w.%\/\-]+)$/;
|
||||
|
||||
export const GITLAB_CONFIG_FORM: LimitedFormlyFieldConfig<IssueProviderGitlab>[] = [
|
||||
...CROSS_ORIGIN_WARNING,
|
||||
|
|
@ -27,7 +36,10 @@ export const GITLAB_CONFIG_FORM: LimitedFormlyFieldConfig<IssueProviderGitlab>[]
|
|||
required: true,
|
||||
label: T.F.GITLAB.FORM.PROJECT,
|
||||
type: 'text',
|
||||
pattern: GITLAB_PROJECT_REGEX,
|
||||
// Pass the source string (not the RegExp object): Formly writes this straight to the
|
||||
// native `<input pattern>` attribute, and a stringified `/…/` RegExp is not a valid
|
||||
// attribute value. See GITLAB_PROJECT_REGEX for the `v`-flag constraints (#9034).
|
||||
pattern: GITLAB_PROJECT_REGEX.source,
|
||||
description: T.F.GITLAB.FORM.PROJECT_HINT,
|
||||
},
|
||||
},
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue