Skip to content

Commit e83e3e7

Browse files
lucy66hwlucy66hw
authored
Merge branch 'main' into use_official_generator
Signed-off-by: Xi Lu <fridalu66@gmail.com>
2 parents 6619033 + 76d0cf1 commit e83e3e7

6 files changed

Lines changed: 363 additions & 4 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +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+
- Add in-place rename support for oneOf fields in post-processing ([#363](https://github.com/opensearch-project/opensearch-protobufs/pull/363))
89
- Use official openapi-generator tool ([#364](https://github.com/opensearch-project/opensearch-protobufs/pull/364))
910

1011
### Changed

‎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: 60 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -129,6 +129,26 @@ function isVersionedName(name: string): boolean {
129129
return /_\d+$/.test(name);
130130
}
131131

132+
/**
133+
* Convert a name to snake_case variable name format.
134+
*/
135+
function toVarName(name: string): string {
136+
name = name.replace(/^_+/, '');
137+
138+
return name
139+
.replace(/([A-Z]+)([A-Z][a-z])/g, '$1_$2')
140+
.replace(/([a-z0-9])([A-Z])/g, '$1_$2')
141+
.replace(/(\d)([A-Za-z])/g, '$1_$2')
142+
.toLowerCase();
143+
}
144+
145+
/**
146+
* Check if field name is the formatted (snake_case) version of the type name.
147+
*/
148+
function isFormattedName(fieldName: string, typeName: string): boolean {
149+
return fieldName === toVarName(typeName);
150+
}
151+
132152
/** Check if message has any oneof with fields */
133153
function hasOneof(msg: ProtoMessage): boolean {
134154
return (msg.oneofs?.some(o => o.fields.length > 0)) ?? false;
@@ -179,13 +199,52 @@ export function mergeMessage(
179199
mergedOneofs = [];
180200
for (const sourceOneof of sourceMsg.oneofs) {
181201
const upcomingOneof = upcomingOneofMap.get(sourceOneof.name);
202+
const upcomingFields = upcomingOneof?.fields || [];
182203
const upcomingOneofByName = new Map(
183-
(upcomingOneof?.fields || []).map(f => [f.name, f])
204+
upcomingFields.map(f => [f.name, f])
184205
);
185206

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

‎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)