Skip to content

Commit 0b611fb

Browse files
MMcMMMcM
authored
Clean up field presence checks and tests in anticipation of Protobuf Editions (#3920)
* Add a test of Proto3 with explicit presence. * Use `hasPresence` instead of `isOptional` in evolution validator. * See #3919 for confirmation that compiling with a newer Proto dependency will work.
1 parent 2d27420 commit 0b611fb

5 files changed

Lines changed: 133 additions & 9 deletions

File tree

‎fdb-record-layer-core/src/main/java/com/apple/foundationdb/record/metadata/MetaDataEvolutionValidator.java‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -274,13 +274,12 @@ private void validateField(@Nonnull FieldDescriptor oldFieldDescriptor, @Nonnull
274274
if (oldFieldDescriptor.isRequired() && !newFieldDescriptor.isRequired()) {
275275
throw new MetaDataException("required field is no longer required",
276276
LogMessageKeys.FIELD_NAME, oldFieldDescriptor.getName());
277-
} else if (oldFieldDescriptor.isOptional() && !newFieldDescriptor.isOptional()) {
278-
// TODO: In theory, optional -> repeated is okay, but only if the field is not indexed
279-
throw new MetaDataException("optional field is no longer optional",
280-
LogMessageKeys.FIELD_NAME, oldFieldDescriptor.getName());
281277
} else if (oldFieldDescriptor.isRepeated() && !newFieldDescriptor.isRepeated()) {
282278
throw new MetaDataException("repeated field is no longer repeated",
283279
LogMessageKeys.FIELD_NAME, oldFieldDescriptor.getName());
280+
} else if (oldFieldDescriptor.hasPresence() != newFieldDescriptor.hasPresence()) {
281+
throw new MetaDataException("field changed whether default values are stored if set explicitly",
282+
LogMessageKeys.FIELD_NAME, oldFieldDescriptor.getName());
284283
}
285284
if (oldFieldDescriptor.getType().equals(FieldDescriptor.Type.ENUM)) {
286285
validateEnum(newFieldDescriptor.getName(), oldFieldDescriptor.getEnumType(), newFieldDescriptor.getEnumType());

‎fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/metadata/MetaDataEvolutionValidatorTest.java‎

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -857,12 +857,17 @@ public void fieldLabelChanged() {
857857
DescriptorProtos.FieldDescriptorProto.Label.LABEL_REQUIRED
858858
);
859859
for (int i = 0; i < labels.size(); i++) {
860-
final int itr = i;
861-
final DescriptorProtos.FieldDescriptorProto.Label label = labels.get(itr);
862-
final String labelText = label.name().substring(label.name().indexOf('_') + 1).toLowerCase(Locale.ROOT);
863-
final String errMsg = labelText + " field is no longer " + labelText;
860+
final DescriptorProtos.FieldDescriptorProto.Label label = labels.get(i);
861+
final String errMsg;
862+
if (label == DescriptorProtos.FieldDescriptorProto.Label.LABEL_OPTIONAL) {
863+
errMsg = "field changed whether default values are stored if set explicitly";
864+
} else {
865+
final String labelText = label.name().substring(label.name().indexOf('_') + 1).toLowerCase(Locale.ROOT);
866+
errMsg = labelText + " field is no longer " + labelText;
867+
}
868+
final DescriptorProtos.FieldDescriptorProto.Label newLabel = labels.get((i + 1) % labels.size());
864869
FileDescriptor updatedFile = mutateField("MySimpleRecord", "str_value_indexed", oldFile,
865-
field -> field.setLabel(labels.get((itr + 1) % labels.size())));
870+
field -> field.setLabel(newLabel));
866871
assertInvalid(errMsg, oldFile, updatedFile);
867872

868873
oldFile = updatedFile;

‎fdb-record-layer-core/src/test/java/com/apple/foundationdb/record/provider/foundationdb/query/FDBRecordStoreNullQueryTest.java‎

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525
import com.apple.foundationdb.record.RecordMetaDataBuilder;
2626
import com.apple.foundationdb.record.TestRecordsNulls2Proto;
2727
import com.apple.foundationdb.record.TestRecordsNulls3Proto;
28+
import com.apple.foundationdb.record.TestRecordsNulls3ExplicitProto;
2829
import com.apple.foundationdb.record.TestRecordsTupleFieldsProto;
2930
import com.apple.foundationdb.record.TupleFieldsProto;
3031
import com.apple.foundationdb.record.metadata.Key;
@@ -108,6 +109,10 @@ protected static RecordMetaData proto3NestedMetaData() {
108109
return metaData.getRecordMetaData();
109110
}
110111

112+
protected static RecordMetaData proto3ExplicitMetaData() {
113+
return RecordMetaData.newBuilder().setRecords(TestRecordsNulls3ExplicitProto.getDescriptor()).getRecordMetaData();
114+
}
115+
111116
@FunctionalInterface
112117
interface RecordBuilder<M extends Message> {
113118
M build(@Nonnull String name, @Nullable Integer intValue, @Nullable String stringValue);
@@ -175,6 +180,18 @@ protected static DynamicMessage buildRecord3Dynamic(@Nonnull String name, @Nulla
175180
return builder.build();
176181
}
177182

183+
protected static TestRecordsNulls3ExplicitProto.MyNullRecord buildRecord3Explicit(@Nonnull String name, @Nullable Integer intValue, @Nullable String stringValue) {
184+
TestRecordsNulls3ExplicitProto.MyNullRecord.Builder builder = TestRecordsNulls3ExplicitProto.MyNullRecord.newBuilder();
185+
builder.setName(name);
186+
if (intValue != null) {
187+
builder.setIntValue(intValue);
188+
}
189+
if (stringValue != null) {
190+
builder.setStringValue(stringValue);
191+
}
192+
return builder.build();
193+
}
194+
178195
protected <M extends Message> void saveTestRecords(@Nonnull RecordBuilder<M> builder) {
179196
recordStore.saveRecord(builder.build("empty", null, null));
180197
recordStore.saveRecord(builder.build("default", 0, ""));
@@ -365,6 +382,53 @@ public void testProto3Nested() {
365382
}
366383
}
367384

385+
// Explicit presence ("optional") correctly distguishes as well (results same as proto2).
386+
@Test
387+
public void testProto3Explicit() {
388+
try (FDBRecordContext context = openContext()) {
389+
createOrOpenRecordStore(context, proto3ExplicitMetaData());
390+
saveTestRecords(FDBRecordStoreNullQueryTest::buildRecord3Explicit);
391+
392+
assertThat(executeQuery(RecordQuery.newBuilder()
393+
.setRecordType("MyNullRecord")
394+
.setFilter(Query.field("int_value").equalsValue(2))
395+
.build()),
396+
is(Collections.singletonList("two")));
397+
assertThat(executeQuery(RecordQuery.newBuilder()
398+
.setRecordType("MyNullRecord")
399+
.setFilter(Query.field("string_value").equalsValue("B"))
400+
.build()),
401+
is(Collections.singletonList("two")));
402+
403+
assertThat(executeQuery(RecordQuery.newBuilder()
404+
.setRecordType("MyNullRecord")
405+
.setFilter(Query.field("int_value").isNull())
406+
.build()),
407+
is(Collections.singletonList("empty")));
408+
assertThat(executeQuery(RecordQuery.newBuilder()
409+
.setRecordType("MyNullRecord")
410+
.setFilter(Query.field("int_value").equalsValue(0))
411+
.build()),
412+
is(Collections.singletonList("default")));
413+
assertThat(executeQuery(RecordQuery.newBuilder()
414+
.setRecordType("MyNullRecord")
415+
.setFilter(Query.field("string_value").isNull())
416+
.build()),
417+
is(Collections.singletonList("empty")));
418+
assertThat(executeQuery(RecordQuery.newBuilder()
419+
.setRecordType("MyNullRecord")
420+
.setFilter(Query.field("string_value").equalsValue(""))
421+
.build()),
422+
is(Collections.singletonList("default")));
423+
424+
assertThat(executeQuery(RecordQuery.newBuilder()
425+
.setRecordType("MyNullRecord")
426+
.setSort(Key.Expressions.field("int_value"))
427+
.build()),
428+
is(Arrays.asList("empty", "minus", "default", "one", "two")));
429+
}
430+
}
431+
368432
@Test
369433
public void testCompareSerialization() throws Exception {
370434
final TestRecordsNulls2Proto.MyNullRecord emptyProto2 = buildRecord2("record", null, null);

‎fdb-record-layer-core/src/test/proto/test_records_nulls_3.proto‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,22 @@
1+
/*
2+
* test_records_nulls_3.proto
3+
*
4+
* This source file is part of the FoundationDB open source project
5+
*
6+
* Copyright 2015-2018 Apple Inc. and the FoundationDB project authors
7+
*
8+
* Licensed under the Apache License, Version 2.0 (the "License");
9+
* you may not use this file except in compliance with the License.
10+
* You may obtain a copy of the License at
11+
*
12+
* http://www.apache.org/licenses/LICENSE-2.0
13+
*
14+
* Unless required by applicable law or agreed to in writing, software
15+
* distributed under the License is distributed on an "AS IS" BASIS,
16+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
17+
* See the License for the specific language governing permissions and
18+
* limitations under the License.
19+
*/
120
syntax = "proto3";
221

322
package com.apple.foundationdb.record.testnulls3;

‎fdb-record-layer-core/src/test/proto/test_records_nulls_3_explicit.proto‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
/*
2+
* test_records_nulls_3_explicit.proto
3+
*
4+
* This source file is part of the FoundationDB open source project
5+
*
6+
* Copyright 2026 Apple Inc. and the FoundationDB project authors
7+
*
8+
* Licensed under the Apache License, Version 2.0 (the "License");
9+
* you may not use this file except in compliance with the License.
10+
* You may obtain a copy of the License at
11+
*
12+
* http://www.apache.org/licenses/LICENSE-2.0
13+
*
14+
* Unless required by applicable law or agreed to in writing, software
15+
* distributed under the License is distributed on an "AS IS" BASIS,
16+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
17+
* See the License for the specific language governing permissions and
18+
* limitations under the License.
19+
*/
20+
syntax = "proto3";
21+
22+
package com.apple.foundationdb.record.testnulls3explicit;
23+
24+
option java_package = "com.apple.foundationdb.record";
25+
option java_outer_classname = "TestRecordsNulls3ExplicitProto";
26+
27+
import "record_metadata_options.proto";
28+
29+
message MyNullRecord {
30+
optional string name = 1 [(field).primary_key = true];
31+
optional string string_value = 2 [(field).index = {}];
32+
optional int32 int_value = 3 [(field).index = {}];
33+
}
34+
35+
message RecordTypeUnion {
36+
optional MyNullRecord _MyNullRecord = 1;
37+
}

0 commit comments

Comments
 (0)