Skip to content

Commit 96bbc44

Browse files
committed
Add @grpc_only annotation to preserve manually-maintained protobuf fields
Signed-off-by: xil <fridalu66@gmail.com>
1 parent 8f45486 commit 96bbc44

6 files changed

Lines changed: 167 additions & 5 deletions

File tree

CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
44

55
## [Unreleased]
66
### Added
7+
- Add tooling_skip field option to preserve manually-maintained protobuf fields ([#378](https://github.com/opensearch-project/opensearch-protobufs/pull/378))
78

89
### Changed
910
- Fix simplifySingleMapSchema to generate named wrapper schemas. ([#406](https://github.com/opensearch-project/opensearch-protobufs/pull/406))

protos/schemas/common.proto

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,13 @@ option go_package = "github.com/opensearch-project/opensearch-protobufs/go/opens
88
option java_multiple_files = true;
99
option java_outer_classname = "CommonProto";
1010
option java_package = "org.opensearch.protobufs";
11+
import "google/protobuf/descriptor.proto";
12+
13+
// Custom field option for tooling
14+
extend google.protobuf.FieldOptions {
15+
// Mark a field to be skipped by tooling. These fields are manually maintained and won't be marked as deprecated.
16+
optional bool tooling_skip = 50001;
17+
}
1118

1219
// The Search API operation to perform a search across all indices in the cluster or index search
1320
message SearchRequest {
@@ -134,7 +141,7 @@ message SearchRequestBody {
134141
optional TrackHits track_total_hits = 7;
135142

136143
// [optional] Values used to boost the score of specified indexes. Specify in the format of <index> : <boost-multiplier>
137-
map<string, float> indices_boost = 8;
144+
map<string, float> indices_boost = 8 [deprecated = true];
138145

139146
// [optional] The fields that OpenSearch should return using their docvalue forms. Specify a format to return results in a certain format, such as date and time.
140147
repeated FieldAndFormat docvalue_fields = 9;
@@ -210,6 +217,9 @@ message SearchRequestBody {
210217

211218
// [optional]
212219
map<string, DerivedField> derived = 35;
220+
221+
// [optional] Values used to boost the score of specified indexes. Specify in the format of <index> : <boost-multiplier>
222+
repeated FloatMap indices_boost_2 = 36;
213223
}
214224

215225
message DerivedField {
@@ -2908,6 +2918,11 @@ message ShardSearchFailure {
29082918
ErrorCause reason = 4;
29092919
}
29102920

2921+
message FloatMap {
2922+
2923+
map<string, float> float_map = 1;
2924+
}
2925+
29112926
enum FieldValueFactorModifier {
29122927
FIELD_VALUE_FACTOR_MODIFIER_UNSPECIFIED = 0;
29132928
FIELD_VALUE_FACTOR_MODIFIER_LN = 1;
Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
2+
import "google/protobuf/descriptor.proto";
3+
4+
// Custom field option for tooling
5+
extend google.protobuf.FieldOptions {
6+
// Mark a field to be skipped by tooling. These fields are manually maintained and won't be marked as deprecated.
7+
optional bool tooling_skip = 50001;
8+
}

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

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import {
1313
import { CompatibilityReporter, formatField } from './CompatibilityReporter';
1414

1515
const DEPRECATED: Annotation = { name: 'deprecated', value: 'true' };
16+
const TOOLING_SKIP: Annotation = { name: '(tooling_skip)', value: 'true' };
1617

1718
/**
1819
* Extract base name
@@ -42,6 +43,16 @@ function isDeprecated(item: HasAnnotations): boolean {
4243
) ?? false;
4344
}
4445

46+
/**
47+
* Check if an item has tooling_skip option set to true.
48+
* Fields with this option are manually maintained and won't be deprecated.
49+
*/
50+
function hasToolingSkip(item: HasAnnotations): boolean {
51+
return item.annotations?.some(a =>
52+
a.name === TOOLING_SKIP.name && a.value === TOOLING_SKIP.value
53+
) ?? false;
54+
}
55+
4556
/**
4657
* Add deprecated annotation to an item if not already deprecated.
4758
*/
@@ -114,6 +125,9 @@ function mergeField(
114125
}
115126
}
116127
} else {
128+
if (hasToolingSkip(sourceField)) {
129+
return sourceField;
130+
}
117131
reporter?.addFieldChange({
118132
messageName: msgName,
119133
changeType: 'DEPRECATED',
@@ -244,7 +258,7 @@ export function mergeMessage(
244258
continue;
245259
}
246260

247-
// Try 3: No match - deprecate
261+
// Try 3: No match - deprecate or skip
248262
mergedOneofFields.push(mergeField(sourceField, upcomingOneofByName, sourceMsg.name, reporter));
249263
}
250264

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

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -9,9 +9,10 @@ import { ProtoMessage, ProtoEnum } from './types';
99

1010
const TEMPLATE = readFileSync(join(__dirname, 'templates', 'proto.mustache'), 'utf8');
1111

12-
const TEMPLATE_DIR = join(__dirname, '../config/protobuf-schema-template');
13-
const PROTO_HEADER = readFileSync(join(TEMPLATE_DIR, 'partial_header.mustache'), 'utf-8');
14-
const CUSTOM_MESSAGES = readFileSync(join(TEMPLATE_DIR, 'custom_message.mustache'), 'utf-8');
12+
const CONFIG_TEMPLATE_DIR = join(__dirname, '../config/protobuf-schema-template');
13+
const PROTO_HEADER = readFileSync(join(CONFIG_TEMPLATE_DIR, 'partial_header.mustache'), 'utf-8');
14+
const FIELD_OPTIONS = readFileSync(join(CONFIG_TEMPLATE_DIR, 'field_options.mustache'), 'utf-8');
15+
const CUSTOM_MESSAGES = readFileSync(join(CONFIG_TEMPLATE_DIR, 'custom_message.mustache'), 'utf-8');
1516

1617
// Custom messages/enums defined in template - always handled separately
1718
export const CUSTOM_MESSAGE_NAMES = new Set(['ObjectMap', 'GeneralNumber']);
@@ -96,6 +97,9 @@ export function writeProtoFile(
9697

9798
// Use fixed header from template
9899
outputParts.push(PROTO_HEADER.trim());
100+
101+
// Add field options (import descriptor + extend)
102+
outputParts.push(FIELD_OPTIONS.trim());
99103

100104
// Generate messages (excluding custom ones - they're added from template)
101105
for (const msg of messages) {

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

Lines changed: 120 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -275,6 +275,125 @@ describe('mergeMessage', () => {
275275
);
276276
expect(deprecatedOptions).toHaveLength(1);
277277
});
278+
279+
it('should keep fields with tooling_skip option without deprecating', () => {
280+
const source: ProtoMessage = {
281+
name: 'TestMessage',
282+
fields: [{
283+
name: 'skip_field',
284+
type: 'string',
285+
number: 1,
286+
annotations: [{ name: '(tooling_skip)', value: 'true' }]
287+
}, {
288+
name: 'normal_field',
289+
type: 'string',
290+
number: 2
291+
}]
292+
};
293+
const upcoming: ProtoMessage = {
294+
name: 'TestMessage',
295+
fields: [{
296+
name: 'normal_field',
297+
type: 'string',
298+
number: 2
299+
}]
300+
};
301+
302+
const result = mergeMessage(source, upcoming);
303+
304+
// skip_field should remain without deprecated annotation added
305+
expect(result.fields).toHaveLength(2);
306+
const skipField = result.fields.find(f => f.name === 'skip_field');
307+
expect(skipField).toBeDefined();
308+
// Should only have tooling_skip, not deprecated
309+
expect(skipField!.annotations).not.toContainEqual({ name: 'deprecated', value: 'true' });
310+
});
311+
312+
it('should not report tooling_skip fields as deprecated', () => {
313+
const source: ProtoMessage = {
314+
name: 'TestMessage',
315+
fields: [{
316+
name: 'skip_field',
317+
type: 'string',
318+
number: 1,
319+
annotations: [{ name: '(tooling_skip)', value: 'true' }]
320+
}]
321+
};
322+
const upcoming: ProtoMessage = {
323+
name: 'TestMessage',
324+
fields: []
325+
};
326+
327+
const reporter = new CompatibilityReporter();
328+
mergeMessage(source, upcoming, reporter);
329+
330+
const markdown = reporter.toMarkdown();
331+
// Should not report deprecation for tooling_skip fields
332+
expect(markdown).toContain('No changes detected');
333+
});
334+
335+
it('should keep tooling_skip field if it exists in upcoming', () => {
336+
const source: ProtoMessage = {
337+
name: 'TestMessage',
338+
fields: [{
339+
name: 'field_name',
340+
type: 'string',
341+
number: 1,
342+
annotations: [{ name: '(tooling_skip)', value: 'true' }]
343+
}]
344+
};
345+
const upcoming: ProtoMessage = {
346+
name: 'TestMessage',
347+
fields: [{
348+
name: 'field_name',
349+
type: 'string',
350+
number: 1
351+
}]
352+
};
353+
354+
const result = mergeMessage(source, upcoming);
355+
356+
expect(result.fields).toHaveLength(1);
357+
expect(result.fields[0].name).toBe('field_name');
358+
});
359+
360+
it('should assign new field number after max including tooling_skip field', () => {
361+
const source: ProtoMessage = {
362+
name: 'TestMessage',
363+
fields: [{
364+
name: 'existing_field',
365+
type: 'string',
366+
number: 1
367+
}, {
368+
name: 'skip_field',
369+
type: 'string',
370+
number: 5, // This is the max field number
371+
annotations: [{ name: '(tooling_skip)', value: 'true' }]
372+
}]
373+
};
374+
const upcoming: ProtoMessage = {
375+
name: 'TestMessage',
376+
fields: [{
377+
name: 'existing_field',
378+
type: 'string',
379+
number: 1
380+
}, {
381+
name: 'new_field',
382+
type: 'int32',
383+
number: 99 // This will be reassigned
384+
}]
385+
};
386+
387+
const result = mergeMessage(source, upcoming);
388+
389+
// skip_field is kept, new_field is added
390+
expect(result.fields).toHaveLength(3);
391+
expect(result.fields.find(f => f.name === 'skip_field')).toBeDefined();
392+
393+
const newField = result.fields.find(f => f.name === 'new_field');
394+
expect(newField).toBeDefined();
395+
expect(newField!.number).toBe(6);
396+
});
278397
});
279398

280399
describe('new fields', () => {
@@ -884,6 +1003,7 @@ describe('mergeEnum', () => {
8841003
const markdown = reporter.toMarkdown();
8851004
expect(markdown).toContain('No changes detected');
8861005
});
1006+
8871007
});
8881008

8891009
describe('new values', () => {

0 commit comments

Comments
 (0)