Skip to content

Commit 6c06aa9

Browse files
lucy66hwlucy66hw
committed
Version oneof field migrations for compatibility
Signed-off-by: xil <fridalu66@gmail.com>
1 parent 7f5d894 commit 6c06aa9

6 files changed

Lines changed: 329 additions & 39 deletions

File tree

‎tools/proto-convert/src/SchemaModifier.ts‎

Lines changed: 22 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -749,23 +749,38 @@ export class SchemaModifier {
749749
return;
750750
}
751751

752+
const existingProperties = schema.properties ? { ...schema.properties } : {};
753+
const existingRequired = Array.isArray(schema.required) ? [...schema.required] : undefined;
752754
const mergedProperties: any = {};
753755

754756
for (const item of schema.oneOf) {
755757
if (item && item.properties) {
756-
Object.assign(mergedProperties, item.properties);
758+
for (const [name, property] of Object.entries(item.properties)) {
759+
mergedProperties[name] = {
760+
...(property as Record<string, unknown>),
761+
'x-oneof-property': true
762+
};
763+
}
757764
}
758765
}
759766

760-
schema.properties = mergedProperties;
767+
schema.properties = {
768+
...existingProperties,
769+
...mergedProperties
770+
};
761771
schema.minProperties = 1;
762772
schema.maxProperties = 1;
773+
if (existingRequired) {
774+
schema.required = existingRequired;
775+
}
763776

764777
if ('unevaluatedProperties' in schema) {
765778
delete schema.unevaluatedProperties;
766779
}
767780
delete schema.oneOf;
768-
delete schema.required;
781+
if (!existingRequired) {
782+
delete schema.required;
783+
}
769784
}
770785

771786
/**
@@ -783,9 +798,12 @@ export class SchemaModifier {
783798

784799
if (hasDirectPattern) {
785800
if (schema.properties) {
801+
const hasPreMarkedProperties = Object.values(schema.properties).some(prop =>
802+
prop && typeof prop === 'object' && 'x-oneof-property' in (prop as Record<string, unknown>)
803+
);
786804
for (const propName in schema.properties) {
787805
const prop = schema.properties[propName] as any;
788-
if (prop && typeof prop === 'object') {
806+
if (prop && typeof prop === 'object' && (!hasPreMarkedProperties || prop['x-oneof-property'])) {
789807
prop['x-oneof-property'] = true;
790808
}
791809
}

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

Lines changed: 113 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,7 @@ function fieldsMatch(a: ProtoField, b: ProtoField): boolean {
7979
function mergeField(
8080
sourceField: ProtoField,
8181
upcomingMap: Map<string, ProtoField>,
82+
versionMap: Map<string, number>,
8283
msgName: string,
8384
reporter?: CompatibilityReporter
8485
): ProtoField {
@@ -99,18 +100,20 @@ function mergeField(
99100
const sourceOptional = sourceField.modifier === 'optional';
100101
const upcomingOptional = upcomingField.modifier === 'optional';
101102
const isOptionalChange = sameType && (sourceOptional !== upcomingOptional);
103+
const newName = nextVersionedName(baseName, versionMap);
102104

103105
if (isOptionalChange) {
106+
upcomingMap.set(newName, { ...upcomingField, name: newName, comment: sourceField.comment });
104107
reporter?.addFieldChange({
105108
messageName: msgName,
106109
changeType: 'OPTIONAL CHANGE',
107110
fieldName: sourceField.name,
108-
existingType: formatField({ ...sourceField, number: sourceField.number }),
109-
incomingType: formatField({ ...upcomingField, number: sourceField.number })
111+
existingType: formatField({ ...sourceField, number: sourceField.number, deprecated: true }),
112+
incomingType: formatField(upcomingField),
113+
versionedName: newName
110114
});
111-
return { ...upcomingField, name: sourceField.name, number: sourceField.number };
115+
return addDeprecated(sourceField);
112116
} else {
113-
const newName = `${baseName}_${getFieldVersion(sourceField.name) + 1}`;
114117
upcomingMap.set(newName, { ...upcomingField, name: newName, comment: sourceField.comment });
115118

116119
reporter?.addFieldChange({
@@ -168,6 +171,34 @@ function hasOneof(msg: ProtoMessage): boolean {
168171
return (msg.oneofs?.some(o => o.fields.length > 0)) ?? false;
169172
}
170173

174+
function collectMaxVersions(msg: ProtoMessage): Map<string, number> {
175+
const versions = new Map<string, number>();
176+
177+
const track = (field: ProtoField): void => {
178+
const baseName = getBaseName(field.name);
179+
const version = getFieldVersion(field.name);
180+
versions.set(baseName, Math.max(versions.get(baseName) ?? 1, version));
181+
};
182+
183+
for (const field of msg.fields) {
184+
track(field);
185+
}
186+
187+
for (const oneof of msg.oneofs || []) {
188+
for (const field of oneof.fields) {
189+
track(field);
190+
}
191+
}
192+
193+
return versions;
194+
}
195+
196+
function nextVersionedName(baseName: string, versionMap: Map<string, number>): string {
197+
const nextVersion = (versionMap.get(baseName) ?? 1) + 1;
198+
versionMap.set(baseName, nextVersion);
199+
return `${baseName}_${nextVersion}`;
200+
}
201+
171202
/**
172203
* Merge a source message with an upcoming message.
173204
*/
@@ -179,6 +210,8 @@ export function mergeMessage(
179210
// Check for oneof structure change (one has oneof, other doesn't)
180211
const sourceHasOneof = hasOneof(sourceMsg);
181212
const upcomingHasOneof = hasOneof(upcomingMsg);
213+
const upcomingOneofs = upcomingMsg.oneofs || [];
214+
const versionMap = collectMaxVersions(sourceMsg);
182215

183216
if (sourceHasOneof !== upcomingHasOneof) {
184217
reporter?.addFieldChange({
@@ -193,12 +226,15 @@ export function mergeMessage(
193226
const upcomingByName = new Map(upcomingMsg.fields.map(f => [f.name, f]));
194227

195228
let maxFieldNumber = 0;
229+
const usedFieldNumbers = new Set<number>();
196230
const mergedFields: ProtoField[] = [];
197231

198232
// Process regular fields
199233
for (const sourceField of sourceMsg.fields) {
200234
maxFieldNumber = Math.max(maxFieldNumber, sourceField.number);
201-
mergedFields.push(mergeField(sourceField, upcomingByName, sourceMsg.name, reporter));
235+
const mergedField = mergeField(sourceField, upcomingByName, versionMap, sourceMsg.name, reporter);
236+
usedFieldNumbers.add(mergedField.number);
237+
mergedFields.push(mergedField);
202238
}
203239

204240
// Process oneofs
@@ -207,7 +243,7 @@ export function mergeMessage(
207243

208244
if (sourceMsg.oneofs) {
209245
const upcomingOneofMap = new Map(
210-
(upcomingMsg.oneofs || []).map(o => [o.name, o])
246+
upcomingOneofs.map(o => [o.name, o])
211247
);
212248

213249
mergedOneofs = [];
@@ -232,7 +268,7 @@ export function mergeMessage(
232268

233269
// Name match
234270
if (upcomingOneofByName.has(getBaseName(sourceField.name))) {
235-
mergedOneofFields.push(mergeField(sourceField, upcomingOneofByName, sourceMsg.name, reporter));
271+
mergedOneofFields.push(mergeField(sourceField, upcomingOneofByName, versionMap, sourceMsg.name, reporter));
236272
continue;
237273
}
238274

@@ -245,6 +281,7 @@ export function mergeMessage(
245281
number: sourceField.number,
246282
comment: sourceField.comment || upcomingField.comment
247283
});
284+
usedFieldNumbers.add(sourceField.number);
248285

249286
upcomingOneofByName.delete(upcomingField.name);
250287
upcomingByType.delete(sourceField.type);
@@ -259,28 +296,78 @@ export function mergeMessage(
259296
}
260297

261298
// Try 3: No match - deprecate or skip
262-
mergedOneofFields.push(mergeField(sourceField, upcomingOneofByName, sourceMsg.name, reporter));
299+
mergedOneofFields.push(mergeField(sourceField, upcomingOneofByName, versionMap, sourceMsg.name, reporter));
263300
}
264301

265302
mergedOneofs.push({ ...sourceOneof, fields: mergedOneofFields });
266303
oneofMaps.set(sourceOneof.name, upcomingOneofByName);
267304
}
305+
} else if (upcomingHasOneof) {
306+
const sourceFieldByName = new Map(
307+
sourceMsg.fields.map(f => [getBaseName(f.name), f])
308+
);
309+
mergedOneofs = [];
310+
311+
for (const upcomingOneof of upcomingOneofs) {
312+
const mergedOneofFields: ProtoField[] = [];
313+
314+
for (const field of upcomingOneof.fields) {
315+
const sourceField = sourceFieldByName.get(getBaseName(field.name));
316+
let oneofField = { ...field };
317+
let fieldNumber = field.number;
318+
319+
if (sourceField) {
320+
oneofField = {
321+
...oneofField,
322+
name: nextVersionedName(getBaseName(sourceField.name), versionMap),
323+
comment: sourceField.comment || field.comment
324+
};
325+
}
326+
327+
if (sourceField || usedFieldNumbers.has(fieldNumber)) {
328+
do {
329+
fieldNumber = ++maxFieldNumber;
330+
} while (usedFieldNumbers.has(fieldNumber));
331+
}
332+
maxFieldNumber = Math.max(maxFieldNumber, fieldNumber);
333+
usedFieldNumbers.add(fieldNumber);
334+
335+
reporter?.addFieldChange({
336+
messageName: sourceMsg.name,
337+
changeType: 'ADDED',
338+
fieldName: `${upcomingOneof.name}.${oneofField.name}`,
339+
incomingType: formatField({ ...oneofField, number: fieldNumber })
340+
});
341+
mergedOneofFields.push({ ...oneofField, number: fieldNumber });
342+
}
343+
344+
mergedOneofs.push({ ...upcomingOneof, fields: mergedOneofFields });
345+
}
268346
}
269347

270348
// Assign field max number to remaining fields.
271349
for (const field of upcomingByName.values()) {
350+
let addedField = { ...field };
351+
if (!isVersionedName(addedField.name) && versionMap.has(getBaseName(addedField.name))) {
352+
addedField = {
353+
...addedField,
354+
name: nextVersionedName(getBaseName(addedField.name), versionMap)
355+
};
356+
}
357+
272358
const fieldNumber = ++maxFieldNumber;
273-
if (isVersionedName(field.name)) {
274-
reporter?.updateVersionedNumber(field.name, fieldNumber);
359+
if (isVersionedName(addedField.name)) {
360+
reporter?.updateVersionedNumber(addedField.name, fieldNumber);
275361
} else {
276362
reporter?.addFieldChange({
277363
messageName: sourceMsg.name,
278364
changeType: 'ADDED',
279-
fieldName: field.name,
280-
incomingType: formatField({ ...field, number: fieldNumber })
365+
fieldName: addedField.name,
366+
incomingType: formatField({ ...addedField, number: fieldNumber })
281367
});
282368
}
283-
mergedFields.push({ ...field, number: fieldNumber });
369+
usedFieldNumbers.add(fieldNumber);
370+
mergedFields.push({ ...addedField, number: fieldNumber });
284371
}
285372

286373
// Assign field max number to remaining oneof fields.
@@ -289,18 +376,26 @@ export function mergeMessage(
289376
const remaining = oneofMaps.get(oneof.name);
290377
if (remaining) {
291378
for (const field of remaining.values()) {
379+
let addedField = { ...field };
380+
if (!isVersionedName(addedField.name) && versionMap.has(getBaseName(addedField.name))) {
381+
addedField = {
382+
...addedField,
383+
name: nextVersionedName(getBaseName(addedField.name), versionMap)
384+
};
385+
}
292386
const fieldNumber = ++maxFieldNumber;
293-
if (isVersionedName(field.name)) {
294-
reporter?.updateVersionedNumber(field.name, fieldNumber);
387+
if (isVersionedName(addedField.name)) {
388+
reporter?.updateVersionedNumber(addedField.name, fieldNumber);
295389
} else {
296390
reporter?.addFieldChange({
297391
messageName: sourceMsg.name,
298392
changeType: 'ADDED',
299-
fieldName: `${oneof.name}.${field.name}`,
300-
incomingType: formatField({ ...field, number: fieldNumber })
393+
fieldName: `${oneof.name}.${addedField.name}`,
394+
incomingType: formatField({ ...addedField, number: fieldNumber })
301395
});
302396
}
303-
oneof.fields.push({ ...field, number: fieldNumber });
397+
usedFieldNumbers.add(fieldNumber);
398+
oneof.fields.push({ ...addedField, number: fieldNumber });
304399
}
305400
}
306401
}

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,7 @@ export class CompatibilityReporter {
5454
*/
5555
updateVersionedNumber(versionedName: string, number: number): void {
5656
for (const change of this.fieldChanges) {
57-
if (change.changeType === 'TYPE CHANGED' && change.versionedName === versionedName) {
57+
if ((change.changeType === 'TYPE CHANGED' || change.changeType === 'OPTIONAL CHANGE') && change.versionedName === versionedName) {
5858
change.versionedNumber = number;
5959
break;
6060
}
@@ -133,7 +133,7 @@ export class CompatibilityReporter {
133133
}
134134

135135
private formatChangeRows(c: FieldChange): string[] {
136-
if (c.changeType === 'TYPE CHANGED') {
136+
if (c.changeType === 'TYPE CHANGED' || c.changeType === 'OPTIONAL CHANGE') {
137137
// Replace field name with versioned name and strip any wrong number
138138
let versionedField = c.incomingType?.replace(c.fieldName, c.versionedName || c.fieldName) || '';
139139
// Remove any existing field number (e.g., " = 1")

‎tools/proto-convert/test/SchemaModifier.test.ts‎

Lines changed: 54 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -839,13 +839,43 @@ describe('SchemaModifier', () => {
839839

840840
expect(schema.oneOf).toBeUndefined();
841841
expect(schema.properties).toEqual({
842-
field1: { type: 'string' },
843-
field2: { type: 'number' }
842+
field1: { type: 'string', 'x-oneof-property': true },
843+
field2: { type: 'number', 'x-oneof-property': true }
844844
});
845845
expect(schema.minProperties).toBe(1);
846846
expect(schema.maxProperties).toBe(1);
847847
});
848848

849+
it('should preserve existing parent properties and required fields', () => {
850+
const doc = createDocument();
851+
const modifier = new SchemaModifier(doc);
852+
853+
const schema: any = {
854+
properties: {
855+
index: { type: 'string' },
856+
path: { type: 'string' }
857+
},
858+
required: ['index', 'path'],
859+
oneOf: [
860+
{ properties: { id: { type: 'string' } }, required: ['id'] },
861+
{ properties: { query: { type: 'string' } }, required: ['query'] }
862+
]
863+
};
864+
865+
modifier.convertOneOfToMinMaxProperties(schema);
866+
867+
expect(schema.oneOf).toBeUndefined();
868+
expect(schema.properties).toEqual({
869+
index: { type: 'string' },
870+
path: { type: 'string' },
871+
id: { type: 'string', 'x-oneof-property': true },
872+
query: { type: 'string', 'x-oneof-property': true }
873+
});
874+
expect(schema.required).toEqual(['index', 'path']);
875+
expect(schema.minProperties).toBe(1);
876+
expect(schema.maxProperties).toBe(1);
877+
});
878+
849879
it('should not convert when items have multiple properties', () => {
850880
const doc = createDocument();
851881
const modifier = new SchemaModifier(doc);
@@ -889,6 +919,28 @@ describe('SchemaModifier', () => {
889919
expect(schema['x-oneof-schema']).toBe(true);
890920
});
891921

922+
it('should preserve pre-marked oneof properties without marking all properties', () => {
923+
const doc = createDocument();
924+
const modifier = new SchemaModifier(doc);
925+
926+
const schema: any = {
927+
type: 'object',
928+
properties: {
929+
index: { type: 'string' },
930+
id: { type: 'string', 'x-oneof-property': true },
931+
query: { type: 'string', 'x-oneof-property': true }
932+
},
933+
maxProperties: 1
934+
};
935+
936+
modifier.markOneOfExtensions(schema);
937+
938+
expect(schema.properties.index['x-oneof-property']).toBeUndefined();
939+
expect(schema.properties.id['x-oneof-property']).toBe(true);
940+
expect(schema.properties.query['x-oneof-property']).toBe(true);
941+
expect(schema['x-oneof-schema']).toBe(true);
942+
});
943+
892944
it('should mark parent schema when nested oneOf pattern exists', () => {
893945
const doc = createDocument();
894946
const modifier = new SchemaModifier(doc);

0 commit comments

Comments
 (0)