Preserve comments on versioned fields and Rename "REMOVED" to "DEPRECATED" in reports (opensearch-project#355)

* Preserve comments on versioned fields and Rename "REMOVED" to "DEPRECATED" in reports

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

* update changelog

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

---------

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

Author: lucy66hw

CHANGELOG.md

@@ -10,6 +10,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
 
 ### Changed
 - Fix enum value annotations not being preserved ([#353](https://github.com/opensearch-project/opensearch-protobufs/pull/353))
+- Preserve comments on versioned fields and Rename "REMOVED" to "DEPRECATED" in reports ([#355](https://github.com/opensearch-project/opensearch-protobufs/pull/355))
 - Updates the backward compatibility report workflow to compare against the latest proto files ([#356](https://github.com/opensearch-project/opensearch-protobufs/pull/356))
 ### Removed
 

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

@@ -100,7 +100,7 @@ function mergeField(
                 return { ...upcomingField, name: sourceField.name, number: sourceField.number };
             } else {
                 const newName = `${baseName}_${getFieldVersion(sourceField.name) + 1}`;
-                upcomingMap.set(newName, { ...upcomingField, name: newName });
+                upcomingMap.set(newName, { ...upcomingField, name: newName, comment: sourceField.comment });
 
                 reporter?.addFieldChange({
                     messageName: msgName,
@@ -116,7 +116,7 @@ function mergeField(
     } else {
         reporter?.addFieldChange({
             messageName: msgName,
-            changeType: 'REMOVED',
+            changeType: 'DEPRECATED',
             fieldName: sourceField.name,
             existingType: formatField({ ...sourceField, number: sourceField.number, deprecated: true })
         });

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

@@ -7,7 +7,7 @@ import { tmpdir } from 'os';
  * Tracks: added, removed, type_changed, optional_change, oneof_change.
  */
 
-export type ChangeType = 'ADDED' | 'REMOVED' | 'TYPE CHANGED' | 'OPTIONAL CHANGE' | 'ONEOF CHANGE';
+export type ChangeType = 'ADDED' | 'DEPRECATED' | 'TYPE CHANGED' | 'OPTIONAL CHANGE' | 'ONEOF CHANGE';
 
 /** Format a field for report display */
 export function formatField(f: { name: string; type: string; modifier?: string; number?: number; deprecated?: boolean }): string {
@@ -153,7 +153,7 @@ export class CompatibilityReporter {
         switch (c.changeType) {
             case 'ADDED':
                 return `\`${c.incomingType}\``;
-            case 'REMOVED':
+            case 'DEPRECATED':
                 return `\`${c.existingType}\``;
             case 'OPTIONAL CHANGE':
                 return `\`${c.existingType}\` → \`${c.incomingType}\``;
@@ -164,12 +164,12 @@ export class CompatibilityReporter {
         }
     }
 
-    private formatChangeType(changeType: ChangeType | 'ADDED' | 'REMOVED'): string {
+    private formatChangeType(changeType: ChangeType | 'ADDED' | 'DEPRECATED'): string {
         switch (changeType) {
             case 'ADDED':
                 return '➕ **ADDED**';
-            case 'REMOVED':
-                return '🗑️ **REMOVED**';
+            case 'DEPRECATED':
+                return '🗑️ **DEPRECATED**';
             case 'OPTIONAL CHANGE':
                 return '🚨 **BREAKING**';
             case 'ONEOF CHANGE':

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

@@ -204,6 +204,29 @@ describe('mergeMessage', () => {
             expect(result.fields[1].name).toBe('data_2');
             expect(result.fields[1].type).toBe('NewType');
         });
+
+        it('should preserve comment on versioned field when type changes', () => {
+            const source: ProtoMessage = {
+                name: 'TestMessage',
+                fields: [{
+                    name: 'data',
+                    type: 'string',
+                    number: 1,
+                    comment: 'This is a field description'
+                }]
+            };
+            const upcoming: ProtoMessage = {
+                name: 'TestMessage',
+                fields: [field('data', 'int32', 1)]
+            };
+
+            const result = mergeMessage(source, upcoming);
+
+            // New versioned field should have the source comment
+            const versionedField = result.fields.find(f => f.name === 'data_2');
+            expect(versionedField).toBeDefined();
+            expect(versionedField!.comment).toBe('This is a field description');
+        });
     });
 
     describe('deprecated fields', () => {

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

@@ -68,13 +68,13 @@ describe('CompatibilityReporter', () => {
         it('should format removed field change', () => {
             reporter.addFieldChange({
                 messageName: 'TestMessage',
-                changeType: 'REMOVED',
+                changeType: 'DEPRECATED',
                 fieldName: 'oldField',
                 existingType: 'int32 oldField'
             });
 
             const md = reporter.toMarkdown();
-            expect(md).toContain('🗑️ **REMOVED**');
+            expect(md).toContain('🗑️ **DEPRECATED**');
             expect(md).toContain('`int32 oldField`');
         });
 
@@ -180,15 +180,15 @@ describe('CompatibilityReporter', () => {
             });
             reporter.addFieldChange({
                 messageName: 'MessageA',
-                changeType: 'REMOVED',
+                changeType: 'DEPRECATED',
                 fieldName: 'field3',
                 existingType: 'bool field3'
             });
 
             const md = reporter.toMarkdown();
             expect(md).toContain('| MessageA | ➕ **ADDED** |');
             expect(md).toContain('| MessageB | ➕ **ADDED** |');
-            expect(md).toContain('| MessageA | 🗑️ **REMOVED** |');
+            expect(md).toContain('| MessageA | 🗑️ **DEPRECATED** |');
         });
     });