Skip to content

Commit f6d4a12

Browse files
lucy66hwlucy66hw
committed
update
1 parent b83866a commit f6d4a12

6 files changed

Lines changed: 88 additions & 113 deletions

File tree

‎tools/proto-convert/src/postprocessing/BackwardCompatibleWriter.ts‎

Lines changed: 19 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -93,23 +93,13 @@ export class BackwardCompatibleWriter {
9393
}
9494
}
9595

96-
// Check for backward incompatible changes
96+
// Report backward incompatible changes (but don't block)
9797
if (this.reporter.hasIncompatibleChanges()) {
98-
const errors = this.reporter.getIncompatibleChanges();
99-
logger.error('Backward incompatible changes detected:');
100-
for (const err of errors) {
101-
logger.error(` ${err.messageName}.${err.fieldName}: ${err.existingType} → ${err.incomingType}`);
98+
const warnings = this.reporter.getIncompatibleChanges();
99+
logger.warn('Backward incompatible changes detected (will be versioned):');
100+
for (const warn of warnings) {
101+
logger.warn(` ${warn.messageName}.${warn.fieldName}: ${warn.existingType} → ${warn.incomingType}`);
102102
}
103-
104-
if (!dryRun) {
105-
// In real run mode, throw error and don't write
106-
throw new BackwardCompatibilityError(
107-
`Found ${errors.length} backward incompatible change(s). Proto file not updated.`
108-
);
109-
}
110-
// In dry-run mode, continue (report will show the errors)
111-
logger.info(`Dry run: ${this.outputPath} would NOT be updated due to incompatible changes`);
112-
return;
113103
}
114104

115105
// Write output using shared function (skip if dry-run)
@@ -155,16 +145,16 @@ if (require.main === module) {
155145

156146
const opts = command.opts() as BackwardCompatOpts;
157147

158-
if (!existsSync(opts.existing)) {
159-
logger.error(`Existing file not found: ${opts.existing}`);
160-
process.exit(1);
161-
}
148+
if (!existsSync(opts.existing)) {
149+
logger.error(`Existing file not found: ${opts.existing}`);
150+
process.exit(1);
151+
}
162152

163-
const existingIncoming = opts.incoming.filter(p => existsSync(p));
164-
if (existingIncoming.length === 0) {
165-
logger.error(`No incoming proto files found.`);
166-
process.exit(1);
167-
}
153+
const existingIncoming = opts.incoming.filter(p => existsSync(p));
154+
if (existingIncoming.length === 0) {
155+
logger.error(`No incoming proto files found.`);
156+
process.exit(1);
157+
}
168158

169159
const writer = new BackwardCompatibleWriter(
170160
opts.existing,
@@ -174,18 +164,18 @@ if (require.main === module) {
174164

175165
try {
176166
writer.process(opts.dryRun);
177-
} catch (error) {
167+
} catch (error) {
178168
// Write report even on error
179169
if (opts.report) {
180170
writeFileSync(opts.report, writer.getReporter().toMarkdown());
181171
logger.info(`Report written: ${opts.report}`);
182172
}
183173

184-
if (error instanceof BackwardCompatibilityError) {
185-
process.exit(1);
186-
}
187-
throw error;
174+
if (error instanceof BackwardCompatibilityError) {
175+
process.exit(1);
188176
}
177+
throw error;
178+
}
189179

190180
// Write report on success
191181
if (opts.report) {

‎tools/proto-convert/src/postprocessing/CompatibilityMerger.ts‎

Lines changed: 7 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -55,31 +55,6 @@ function addDeprecated<T extends HasAnnotations>(item: T): T {
5555
};
5656
}
5757

58-
/**
59-
* Check if optional modifier changed and report it.
60-
*/
61-
function checkOptionalChange(
62-
source: ProtoField,
63-
upcoming: ProtoField,
64-
msgName: string,
65-
reporter?: MergeReporter
66-
): boolean {
67-
const sourceOptional = source.modifier === 'optional';
68-
const upcomingOptional = upcoming.modifier === 'optional';
69-
70-
if (sourceOptional !== upcomingOptional) {
71-
reporter?.addFieldChange({
72-
messageName: msgName,
73-
changeType: 'optional_error',
74-
fieldName: source.name,
75-
existingType: formatField(source),
76-
incomingType: formatField(upcoming)
77-
});
78-
return true;
79-
}
80-
return false;
81-
}
82-
8358
/**
8459
* Check if two fields are compatible (same type and compatible modifiers).
8560
*/
@@ -106,18 +81,20 @@ function mergeField(
10681
if (upcomingField) {
10782
upcomingMap.delete(baseName);
10883

109-
// Report optional modifier changes (but don't stop)
110-
checkOptionalChange(sourceField, upcomingField, msgName, reporter);
111-
11284
if (fieldsMatch(sourceField, upcomingField)) {
11385
return sourceField;
11486
} else {
115-
// Type or repeated change - deprecate and version
11687
const newName = `${baseName}_${getFieldVersion(sourceField.name) + 1}`;
11788
upcomingMap.set(newName, { ...upcomingField, name: newName });
89+
90+
const sameType = sourceField.type === upcomingField.type;
91+
const sourceOptional = sourceField.modifier === 'optional';
92+
const upcomingOptional = upcomingField.modifier === 'optional';
93+
const isOptionalChange = sameType && (sourceOptional !== upcomingOptional);
94+
11895
reporter?.addFieldChange({
11996
messageName: msgName,
120-
changeType: 'type_changed',
97+
changeType: isOptionalChange ? 'optional_change' : 'type_changed',
12198
fieldName: sourceField.name,
12299
existingType: formatField(sourceField),
123100
incomingType: formatField(upcomingField),

‎tools/proto-convert/src/postprocessing/MergeReporter.ts‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,9 @@
11
/**
22
* Reporter for tracking changes during proto merge.
3-
* Only tracks meaningful changes: added, removed, type_changed.
3+
* Tracks: added, removed, type_changed, optional_change.
44
*/
55

6-
export type ChangeType = 'added' | 'removed' | 'type_changed' | 'optional_error';
6+
export type ChangeType = 'added' | 'removed' | 'type_changed' | 'optional_change';
77

88
/** Format a field for report display */
99
export function formatField(f: { name: string; type: string; modifier?: string }): string {
@@ -46,14 +46,14 @@ export class MergeReporter {
4646
* Check if there are any backward incompatible changes (optional modifier changes).
4747
*/
4848
hasIncompatibleChanges(): boolean {
49-
return this.fieldChanges.some(c => c.changeType === 'optional_error');
49+
return this.fieldChanges.some(c => c.changeType === 'optional_change');
5050
}
5151

5252
/**
5353
* Get list of backward incompatible changes.
5454
*/
5555
getIncompatibleChanges(): FieldChange[] {
56-
return this.fieldChanges.filter(c => c.changeType === 'optional_error');
56+
return this.fieldChanges.filter(c => c.changeType === 'optional_change');
5757
}
5858

5959
getFieldChanges(): FieldChange[] {
@@ -132,8 +132,8 @@ export class MergeReporter {
132132
return `Deprecated: \`${c.existingType}\``;
133133
case 'type_changed':
134134
return `\`${c.existingType}\` → \`${c.incomingType}\` (versioned as \`${c.versionedName}\`)`;
135-
case 'optional_error':
136-
return `Optional modifier changed: \`${c.existingType}\` → \`${c.incomingType}\``;
135+
case 'optional_change':
136+
return `⚠️ Breaking: \`${c.existingType}\` → \`${c.incomingType}\` (versioned as \`${c.versionedName}\`)`;
137137
default:
138138
return '';
139139
}

‎tools/proto-convert/test/postprocessing/BackwardCompatibleWriter.test.ts‎

Lines changed: 18 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -211,31 +211,36 @@ describe('BackwardCompatibleWriter', () => {
211211
});
212212

213213
describe('change reporting', () => {
214-
it('should throw BackwardCompatibilityError on optional change in real run', () => {
215-
const badIncoming = path.join(tempDir, 'bad_incoming.proto');
216-
fs.writeFileSync(badIncoming, PROTO_OPTIONAL_ADDED);
214+
it('should report optional change as incompatible but still write', () => {
215+
const optionalIncoming = path.join(tempDir, 'optional_incoming.proto');
216+
fs.writeFileSync(optionalIncoming, PROTO_OPTIONAL_ADDED);
217217

218218
const writer = new BackwardCompatibleWriter(
219219
EXISTING_PROTO,
220-
[badIncoming],
220+
[optionalIncoming],
221221
outputPath
222222
);
223223

224-
// Should throw in real run mode
225-
expect(() => writer.process()).toThrow(BackwardCompatibilityError);
224+
// Should NOT throw - writes even with incompatible changes
225+
writer.process();
226226

227-
// Report should still have the error recorded
227+
// Report should have the change recorded as optional_change (incompatible)
228228
const reporter = writer.getReporter();
229229
expect(reporter.hasIncompatibleChanges()).toBe(true);
230+
const optionalChanges = reporter.getFieldChanges().filter(c => c.changeType === 'optional_change');
231+
expect(optionalChanges.length).toBeGreaterThan(0);
232+
233+
// Output file should still be written
234+
expect(fs.existsSync(outputPath)).toBe(true);
230235
});
231236

232-
it('should report optional change without throwing in dry-run mode', () => {
233-
const badIncoming = path.join(tempDir, 'bad_incoming.proto');
234-
fs.writeFileSync(badIncoming, PROTO_OPTIONAL_ADDED);
237+
it('should report optional change in dry-run mode without writing', () => {
238+
const optionalIncoming = path.join(tempDir, 'optional_incoming.proto');
239+
fs.writeFileSync(optionalIncoming, PROTO_OPTIONAL_ADDED);
235240

236241
const writer = new BackwardCompatibleWriter(
237242
EXISTING_PROTO,
238-
[badIncoming],
243+
[optionalIncoming],
239244
outputPath
240245
);
241246

@@ -244,7 +249,8 @@ describe('BackwardCompatibleWriter', () => {
244249

245250
const reporter = writer.getReporter();
246251
expect(reporter.hasIncompatibleChanges()).toBe(true);
247-
expect(reporter.getIncompatibleChanges().length).toBeGreaterThan(0);
252+
const optionalChanges = reporter.getFieldChanges().filter(c => c.changeType === 'optional_change');
253+
expect(optionalChanges.length).toBeGreaterThan(0);
248254
});
249255

250256
it('should handle type change by deprecating old field', () => {

‎tools/proto-convert/test/postprocessing/CleanupUnusedMessages.test.ts‎

Lines changed: 27 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -33,17 +33,17 @@ describe('CleanupUnusedMessages', () => {
3333
const parsed = parseProtoFile(TEST_PROTO);
3434

3535
describe('findReachableTypes', () => {
36-
it('should identify reachable messages from roots', () => {
37-
const reachable = findReachableTypes(['SearchRequest', 'SearchResponse'], parsed.messages);
36+
it('should identify reachable messages from roots', () => {
37+
const reachable = findReachableTypes(['SearchRequest', 'SearchResponse'], parsed.messages);
3838

39-
// Root messages are reachable
40-
expect(reachable.has('SearchRequest')).toBe(true);
41-
expect(reachable.has('SearchResponse')).toBe(true);
39+
// Root messages are reachable
40+
expect(reachable.has('SearchRequest')).toBe(true);
41+
expect(reachable.has('SearchResponse')).toBe(true);
4242

43-
// Direct references are reachable
44-
expect(reachable.has('SearchOptions')).toBe(true);
45-
expect(reachable.has('SearchResult')).toBe(true);
46-
});
43+
// Direct references are reachable
44+
expect(reachable.has('SearchOptions')).toBe(true);
45+
expect(reachable.has('SearchResult')).toBe(true);
46+
});
4747

4848
it('should follow transitive references', () => {
4949
const reachable = findReachableTypes(['SearchRequest'], parsed.messages);
@@ -160,16 +160,16 @@ describe('CleanupUnusedMessages', () => {
160160
});
161161

162162
describe('filterMessages', () => {
163-
it('should filter messages to keep only reachable ones', () => {
164-
const reachable = findReachableTypes(['SearchRequest', 'SearchResponse'], parsed.messages);
165-
const kept = filterMessages(parsed.messages, reachable);
166-
167-
const keptNames = kept.map(m => m.name);
168-
expect(keptNames).toContain('SearchRequest');
169-
expect(keptNames).toContain('SearchResponse');
170-
expect(keptNames).not.toContain('UnusedMessage');
171-
expect(keptNames).not.toContain('AnotherUnused');
172-
});
163+
it('should filter messages to keep only reachable ones', () => {
164+
const reachable = findReachableTypes(['SearchRequest', 'SearchResponse'], parsed.messages);
165+
const kept = filterMessages(parsed.messages, reachable);
166+
167+
const keptNames = kept.map(m => m.name);
168+
expect(keptNames).toContain('SearchRequest');
169+
expect(keptNames).toContain('SearchResponse');
170+
expect(keptNames).not.toContain('UnusedMessage');
171+
expect(keptNames).not.toContain('AnotherUnused');
172+
});
173173

174174
it('should always keep custom message names (ObjectMap, GeneralNumber)', () => {
175175
const messages: ProtoMessage[] = [
@@ -189,14 +189,14 @@ describe('CleanupUnusedMessages', () => {
189189
});
190190

191191
describe('filterEnums', () => {
192-
it('should filter enums to keep only referenced ones', () => {
193-
const reachable = findReachableTypes(['SearchRequest', 'SearchResponse'], parsed.messages);
194-
const kept = filterEnums(parsed.enums, reachable);
195-
196-
const keptNames = kept.map(e => e.name);
197-
// SortOrder is referenced by SearchOptions
198-
expect(keptNames).toContain('SortOrder');
199-
expect(keptNames).not.toContain('UnusedEnum');
192+
it('should filter enums to keep only referenced ones', () => {
193+
const reachable = findReachableTypes(['SearchRequest', 'SearchResponse'], parsed.messages);
194+
const kept = filterEnums(parsed.enums, reachable);
195+
196+
const keptNames = kept.map(e => e.name);
197+
// SortOrder is referenced by SearchOptions
198+
expect(keptNames).toContain('SortOrder');
199+
expect(keptNames).not.toContain('UnusedEnum');
200200
});
201201

202202
it('should always keep custom enum names (NullValue)', () => {

‎tools/proto-convert/test/postprocessing/CompatibilityMerger.test.ts‎

Lines changed: 11 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -64,8 +64,8 @@ describe('mergeMessage', () => {
6464
});
6565
});
6666

67-
describe('optional changes (reported)', () => {
68-
it('should report when optional is added and handle as type change', () => {
67+
describe('optional changes (reported as incompatible but versioned)', () => {
68+
it('should report and version when optional is added', () => {
6969
const source: ProtoMessage = {
7070
name: 'TestMessage',
7171
fields: [field('id', 'int32', 1)]
@@ -78,18 +78,19 @@ describe('mergeMessage', () => {
7878

7979
const result = mergeMessage(source, upcoming, reporter);
8080

81-
// Optional change is reported
82-
const optionalChanges = reporter.getFieldChanges().filter(c => c.changeType === 'optional_error');
81+
// Optional change is reported as optional_change (incompatible)
82+
const optionalChanges = reporter.getFieldChanges().filter(c => c.changeType === 'optional_change');
8383
expect(optionalChanges).toHaveLength(1);
84-
// Modifier mismatch causes deprecation + versioning (2 fields)
84+
expect(reporter.hasIncompatibleChanges()).toBe(true);
85+
// Optional mismatch causes deprecation + versioning (2 fields)
8586
expect(result.fields).toHaveLength(2);
8687
expect(result.fields[0].name).toBe('id');
8788
expect(result.fields[0].annotations).toContainEqual({ name: 'deprecated', value: 'true' });
8889
expect(result.fields[1].name).toBe('id_1');
8990
expect(result.fields[1].modifier).toBe('optional');
9091
});
9192

92-
it('should report when optional is removed and handle as type change', () => {
93+
it('should report and version when optional is removed', () => {
9394
const source: ProtoMessage = {
9495
name: 'TestMessage',
9596
fields: [field('id', 'int32', 1, 'optional')]
@@ -102,10 +103,11 @@ describe('mergeMessage', () => {
102103

103104
const result = mergeMessage(source, upcoming, reporter);
104105

105-
// Optional change is reported
106-
const optionalChanges = reporter.getFieldChanges().filter(c => c.changeType === 'optional_error');
106+
// Optional change is reported as optional_change (incompatible)
107+
const optionalChanges = reporter.getFieldChanges().filter(c => c.changeType === 'optional_change');
107108
expect(optionalChanges).toHaveLength(1);
108-
// Modifier mismatch causes deprecation + versioning (2 fields)
109+
expect(reporter.hasIncompatibleChanges()).toBe(true);
110+
// Optional mismatch causes deprecation + versioning (2 fields)
109111
expect(result.fields).toHaveLength(2);
110112
expect(result.fields[0].name).toBe('id');
111113
expect(result.fields[0].modifier).toBe('optional');

0 commit comments

Comments
 (0)