Fix enum value annotations not being preserved (opensearch-project#353)

* Fix enum value annotations not being preserved

Signed-off-by: xil <fridalu66@gmail.com>

---------

Signed-off-by: xil <fridalu66@gmail.com>

Author: lucy66hw

CHANGELOG.md

@@ -9,7 +9,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
 - Add CompatibilityReporter for backward compatibility change tracking ([#349](https://github.com/opensearch-project/opensearch-protobufs/pull/349))
 
 ### Changed
-
+- Fix enum value annotations not being preserved ([#353](https://github.com/opensearch-project/opensearch-protobufs/pull/353))
 ### Removed
 
 ### Fixed

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

@@ -159,7 +159,7 @@ export function cleanupUnusedMessages(opts: CleanupOptions): { removedMessages:
     // Find reachable types
     const reachable = findReachableTypes(roots, parsed.messages);
 
-    // Filter to keep only reachable
+    // Filter to keep only reachable messages and enums
     const keptMessages = filterMessages(parsed.messages, reachable);
     const keptEnums = filterEnums(parsed.enums, reachable);
 

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

@@ -257,6 +257,11 @@ export function mergeEnum(
     for (const sourceValue of sourceEnum.values) {
         maxValueNumber = Math.max(maxValueNumber, sourceValue.number);
 
+        if (isDeprecated(sourceValue)) {
+            mergedValues.push(sourceValue);
+            continue;
+        }
+
         const upcomingValue = upcomingValueMap.get(sourceValue.name);
 
         if (upcomingValue) {

tools/proto-convert/src/postprocessing/parser.ts

@@ -54,10 +54,59 @@ export function convertField(field: Field): ProtoField {
     };
 }
 
+/**
+ * Extract enum value annotations from raw proto file content.
+
+ */
+export function extractEnumValueAnnotations(content: string): Map<string, Map<string, Annotation[]>> {
+    const result = new Map<string, Map<string, Annotation[]>>();
+
+    // Match enum blocks
+    const enumRegex = /enum\s+(\w+)\s*\{([^}]+)\}/g;
+    let enumMatch;
+
+    while ((enumMatch = enumRegex.exec(content)) !== null) {
+        const enumName = enumMatch[1];
+        const enumBody = enumMatch[2];
+        const valueAnnotations = new Map<string, Annotation[]>();
+
+        // Match enum values with options: VALUE_NAME = 123 [option = value, ...];
+        const valueRegex = /(\w+)\s*=\s*\d+\s*\[([^\]]+)\]/g;
+        let valueMatch;
+
+        while ((valueMatch = valueRegex.exec(enumBody)) !== null) {
+            const valueName = valueMatch[1];
+            const optionsStr = valueMatch[2];
+            const annotations: Annotation[] = [];
+
+            // Parse options like "deprecated = true, custom = value"
+            const optionParts = optionsStr.split(',');
+            for (const part of optionParts) {
+                const eqIndex = part.indexOf('=');
+                if (eqIndex > 0) {
+                    const name = part.substring(0, eqIndex).trim();
+                    const value = part.substring(eqIndex + 1).trim();
+                    annotations.push({ name, value });
+                }
+            }
+
+            if (annotations.length > 0) {
+                valueAnnotations.set(valueName, annotations);
+            }
+        }
+
+        if (valueAnnotations.size > 0) {
+            result.set(enumName, valueAnnotations);
+        }
+    }
+
+    return result;
+}
+
 /**
  * Convert a protobufjs Enum to internal ProtoEnum type.
  */
-export function convertEnum(enumDef: Enum): ProtoEnum {
+export function convertEnum(enumDef: Enum, valueAnnotations?: Map<string, Annotation[]>): ProtoEnum {
     const values: ProtoEnumValue[] = [];
 
     for (const [name, number] of Object.entries(enumDef.values)) {
@@ -66,12 +115,9 @@ export function convertEnum(enumDef: Enum): ProtoEnum {
             number: number as number
         };
 
-        const valuesOptions = (enumDef as any).valuesOptions;
-        if (valuesOptions && valuesOptions[name]) {
-            value.annotations = Object.entries(valuesOptions[name]).map(([k, v]) => ({
-                name: k,
-                value: String(v)
-            }));
+        // Get annotations from raw file parsing (protobufjs doesn't parse these)
+        if (valueAnnotations && valueAnnotations.has(name)) {
+            value.annotations = valueAnnotations.get(name);
         }
 
         values.push(value);
@@ -188,6 +234,9 @@ export function parseProtoFile(filePath: string): ParsedProtoFile {
     const content = readFileSync(filePath, 'utf8');
     const parsed = parse(content, { keepCase: true, alternateCommentMode: true });
 
+    // Extract enum value annotations from raw content (protobufjs doesn't parse these)
+    const enumValueAnnotations = extractEnumValueAnnotations(content);
+
     const messages: ProtoMessage[] = [];
     const enums: ProtoEnum[] = [];
     const services: ProtoService[] = [];
@@ -198,7 +247,8 @@ export function parseProtoFile(filePath: string): ParsedProtoFile {
                 if (nested instanceof Type) {
                     messages.push(convertMessage(nested));
                 } else if (nested instanceof Enum) {
-                    enums.push(convertEnum(nested));
+                    const annotations = enumValueAnnotations.get(nested.name);
+                    enums.push(convertEnum(nested, annotations));
                 } else if (nested instanceof Service) {
                     services.push(convertService(nested));
                 } else if (nested instanceof Namespace) {

tools/proto-convert/test/fixtures/proto/test.proto

@@ -102,6 +102,7 @@ enum Status {
   STATUS_UNSPECIFIED = 0;
   STATUS_ACTIVE = 1;
   STATUS_INACTIVE = 2;
+  STATUS_DEPRECATED = 3 [deprecated = true];
 }
 
 // ==================== UNUSED (for cleanup testing) ====================

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

@@ -741,6 +741,28 @@ describe('mergeEnum', () => {
             );
             expect(deprecatedOptions).toHaveLength(1);
         });
+
+        it('should not report already deprecated values', () => {
+            const source: ProtoEnum = {
+                name: 'Status',
+                values: [{
+                    name: 'STATUS_OLD',
+                    number: 1,
+                    annotations: [{ name: 'deprecated', value: 'true' }]
+                }]
+            };
+            const upcoming: ProtoEnum = {
+                name: 'Status',
+                values: []
+            };
+
+            const reporter = new CompatibilityReporter();
+            mergeEnum(source, upcoming, reporter);
+
+            // Reporter should have no enum changes since value was already deprecated
+            const markdown = reporter.toMarkdown();
+            expect(markdown).toContain('No changes detected');
+        });
     });
 
     describe('new values', () => {

tools/proto-convert/test/postprocessing/parser.test.ts

@@ -4,7 +4,7 @@
  */
 
 import * as path from 'path';
-import { parseProtoFile } from '../../src/postprocessing/parser';
+import { parseProtoFile, extractEnumValueAnnotations } from '../../src/postprocessing/parser';
 
 const TEST_PROTO = path.join(__dirname, '../fixtures/proto/test.proto');
 
@@ -96,4 +96,66 @@ describe('parseProtoFile', () => {
         const filterMsg = result.messages.find(m => m.name === 'FilterSettings');
         expect(filterMsg!.oneofs![0].comment).toBe('Filter value - can be different types');
     });
+
+    it('should parse enum value annotations', () => {
+        const statusEnum = result.enums.find(e => e.name === 'Status');
+        expect(statusEnum).toBeDefined();
+
+        // Regular values should not have annotations
+        const activeValue = statusEnum!.values.find(v => v.name === 'STATUS_ACTIVE');
+        expect(activeValue!.annotations).toBeUndefined();
+
+        // Deprecated value should have annotation
+        const deprecatedValue = statusEnum!.values.find(v => v.name === 'STATUS_DEPRECATED');
+        expect(deprecatedValue).toBeDefined();
+        expect(deprecatedValue!.annotations).toContainEqual({ name: 'deprecated', value: 'true' });
+    });
+});
+
+describe('extractEnumValueAnnotations', () => {
+
+    it('should extract multiple annotations from enum values', () => {
+        const content = `
+        enum MultiAnnotation {
+        VALUE_A = 0 [deprecated = true, custom = value];
+        }
+        `;
+        const result = extractEnumValueAnnotations(content);
+
+        expect(result.has('MultiAnnotation')).toBe(true);
+        const annotations = result.get('MultiAnnotation')!.get('VALUE_A')!;
+
+        expect(annotations).toHaveLength(2);
+        expect(annotations).toContainEqual({ name: 'deprecated', value: 'true' });
+        expect(annotations).toContainEqual({ name: 'custom', value: 'value' });
+    });
+
+    it('should handle multiple enums', () => {
+        const content = `
+            enum EnumA {
+            A_VALUE = 0 [deprecated = true];
+            }
+            enum EnumB {
+            B_VALUE = 1 [deprecated = true];
+            }
+        `;
+        const result = extractEnumValueAnnotations(content);
+
+        expect(result.has('EnumA')).toBe(true);
+        expect(result.has('EnumB')).toBe(true);
+        expect(result.get('EnumA')!.has('A_VALUE')).toBe(true);
+        expect(result.get('EnumB')!.has('B_VALUE')).toBe(true);
+    });
+
+    it('should return empty map when no annotations exist', () => {
+        const content = `
+        enum NoAnnotations {
+        VALUE_A = 0;
+        VALUE_B = 1;
+        }
+        `;
+        const result = extractEnumValueAnnotations(content);
+
+        expect(result.size).toBe(0);
+    });
 });