Skip to content

Commit 4886a9c

Browse files
zhangskzzhangskz
committed
Set label to REQUIRED for descriptors with LEGACY_REQUIRED feature.
Ensures isOptional() does not return true for LEGACY_REQUIRED fields which would otherwise get the optional label applied by default (non-optional fields still get the optional label). Adds validation to feature resolution instead of cross link, which is too early to have FieldPresence.LEGACY_REQUIRED resolved. PiperOrigin-RevId: 618857590
1 parent b4e1870 commit 4886a9c

File tree

1 file changed

+59
-43
lines changed

1 file changed

+59
-43
lines changed

‎java/core/src/main/java/com/google/protobuf/Descriptors.java‎

Lines changed: 59 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@
2929
import com.google.protobuf.DescriptorProtos.OneofOptions;
3030
import com.google.protobuf.DescriptorProtos.ServiceDescriptorProto;
3131
import com.google.protobuf.DescriptorProtos.ServiceOptions;
32+
import com.google.protobuf.Descriptors.DescriptorValidationException;
3233
import com.google.protobuf.JavaFeaturesProto.JavaFeatures;
3334
import java.lang.ref.ReferenceQueue;
3435
import java.lang.ref.WeakReference;
@@ -618,14 +619,18 @@ private FileDescriptor(
618619
}
619620

620621
public void resolveAllFeaturesImmutable() {
621-
resolveAllFeaturesInternal();
622+
try {
623+
resolveAllFeaturesInternal();
624+
} catch (DescriptorValidationException e) {
625+
throw new IllegalArgumentException("Invalid features for \"" + proto.getName() + "\".", e);
626+
}
622627
}
623628

624629
/**
625630
* This method is to be called by generated code only. It resolves features for the descriptor
626631
* and all of its children.
627632
*/
628-
private void resolveAllFeaturesInternal() {
633+
private void resolveAllFeaturesInternal() throws DescriptorValidationException {
629634
if (this.features != null) {
630635
return;
631636
}
@@ -712,22 +717,26 @@ private void crossLink() throws DescriptorValidationException {
712717
private synchronized void setProto(final FileDescriptorProto proto) {
713718
this.proto = proto;
714719
this.options = null;
715-
this.features = resolveFeatures(proto.getOptions().getFeatures());
720+
try {
721+
this.features = resolveFeatures(proto.getOptions().getFeatures());
716722

717-
for (int i = 0; i < messageTypes.length; i++) {
718-
messageTypes[i].setProto(proto.getMessageType(i));
719-
}
723+
for (int i = 0; i < messageTypes.length; i++) {
724+
messageTypes[i].setProto(proto.getMessageType(i));
725+
}
720726

721-
for (int i = 0; i < enumTypes.length; i++) {
722-
enumTypes[i].setProto(proto.getEnumType(i));
723-
}
727+
for (int i = 0; i < enumTypes.length; i++) {
728+
enumTypes[i].setProto(proto.getEnumType(i));
729+
}
724730

725-
for (int i = 0; i < services.length; i++) {
726-
services[i].setProto(proto.getService(i));
727-
}
731+
for (int i = 0; i < services.length; i++) {
732+
services[i].setProto(proto.getService(i));
733+
}
728734

729-
for (int i = 0; i < extensions.length; i++) {
730-
extensions[i].setProto(proto.getExtension(i));
735+
for (int i = 0; i < extensions.length; i++) {
736+
extensions[i].setProto(proto.getExtension(i));
737+
}
738+
} catch (DescriptorValidationException e) {
739+
throw new IllegalArgumentException("Invalid features for \"" + proto.getName() + "\".", e);
731740
}
732741
}
733742
}
@@ -1102,7 +1111,7 @@ private Descriptor(
11021111
}
11031112

11041113
/** See {@link FileDescriptor#resolveAllFeatures}. */
1105-
private void resolveAllFeatures() {
1114+
private void resolveAllFeatures() throws DescriptorValidationException {
11061115
this.features = resolveFeatures(proto.getOptions().getFeatures());
11071116

11081117
for (Descriptor nestedType : nestedTypes) {
@@ -1162,7 +1171,7 @@ private void validateNoDuplicateFieldNumbers() throws DescriptorValidationExcept
11621171
}
11631172

11641173
/** See {@link FileDescriptor#setProto}. */
1165-
private void setProto(final DescriptorProto proto) {
1174+
private void setProto(final DescriptorProto proto) throws DescriptorValidationException {
11661175
this.proto = proto;
11671176
this.options = null;
11681177
this.features = resolveFeatures(proto.getOptions().getFeatures());
@@ -1327,7 +1336,9 @@ public boolean isRequired() {
13271336

13281337
/** Is this field declared optional? */
13291338
public boolean isOptional() {
1330-
return proto.getLabel() == FieldDescriptorProto.Label.LABEL_OPTIONAL;
1339+
return proto.getLabel() == FieldDescriptorProto.Label.LABEL_OPTIONAL
1340+
&& this.features.getFieldPresence()
1341+
!= DescriptorProtos.FeatureSet.FieldPresence.LEGACY_REQUIRED;
13311342
}
13321343

13331344
/** Is this field declared repeated? */
@@ -1732,7 +1743,7 @@ private FieldDescriptor(
17321743
}
17331744

17341745
/** See {@link FileDescriptor#resolveAllFeatures}. */
1735-
private void resolveAllFeatures() {
1746+
private void resolveAllFeatures() throws DescriptorValidationException {
17361747
this.features = resolveFeatures(proto.getOptions().getFeatures());
17371748
}
17381749

@@ -1791,6 +1802,19 @@ boolean hasInferredLegacyProtoFeatures() {
17911802
return false;
17921803
}
17931804

1805+
@Override
1806+
void validateFeatures(FeatureSet features) throws DescriptorValidationException {
1807+
if (containingType != null
1808+
&& containingType.toProto().getOptions().getMessageSetWireFormat()) {
1809+
if (isExtension()) {
1810+
if (!isOptional() || getType() != Type.MESSAGE) {
1811+
throw new DescriptorValidationException(
1812+
this, "Extensions of MessageSets must be optional messages.");
1813+
}
1814+
}
1815+
}
1816+
}
1817+
17941818
/** Look up and cross-link all field types, etc. */
17951819
private void crossLink() throws DescriptorValidationException {
17961820
if (proto.hasExtendee()) {
@@ -1962,23 +1986,10 @@ private void crossLink() throws DescriptorValidationException {
19621986
}
19631987
}
19641988
}
1965-
1966-
if (containingType != null
1967-
&& containingType.toProto().getOptions().getMessageSetWireFormat()) {
1968-
if (isExtension()) {
1969-
if (!isOptional() || getType() != Type.MESSAGE) {
1970-
throw new DescriptorValidationException(
1971-
this, "Extensions of MessageSets must be optional messages.");
1972-
}
1973-
} else {
1974-
throw new DescriptorValidationException(
1975-
this, "MessageSets cannot have fields, only extensions.");
1976-
}
1977-
}
19781989
}
19791990

19801991
/** See {@link FileDescriptor#setProto}. */
1981-
private void setProto(final FieldDescriptorProto proto) {
1992+
private void setProto(final FieldDescriptorProto proto) throws DescriptorValidationException {
19821993
this.proto = proto;
19831994
this.options = null;
19841995
this.features = resolveFeatures(proto.getOptions().getFeatures());
@@ -2250,15 +2261,15 @@ private EnumDescriptor(
22502261
}
22512262

22522263
/** See {@link FileDescriptor#resolveAllFeatures}. */
2253-
private void resolveAllFeatures() {
2264+
private void resolveAllFeatures() throws DescriptorValidationException {
22542265
this.features = resolveFeatures(proto.getOptions().getFeatures());
22552266
for (EnumValueDescriptor value : values) {
22562267
value.resolveAllFeatures();
22572268
}
22582269
}
22592270

22602271
/** See {@link FileDescriptor#setProto}. */
2261-
private void setProto(final EnumDescriptorProto proto) {
2272+
private void setProto(final EnumDescriptorProto proto) throws DescriptorValidationException {
22622273
this.proto = proto;
22632274
this.options = null;
22642275
this.features = resolveFeatures(proto.getOptions().getFeatures());
@@ -2402,12 +2413,13 @@ private EnumValueDescriptor(final EnumDescriptor parent, final Integer number) {
24022413
}
24032414

24042415
/** See {@link FileDescriptor#resolveAllFeatures}. */
2405-
private void resolveAllFeatures() {
2416+
private void resolveAllFeatures() throws DescriptorValidationException {
24062417
this.features = resolveFeatures(proto.getOptions().getFeatures());
24072418
}
24082419

24092420
/** See {@link FileDescriptor#setProto}. */
2410-
private void setProto(final EnumValueDescriptorProto proto) {
2421+
private void setProto(final EnumValueDescriptorProto proto)
2422+
throws DescriptorValidationException {
24112423
this.proto = proto;
24122424
this.options = null;
24132425
this.features = resolveFeatures(proto.getOptions().getFeatures());
@@ -2517,7 +2529,7 @@ private ServiceDescriptor(
25172529
}
25182530

25192531
/** See {@link FileDescriptor#resolveAllFeatures}. */
2520-
private void resolveAllFeatures() {
2532+
private void resolveAllFeatures() throws DescriptorValidationException {
25212533
this.features = resolveFeatures(proto.getOptions().getFeatures());
25222534

25232535
for (MethodDescriptor method : methods) {
@@ -2532,7 +2544,7 @@ private void crossLink() throws DescriptorValidationException {
25322544
}
25332545

25342546
/** See {@link FileDescriptor#setProto}. */
2535-
private void setProto(final ServiceDescriptorProto proto) {
2547+
private void setProto(final ServiceDescriptorProto proto) throws DescriptorValidationException {
25362548
this.proto = proto;
25372549
this.options = null;
25382550
this.features = resolveFeatures(proto.getOptions().getFeatures());
@@ -2655,7 +2667,7 @@ private MethodDescriptor(
26552667
}
26562668

26572669
/** See {@link FileDescriptor#resolveAllFeatures}. */
2658-
private void resolveAllFeatures() {
2670+
private void resolveAllFeatures() throws DescriptorValidationException {
26592671
this.features = resolveFeatures(proto.getOptions().getFeatures());
26602672
}
26612673

@@ -2682,7 +2694,7 @@ private void crossLink() throws DescriptorValidationException {
26822694
}
26832695

26842696
/** See {@link FileDescriptor#setProto}. */
2685-
private void setProto(final MethodDescriptorProto proto) {
2697+
private void setProto(final MethodDescriptorProto proto) throws DescriptorValidationException {
26862698
this.proto = proto;
26872699
this.options = null;
26882700
this.features = resolveFeatures(proto.getOptions().getFeatures());
@@ -2723,7 +2735,7 @@ private GenericDescriptor() {}
27232735

27242736
public abstract FileDescriptor getFile();
27252737

2726-
FeatureSet resolveFeatures(FeatureSet unresolvedFeatures) {
2738+
FeatureSet resolveFeatures(FeatureSet unresolvedFeatures) throws DescriptorValidationException {
27272739
if (this.parent != null
27282740
&& unresolvedFeatures.equals(FeatureSet.getDefaultInstance())
27292741
&& !hasInferredLegacyProtoFeatures()) {
@@ -2738,6 +2750,7 @@ FeatureSet resolveFeatures(FeatureSet unresolvedFeatures) {
27382750
}
27392751
features.mergeFrom(inferLegacyProtoFeatures());
27402752
features.mergeFrom(unresolvedFeatures);
2753+
validateFeatures(features.build());
27412754
return internFeatures(features.build());
27422755
}
27432756

@@ -2749,6 +2762,9 @@ boolean hasInferredLegacyProtoFeatures() {
27492762
return false;
27502763
}
27512764

2765+
void validateFeatures(FeatureSet features) throws DescriptorValidationException {
2766+
}
2767+
27522768
GenericDescriptor parent;
27532769
volatile FeatureSet features;
27542770
}
@@ -3214,11 +3230,11 @@ boolean isSynthetic() {
32143230
}
32153231

32163232
/** See {@link FileDescriptor#resolveAllFeatures}. */
3217-
private void resolveAllFeatures() {
3233+
private void resolveAllFeatures() throws DescriptorValidationException {
32183234
this.features = resolveFeatures(proto.getOptions().getFeatures());
32193235
}
32203236

3221-
private void setProto(final OneofDescriptorProto proto) {
3237+
private void setProto(final OneofDescriptorProto proto) throws DescriptorValidationException {
32223238
this.proto = proto;
32233239
this.options = null;
32243240
this.features = resolveFeatures(proto.getOptions().getFeatures());

0 commit comments

Comments
 (0)