Skip to content

Commit e426a9c

Browse files
lucy66hwlucy66hw
committed
Add in-place rename support for oneOf fields in procestprocessing
Signed-off-by: xil <fridalu66@gmail.com>
1 parent a090aa0 commit e426a9c

5 files changed

Lines changed: 174 additions & 5 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
55
## [Unreleased]
66
### Added
77
- Add local Protobuf conversion instructions to README ([#362](https://github.com/opensearch-project/opensearch-protobufs/pull/362))
8-
8+
- Add in-place rename support for oneOf fields in procestprocessing ([#363](https://github.com/opensearch-project/opensearch-protobufs/pull/363))
99
### Changed
1010

1111
### Removed

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

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,8 @@ import { writeProtoFile, CUSTOM_MESSAGE_NAMES, CUSTOM_ENUM_NAMES } from './write
1111
import { ProtoMessage, ProtoEnum } from './types';
1212
import logger from '../utils/logger';
1313

14+
const GOOGLE_PROTOBUF_PREFIX = 'google.protobuf.';
15+
1416
export function isBuiltInType(type: string): boolean {
1517
const builtIns = new Set([
1618
'double', 'float', 'int32', 'int64', 'uint32', 'uint64',
@@ -20,6 +22,13 @@ export function isBuiltInType(type: string): boolean {
2022
return builtIns.has(type);
2123
}
2224

25+
/**
26+
* Check if a type is Google protobuf type.
27+
*/
28+
export function isWellKnownType(type: string): boolean {
29+
return type.startsWith(GOOGLE_PROTOBUF_PREFIX);
30+
}
31+
2332
/**
2433
* Collect all type references from a message (fields + oneofs).
2534
*/
@@ -148,10 +157,10 @@ export function cleanupUnusedMessages(opts: CleanupOptions): { removedMessages:
148157

149158
const parsed = parseProtoFile(opts.input);
150159

151-
// Verify root messages exist
160+
// Verify root messages exist (skip well-known types like google.protobuf.*)
152161
const messageNames = new Set(parsed.messages.map(m => m.name));
153162
for (const rootMsg of roots) {
154-
if (!messageNames.has(rootMsg)) {
163+
if (!isWellKnownType(rootMsg) && !messageNames.has(rootMsg)) {
155164
throw new Error(`Root message not found: ${rootMsg}`);
156165
}
157166
}

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

Lines changed: 58 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -129,6 +129,24 @@ function isVersionedName(name: string): boolean {
129129
return /_\d+$/.test(name);
130130
}
131131

132+
/**
133+
* Convert PascalCase/camelCase type name to snake_case.
134+
* e.g., "TermsAggregation" -> "terms_aggregation"
135+
*/
136+
function toSnakeCase(typeName: string): string {
137+
return typeName
138+
.replace(/([A-Z])/g, '_$1')
139+
.toLowerCase()
140+
.replace(/^_/, '');
141+
}
142+
143+
/**
144+
* Check if field name is the formatted (snake_case) version of the type name.
145+
*/
146+
function isFormattedName(fieldName: string, typeName: string): boolean {
147+
return fieldName === toSnakeCase(typeName);
148+
}
149+
132150
/** Check if message has any oneof with fields */
133151
function hasOneof(msg: ProtoMessage): boolean {
134152
return (msg.oneofs?.some(o => o.fields.length > 0)) ?? false;
@@ -179,13 +197,52 @@ export function mergeMessage(
179197
mergedOneofs = [];
180198
for (const sourceOneof of sourceMsg.oneofs) {
181199
const upcomingOneof = upcomingOneofMap.get(sourceOneof.name);
200+
const upcomingFields = upcomingOneof?.fields || [];
182201
const upcomingOneofByName = new Map(
183-
(upcomingOneof?.fields || []).map(f => [f.name, f])
202+
upcomingFields.map(f => [f.name, f])
184203
);
185204

205+
const sourceTypes = sourceOneof.fields.map(f => f.type);
206+
const allTypesUnique = new Set(sourceTypes).size === sourceTypes.length;
207+
208+
// Build type-based map if types are unique
209+
const upcomingByType = allTypesUnique
210+
? new Map(upcomingFields.map(f => [f.type, f]))
211+
: new Map<string, ProtoField>();
212+
186213
const mergedOneofFields: ProtoField[] = [];
187214
for (const sourceField of sourceOneof.fields) {
188215
maxFieldNumber = Math.max(maxFieldNumber, sourceField.number);
216+
217+
// Name match
218+
if (upcomingOneofByName.has(getBaseName(sourceField.name))) {
219+
mergedOneofFields.push(mergeField(sourceField, upcomingOneofByName, sourceMsg.name, reporter));
220+
continue;
221+
}
222+
223+
// Type match with formatted name
224+
if (allTypesUnique && upcomingByType.has(sourceField.type) && isFormattedName(sourceField.name, sourceField.type)) {
225+
const upcomingField = upcomingByType.get(sourceField.type)!;
226+
227+
mergedOneofFields.push({
228+
...upcomingField,
229+
number: sourceField.number,
230+
comment: sourceField.comment || upcomingField.comment
231+
});
232+
233+
upcomingOneofByName.delete(upcomingField.name);
234+
upcomingByType.delete(sourceField.type);
235+
236+
reporter?.addFieldChange({
237+
messageName: sourceMsg.name,
238+
changeType: 'RENAMED',
239+
fieldName: `${sourceOneof.name}.${sourceField.name}`,
240+
incomingType: `→ ${sourceOneof.name}.${upcomingField.name}`
241+
});
242+
continue;
243+
}
244+
245+
// Try 3: No match - deprecate
189246
mergedOneofFields.push(mergeField(sourceField, upcomingOneofByName, sourceMsg.name, reporter));
190247
}
191248

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

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ import { tmpdir } from 'os';
77
* Tracks: added, removed, type_changed, optional_change, oneof_change.
88
*/
99

10-
export type ChangeType = 'ADDED' | 'DEPRECATED' | 'TYPE CHANGED' | 'OPTIONAL CHANGE' | 'ONEOF CHANGE';
10+
export type ChangeType = 'ADDED' | 'DEPRECATED' | 'TYPE CHANGED' | 'OPTIONAL CHANGE' | 'ONEOF CHANGE' | 'RENAMED';
1111

1212
/** Format a field for report display */
1313
export function formatField(f: { name: string; type: string; modifier?: string; number?: number; deprecated?: boolean }): string {
@@ -110,6 +110,7 @@ export class CompatibilityReporter {
110110
111111
- 🗑️ **DEPRECATED** - Field/value annotated as deprecated in protobufs and will be officially removed in the next major OpenSearch release
112112
- ➕ **ADDED** - New field/value added at the end of the message/enum
113+
- ✏️ **RENAMED** - Field renamed in-place
113114
- 🚨 **BREAKING** - This change will cause breaking change to Protobuf`;
114115
}
115116

@@ -159,6 +160,8 @@ export class CompatibilityReporter {
159160
return `\`${c.existingType}\` → \`${c.incomingType}\``;
160161
case 'ONEOF CHANGE':
161162
return `\`${c.fieldName}\` (moved from \`${c.existingLocation}\` to \`${c.incomingLocation}\`)`;
163+
case 'RENAMED':
164+
return `\`${c.fieldName}\` ${c.incomingType}`;
162165
default:
163166
return '';
164167
}
@@ -174,6 +177,8 @@ export class CompatibilityReporter {
174177
return '🚨 **BREAKING**';
175178
case 'ONEOF CHANGE':
176179
return '🚨 **BREAKING**';
180+
case 'RENAMED':
181+
return '✏️ **RENAMED**';
177182
default:
178183
return `**${changeType}**`;
179184
}

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

Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -483,6 +483,104 @@ describe('mergeMessage', () => {
483483
expect(newRegularField!.number).toBe(6);
484484
expect(newOneofField!.number).toBe(7);
485485
});
486+
487+
it('should rename oneof field in-place when type is unique and name is formatted from type', () => {
488+
const source: ProtoMessage = {
489+
name: 'TestMessage',
490+
fields: [],
491+
oneofs: [{
492+
name: 'variant',
493+
fields: [
494+
field('terms_aggregation', 'TermsAggregation', 1),
495+
field('avg_aggregation', 'AvgAggregation', 2)
496+
]
497+
}]
498+
};
499+
const upcoming: ProtoMessage = {
500+
name: 'TestMessage',
501+
fields: [],
502+
oneofs: [{
503+
name: 'variant',
504+
fields: [
505+
field('terms', 'TermsAggregation', 1),
506+
field('average', 'AvgAggregation', 2)
507+
]
508+
}]
509+
};
510+
511+
const result = mergeMessage(source, upcoming);
512+
513+
expect(result.oneofs![0].fields).toHaveLength(2);
514+
expect(result.oneofs![0].fields[0].name).toBe('terms');
515+
expect(result.oneofs![0].fields[0].type).toBe('TermsAggregation');
516+
expect(result.oneofs![0].fields[0].number).toBe(1);
517+
expect(result.oneofs![0].fields[1].name).toBe('average');
518+
expect(result.oneofs![0].fields[1].type).toBe('AvgAggregation');
519+
expect(result.oneofs![0].fields[1].number).toBe(2);
520+
});
521+
522+
it('should not rename in-place when source name is not formatted from type', () => {
523+
const source: ProtoMessage = {
524+
name: 'TestMessage',
525+
fields: [],
526+
oneofs: [{
527+
name: 'variant',
528+
fields: [
529+
field('my_custom_name', 'TermsAggregation', 1)
530+
]
531+
}]
532+
};
533+
const upcoming: ProtoMessage = {
534+
name: 'TestMessage',
535+
fields: [],
536+
oneofs: [{
537+
name: 'variant',
538+
fields: [
539+
field('terms', 'TermsAggregation', 1)
540+
]
541+
}]
542+
};
543+
544+
const result = mergeMessage(source, upcoming);
545+
546+
expect(result.oneofs![0].fields).toHaveLength(2);
547+
expect(result.oneofs![0].fields[0].name).toBe('my_custom_name');
548+
expect(result.oneofs![0].fields[0].annotations).toContainEqual({ name: 'deprecated', value: 'true' });
549+
expect(result.oneofs![0].fields[1].name).toBe('terms');
550+
});
551+
552+
it('should not rename in-place when types are not unique', () => {
553+
const source: ProtoMessage = {
554+
name: 'TestMessage',
555+
fields: [],
556+
oneofs: [{
557+
name: 'variant',
558+
fields: [
559+
field('first_string', 'string', 1),
560+
field('second_string', 'string', 2)
561+
]
562+
}]
563+
};
564+
const upcoming: ProtoMessage = {
565+
name: 'TestMessage',
566+
fields: [],
567+
oneofs: [{
568+
name: 'variant',
569+
fields: [
570+
field('renamed_first', 'string', 1),
571+
field('renamed_second', 'string', 2)
572+
]
573+
}]
574+
};
575+
576+
const result = mergeMessage(source, upcoming);
577+
578+
expect(result.oneofs![0].fields).toHaveLength(4);
579+
expect(result.oneofs![0].fields[0].name).toBe('first_string');
580+
expect(result.oneofs![0].fields[0].annotations).toContainEqual({ name: 'deprecated', value: 'true' });
581+
expect(result.oneofs![0].fields[1].name).toBe('second_string');
582+
expect(result.oneofs![0].fields[1].annotations).toContainEqual({ name: 'deprecated', value: 'true' });
583+
});
486584
});
487585

488586
describe('oneof structure changes (breaking)', () => {

0 commit comments

Comments
 (0)