Skip to content

Commit 88a55a3

Browse files
authored
feat(merge_requests): preserve suggestion metadata (#583)
* feat(merge_requests): preserve suggestion metadata Keep exact original and replacement content, sparse application state and documented applicability spellings while validating identities before generated numeric parsing. Signed-off-by: sjungwon03 <sjungwon03@gmail.com> * feat(merge_requests): preserve suggestion metadata Keep original suggestion content and explicit unknown states available for future review controls without inferring permissions. Signed-off-by: sjungwon03 <sjungwon03@gmail.com> * docs(parity): link suggestion metadata PR Link the validated suggestion metadata foundation and its remaining application scope. Signed-off-by: sjungwon03 <sjungwon03@gmail.com> --------- Signed-off-by: sjungwon03 <sjungwon03@gmail.com>
1 parent 983bdf9 commit 88a55a3

12 files changed

Lines changed: 1119 additions & 4 deletions

‎docs/mobile-web-parity.md‎

Lines changed: 1 addition & 1 deletion
Large diffs are not rendered by default.

‎docs/mr-suggestion-metadata.md‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
# MR suggestion metadata
2+
3+
`Note.suggestions` preserves the server-provided suggestion collection. The generated Freezed `Suggestion` DTO retains its global suggestion ID, original/replacement content, original inclusive line range, applicability fields and applied status. IDs do not identify notes, MR IIDs or diff versions.
4+
5+
## Omission and applicability
6+
7+
An omitted/null suggestion collection remains unknown; an explicit empty list stays empty. Missing legacy coordinates, content and boolean state remain null. Empty original/replacement strings remain present: they can represent insertion or deletion. Original whitespace and newlines are never normalized. Nested note/discussion collections are immutable through their exposed DTO lists.
8+
9+
The official Discussions API example uses `appliable`, while the Suggest Changes API documents `applicable`. Preserve both independently. `patchApplicable` reads the known spelling or their agreed value; a direct model with conflicting values yields unknown. This describes patch applicability only, not current-user permission, and is independent of `applied`. Future UI must verify the current loaded state and let GitLab enforce write permission.
10+
11+
## MR API boundary validation
12+
13+
Discussion reads, reply responses, resolution responses and positioned creation validate suggestion collections before generated deserialization. IDs must be positive integral numbers and unique throughout each decoded response. Provided line coordinates must be positive integers; a complete range cannot be reversed. Provided content must be strings, and provided status flags must be booleans. Conflicting applicability spellings are a malformed response, never a choice of whichever enables an action.
14+
15+
HTTP status mapping precedes parsing. Malformed suggestion metadata becomes the existing sanitized domain error for that response context; source text, identifiers and parsing details are not exposed. Numeric validation avoids generated `toInt()` truncation directing a future action at another suggestion or line. Sparse valid metadata remains available as unknown fields instead of fabricated defaults.
16+
17+
This slice adds no suggestion mutation endpoint, preview control, apply button or automatic repository change. It supplies the read foundation for those later slices. Multiline context/creation and suggestion presentation/application remain separate work. Tests use documented synthetic payloads and dummy credentials; no live GitLab/device validation is claimed.
18+
19+
References: [Discussions API](https://docs.gitlab.com/api/discussions/#list-merge-request-discussion-items) and [Suggest Changes API](https://docs.gitlab.com/api/suggestions/).

‎packages/gitlab_api/lib/src/merge_requests/merge_requests_api.dart‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -774,6 +774,7 @@ class MergeRequestsApi {
774774
if (userId is! int || userId <= 0) throw const FormatException();
775775
}
776776
_validateDiffPosition(payload['position']);
777+
_validateSuggestions(payload['suggestions']);
777778
return Note.fromJson(payload);
778779
} on FormatException {
779780
throw const GitLabServerException('Invalid discussion reply response.');
@@ -842,9 +843,45 @@ class MergeRequestsApi {
842843
}
843844
}
844845

846+
// Validate before generated numeric parsing so suggestion identities and
847+
// line coordinates cannot be truncated or confused with another patch.
848+
static void _validateSuggestions(Object? value, {Set<int>? seenIds}) {
849+
if (value == null) return;
850+
if (value is! List) throw const FormatException();
851+
final ids = seenIds ?? <int>{};
852+
for (final entry in value) {
853+
if (entry is! Map<String, dynamic>) throw const FormatException();
854+
final id = entry['id'];
855+
if (id is! int || id < 1 || !ids.add(id)) throw const FormatException();
856+
for (final key in ['from_line', 'to_line']) {
857+
final line = entry[key];
858+
if (line != null && (line is! int || line < 1)) {
859+
throw const FormatException();
860+
}
861+
}
862+
final from = entry['from_line'], to = entry['to_line'];
863+
if (from is int && to is int && to < from) throw const FormatException();
864+
for (final key in ['from_content', 'to_content']) {
865+
final content = entry[key];
866+
if (content != null && content is! String) {
867+
throw const FormatException();
868+
}
869+
}
870+
for (final key in ['appliable', 'applicable', 'applied']) {
871+
final flag = entry[key];
872+
if (flag != null && flag is! bool) throw const FormatException();
873+
}
874+
final legacy = entry['appliable'], documented = entry['applicable'];
875+
if (legacy != null && documented != null && legacy != documented) {
876+
throw const FormatException();
877+
}
878+
}
879+
}
880+
845881
static List<Discussion> _parseDiscussions(Object? payload) {
846882
try {
847883
if (payload is! List) throw const FormatException();
884+
final suggestionIds = <int>{};
848885
return payload
849886
.map((entry) {
850887
if (entry is! Map<String, dynamic>) throw const FormatException();
@@ -855,6 +892,7 @@ class MergeRequestsApi {
855892
for (final note in notes) {
856893
if (note is! Map<String, dynamic>) throw const FormatException();
857894
_validateDiffPosition(note['position']);
895+
_validateSuggestions(note['suggestions'], seenIds: suggestionIds);
858896
final noteId = note['id'];
859897
if (noteId is! int || noteId <= 0) throw const FormatException();
860898
for (final key in ['author', 'resolved_by']) {
Lines changed: 299 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,299 @@
1+
import 'dart:convert';
2+
import 'dart:typed_data';
3+
import 'package:dio/dio.dart';
4+
import 'package:gitlab_api/gitlab_api.dart';
5+
import 'package:gitlab_models/gitlab_models.dart';
6+
import 'package:test/test.dart';
7+
8+
const _position = DiffNotePosition(
9+
baseSha: 'base',
10+
startSha: 'start',
11+
headSha: 'head',
12+
oldPath: 'a.dart',
13+
newPath: 'a.dart',
14+
positionType: 'text',
15+
oldLine: 27,
16+
newLine: 27,
17+
);
18+
const _suggestion = <String, Object?>{
19+
'id': 5,
20+
'from_line': 27,
21+
'to_line': 28,
22+
'appliable': true,
23+
'applied': false,
24+
'from_content': ' original\n',
25+
'to_content': ' replacement\n',
26+
};
27+
28+
enum _Operation { read, reply, resolve, create }
29+
30+
Map<String, Object?> _note(Object? suggestions, {bool omit = false}) => {
31+
'id': 301,
32+
'body': 'Review suggestion',
33+
'type': 'DiffNote',
34+
'system': false,
35+
'position': _position.toJson(),
36+
'resolvable': true,
37+
'resolved': true,
38+
if (!omit) 'suggestions': suggestions,
39+
};
40+
Map<String, Object?> _thread(Map<String, Object?> note) => {
41+
'id': 'thread',
42+
'individual_note': false,
43+
'notes': [note],
44+
};
45+
Future<Note> _read(GitLabClient c, _Operation op) async => switch (op) {
46+
_Operation.read => (await c.mergeRequests.discussions(
47+
8,
48+
iid: 142,
49+
)).items.single.notes.single,
50+
_Operation.reply => await c.mergeRequests.replyToDiscussion(
51+
8,
52+
iid: 142,
53+
discussionId: 'thread',
54+
body: 'Reply',
55+
),
56+
_Operation.resolve => (await c.mergeRequests.setDiscussionResolved(
57+
8,
58+
iid: 142,
59+
discussionId: 'thread',
60+
resolved: true,
61+
)).notes.single,
62+
_Operation.create => (await c.mergeRequests.createPositionedDiscussion(
63+
8,
64+
iid: 142,
65+
body: 'Review',
66+
position: _position,
67+
)).notes.single,
68+
};
69+
Object _payload(_Operation op, Map<String, Object?> note) => switch (op) {
70+
_Operation.read => [_thread(note)],
71+
_Operation.reply => note,
72+
_ => _thread(note),
73+
};
74+
void main() {
75+
for (final op in _Operation.values) {
76+
for (final shape in [
77+
'omitted',
78+
'null',
79+
'empty',
80+
'legacy',
81+
'documented',
82+
'same aliases',
83+
'sparse',
84+
'two',
85+
]) {
86+
test('$op preserves suggestion metadata $shape', () async {
87+
final value = switch (shape) {
88+
'omitted' || 'null' => null,
89+
'empty' => <Object?>[],
90+
'legacy' => [_suggestion],
91+
'documented' => [
92+
{..._suggestion, 'appliable': null, 'applicable': true},
93+
],
94+
'same aliases' => [
95+
{..._suggestion, 'applicable': true},
96+
],
97+
'sparse' => [
98+
{'id': 5},
99+
],
100+
'two' => [
101+
_suggestion,
102+
{..._suggestion, 'id': 6, 'to_content': ''},
103+
],
104+
_ => throw StateError('fixture'),
105+
};
106+
final note = _note(value, omit: shape == 'omitted');
107+
final client = _client(
108+
(_) => (
109+
status: op == _Operation.reply || op == _Operation.create
110+
? 201
111+
: 200,
112+
body: _payload(op, note),
113+
),
114+
);
115+
addTearDown(client.close);
116+
final result = await _read(client, op);
117+
if (shape == 'omitted' || shape == 'null') {
118+
expect(result.suggestions, isNull);
119+
} else if (shape == 'empty') {
120+
expect(result.suggestions, isEmpty);
121+
} else {
122+
final item = result.suggestions!.first;
123+
expect(item.id, 5);
124+
if (shape == 'sparse') {
125+
expect(item.fromLine, isNull);
126+
expect(item.toContent, isNull);
127+
expect(item.patchApplicable, isNull);
128+
expect(item.applied, isNull);
129+
} else {
130+
expect(item.fromLine, 27);
131+
expect(item.toLine, 28);
132+
expect(item.fromContent, ' original\n');
133+
expect(item.toContent, ' replacement\n');
134+
expect(item.patchApplicable, true);
135+
expect(item.applied, false);
136+
}
137+
expect(() => result.suggestions!.add(item), throwsUnsupportedError);
138+
if (shape == 'two') {
139+
expect(result.suggestions![1].id, 6);
140+
expect(result.suggestions![1].toContent, '');
141+
}
142+
}
143+
});
144+
}
145+
final bad = <String, Object?>{
146+
'scalar': 'private-content-marker',
147+
'map': _suggestion,
148+
'invalid entry': [null],
149+
'list entry': [[]],
150+
for (final id in [null, 0, -1, 5.5, '5'])
151+
'ID $id': [
152+
{..._suggestion, 'id': id},
153+
],
154+
for (final key in ['from_line', 'to_line'])
155+
for (final line in [0, -1, 27.5, '27'])
156+
'$key $line': [
157+
{..._suggestion, key: line},
158+
],
159+
'reversed range': [
160+
{..._suggestion, 'from_line': 29},
161+
],
162+
for (final key in ['from_content', 'to_content'])
163+
'$key shape': [
164+
{..._suggestion, key: []},
165+
],
166+
for (final key in ['appliable', 'applicable', 'applied'])
167+
for (final flag in [1, 'true', []])
168+
'$key flag $flag': [
169+
{..._suggestion, key: flag},
170+
],
171+
'duplicate IDs': [_suggestion, _suggestion],
172+
'conflicting aliases': [
173+
{..._suggestion, 'applicable': false},
174+
],
175+
};
176+
for (final entry in bad.entries) {
177+
test('$op rejects malformed suggestion ${entry.key}', () async {
178+
final client = _client(
179+
(_) => (
180+
status: op == _Operation.reply || op == _Operation.create
181+
? 201
182+
: 200,
183+
body: _payload(op, _note(entry.value)),
184+
),
185+
);
186+
addTearDown(client.close);
187+
await expectLater(
188+
_read(client, op),
189+
throwsA(
190+
isA<GitLabServerException>().having(
191+
(e) => e.message,
192+
'sanitized',
193+
isNot(contains('private-content-marker')),
194+
),
195+
),
196+
);
197+
});
198+
}
199+
if (op != _Operation.reply) {
200+
test('$op rejects duplicate suggestion identity across notes', () async {
201+
final first = _note([_suggestion]);
202+
final second = {...first, 'id': 302};
203+
final group = {
204+
..._thread(first),
205+
'notes': [first, second],
206+
};
207+
final client = _client(
208+
(_) => (
209+
status: op == _Operation.create ? 201 : 200,
210+
body: op == _Operation.read ? [group] : group,
211+
),
212+
);
213+
addTearDown(client.close);
214+
await expectLater(
215+
_read(client, op),
216+
throwsA(isA<GitLabServerException>()),
217+
);
218+
});
219+
}
220+
if (op == _Operation.read) {
221+
test(
222+
'read rejects duplicate suggestion identity across groups',
223+
() async {
224+
final first = _note([_suggestion]);
225+
final second = {...first, 'id': 302};
226+
final client = _client(
227+
(_) => (
228+
status: 200,
229+
body: [
230+
_thread(first),
231+
{..._thread(second), 'id': 'other-thread'},
232+
],
233+
),
234+
);
235+
addTearDown(client.close);
236+
await expectLater(
237+
_read(client, op),
238+
throwsA(isA<GitLabServerException>()),
239+
);
240+
},
241+
);
242+
}
243+
test('$op maps status before malformed suggestion decoding', () async {
244+
final client = _client(
245+
(_) => (
246+
status: 401,
247+
body: _payload(
248+
op,
249+
_note([
250+
{'id': 5.5},
251+
]),
252+
),
253+
),
254+
);
255+
addTearDown(client.close);
256+
await expectLater(_read(client, op), throwsA(isA<GitLabAuthException>()));
257+
});
258+
}
259+
}
260+
261+
GitLabClient _client(
262+
({int status, Object? body}) Function(RequestOptions) handler, {
263+
Map<String, List<String>> headers = const {},
264+
Future<String?> Function()? onUnauthorized,
265+
}) {
266+
final dio = Dio()..httpClientAdapter = _Adapter(handler, headers);
267+
return GitLabClient(
268+
baseUrl: 'https://gitlab.example.com',
269+
token: 'glpat-xxxxxxxxxxxx',
270+
bearer: onUnauthorized != null,
271+
onUnauthorized: onUnauthorized,
272+
dio: dio,
273+
);
274+
}
275+
276+
class _Adapter implements HttpClientAdapter {
277+
_Adapter(this.handler, this.headers);
278+
final ({int status, Object? body}) Function(RequestOptions) handler;
279+
final Map<String, List<String>> headers;
280+
@override
281+
Future<ResponseBody> fetch(
282+
RequestOptions options,
283+
Stream<Uint8List>? requestStream,
284+
Future<void>? cancelFuture,
285+
) async {
286+
final r = handler(options);
287+
return ResponseBody.fromString(
288+
r.body == null ? '' : json.encode(r.body),
289+
r.status,
290+
headers: {
291+
Headers.contentTypeHeader: [Headers.jsonContentType],
292+
...headers,
293+
},
294+
);
295+
}
296+
297+
@override
298+
void close({bool force = false}) {}
299+
}

‎packages/gitlab_models/lib/gitlab_models.dart‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,7 @@ export 'src/registry_tag.dart';
4949
export 'src/repository_entry.dart';
5050
export 'src/repository_tag.dart';
5151
export 'src/snippet.dart';
52+
export 'src/suggestion.dart';
5253
export 'src/todo.dart';
5354
export 'src/unified_diff.dart';
5455
export 'src/user.dart';

0 commit comments

Comments
 (0)