fix: don't reuse request field numbers when arguments are removed

protographic/src/sdl-to-proto-visitor.ts

@@ -9,7 +9,6 @@ import {
   GraphQLList,
   GraphQLNamedType,
   GraphQLNonNull,
-  GraphQLNullableType,
   GraphQLObjectType,
   GraphQLSchema,
   GraphQLType,
@@ -845,10 +844,7 @@ Example:
     const lockData = this.lockManager.getLockData();
     const argNames = field.args.map((arg) => graphqlFieldToProtoField(arg.name));
 
-    if (lockData.messages[requestName]) {
-      const originalFieldNames = Object.keys(lockData.messages[requestName].fields);
-      this.trackRemovedFields(requestName, originalFieldNames, argNames);
-    }
+    this.lockManager.reconcileMessageFieldOrder(requestName, argNames);
 
     // Add a description comment for the request message
     if (this.includeComments) {
@@ -883,8 +879,11 @@ Example:
         const argType = this.getProtoTypeFromGraphQL(arg.type);
         const argProtoName = graphqlFieldToProtoField(arg.name);
 
-        // Get the field number from the messages structure using the original field name
-        const fieldNumber = lockData.messages[operationName]?.fields[argName];
+        const fieldNumber = this.getFieldNumber(
+          requestName,
+          argProtoName,
+          this.getNextAvailableFieldNumber(requestName),
+        );
 
         // Add argument description as comment
         if (arg.description) {

protographic/tests/sdl-to-proto/08-proto-lock-manager.test.ts

@@ -1,5 +1,6 @@
 import { describe, expect, test } from 'vitest';
 import { ProtoLockManager } from '../../src/proto-lock';
+import { compileGraphQLToProto } from '../../src';
 
 describe('ProtoLockManager', () => {
   test('should correctly initialize lock data with ordered fields', () => {
@@ -186,4 +187,39 @@ describe('ProtoLockManager', () => {
     expect(lockData.messages.Message.reservedNumbers).toContain(3);
     expect(lockData.messages.Message.reservedNumbers).toContain(4);
   });
+
+  test('field indexes should not overlap when re-adding arguments', () => {
+    const schemaWithArgument = `
+      type Query {
+        hello(name: String!): String!
+      }
+    `;
+
+    const schemaWithoutArgument = `
+      type Query {
+        hello: String!
+      }
+    `;
+
+    const result1 = compileGraphQLToProto(schemaWithArgument);
+    expect(result1.lockData).not.toBeNull();
+
+    // Argument 'name' should be field 1 on the request message
+    expect(result1.lockData!.messages?.QueryHelloRequest?.fields?.name).toBe(1);
+
+    // Remove the name field from the arguments, and thus lock field 1 of the request message
+    const result2 = compileGraphQLToProto(schemaWithoutArgument, { lockData: result1.lockData! });
+    expect(result2.lockData).not.toBeNull();
+
+    // The index of the previous 'name' field on the request message should be reserved
+    expect(result2.lockData!.messages?.QueryHelloRequest?.reservedNumbers).toContain(1);
+
+    // Add the name argument back, should be index 2 now, we do not reuse indices
+    const result3 = compileGraphQLToProto(schemaWithArgument, { lockData: result2.lockData! });
+    expect(result3.lockData).not.toBeNull();
+
+    // Check that field 1 is still reserved, and that name is now field 2
+    expect(result3.lockData!.messages?.QueryHelloRequest?.reservedNumbers).toContain(1);
+    expect(result3.lockData!.messages?.QueryHelloRequest?.fields?.name, 'name field should be field 2').toBe(2);
+  });
 });