Skip to content

Commit

Permalink
Set label to REQUIRED for descriptors with LEGACY_REQUIRED feature.
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@zhangskz
zhangskz committed Mar 26, 2024
1 parent b4e1870 commit 4886a9c
Showing 1 changed file with 59 additions and 43 deletions.
102 changes: 59 additions & 43 deletions java/core/src/main/java/com/google/protobuf/Descriptors.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import com.google.protobuf.DescriptorProtos.OneofOptions;
import com.google.protobuf.DescriptorProtos.ServiceDescriptorProto;
import com.google.protobuf.DescriptorProtos.ServiceOptions;
import com.google.protobuf.Descriptors.DescriptorValidationException;
import com.google.protobuf.JavaFeaturesProto.JavaFeatures;
import java.lang.ref.ReferenceQueue;
import java.lang.ref.WeakReference;
Expand Down Expand Up @@ -618,14 +619,18 @@ private FileDescriptor(
}

public void resolveAllFeaturesImmutable() {
resolveAllFeaturesInternal();
try {
resolveAllFeaturesInternal();
} catch (DescriptorValidationException e) {
throw new IllegalArgumentException("Invalid features for \"" + proto.getName() + "\".", e);
}
}

/**
* This method is to be called by generated code only. It resolves features for the descriptor
* and all of its children.
*/
private void resolveAllFeaturesInternal() {
private void resolveAllFeaturesInternal() throws DescriptorValidationException {
if (this.features != null) {
return;
}
Expand Down Expand Up @@ -712,22 +717,26 @@ private void crossLink() throws DescriptorValidationException {
private synchronized void setProto(final FileDescriptorProto proto) {
this.proto = proto;
this.options = null;
this.features = resolveFeatures(proto.getOptions().getFeatures());
try {
this.features = resolveFeatures(proto.getOptions().getFeatures());

for (int i = 0; i < messageTypes.length; i++) {
messageTypes[i].setProto(proto.getMessageType(i));
}
for (int i = 0; i < messageTypes.length; i++) {
messageTypes[i].setProto(proto.getMessageType(i));
}

for (int i = 0; i < enumTypes.length; i++) {
enumTypes[i].setProto(proto.getEnumType(i));
}
for (int i = 0; i < enumTypes.length; i++) {
enumTypes[i].setProto(proto.getEnumType(i));
}

for (int i = 0; i < services.length; i++) {
services[i].setProto(proto.getService(i));
}
for (int i = 0; i < services.length; i++) {
services[i].setProto(proto.getService(i));
}

for (int i = 0; i < extensions.length; i++) {
extensions[i].setProto(proto.getExtension(i));
for (int i = 0; i < extensions.length; i++) {
extensions[i].setProto(proto.getExtension(i));
}
} catch (DescriptorValidationException e) {
throw new IllegalArgumentException("Invalid features for \"" + proto.getName() + "\".", e);
}
}
}
Expand Down Expand Up @@ -1102,7 +1111,7 @@ private Descriptor(
}

/** See {@link FileDescriptor#resolveAllFeatures}. */
private void resolveAllFeatures() {
private void resolveAllFeatures() throws DescriptorValidationException {
this.features = resolveFeatures(proto.getOptions().getFeatures());

for (Descriptor nestedType : nestedTypes) {
Expand Down Expand Up @@ -1162,7 +1171,7 @@ private void validateNoDuplicateFieldNumbers() throws DescriptorValidationExcept
}

/** See {@link FileDescriptor#setProto}. */
private void setProto(final DescriptorProto proto) {
private void setProto(final DescriptorProto proto) throws DescriptorValidationException {
this.proto = proto;
this.options = null;
this.features = resolveFeatures(proto.getOptions().getFeatures());
Expand Down Expand Up @@ -1327,7 +1336,9 @@ public boolean isRequired() {

/** Is this field declared optional? */
public boolean isOptional() {
return proto.getLabel() == FieldDescriptorProto.Label.LABEL_OPTIONAL;
return proto.getLabel() == FieldDescriptorProto.Label.LABEL_OPTIONAL
&& this.features.getFieldPresence()
!= DescriptorProtos.FeatureSet.FieldPresence.LEGACY_REQUIRED;
}

/** Is this field declared repeated? */
Expand Down Expand Up @@ -1732,7 +1743,7 @@ private FieldDescriptor(
}

/** See {@link FileDescriptor#resolveAllFeatures}. */
private void resolveAllFeatures() {
private void resolveAllFeatures() throws DescriptorValidationException {
this.features = resolveFeatures(proto.getOptions().getFeatures());
}

Expand Down Expand Up @@ -1791,6 +1802,19 @@ boolean hasInferredLegacyProtoFeatures() {
return false;
}

@Override
void validateFeatures(FeatureSet features) throws DescriptorValidationException {
if (containingType != null
&& containingType.toProto().getOptions().getMessageSetWireFormat()) {
if (isExtension()) {
if (!isOptional() || getType() != Type.MESSAGE) {
throw new DescriptorValidationException(
this, "Extensions of MessageSets must be optional messages.");
}
}
}
}

/** Look up and cross-link all field types, etc. */
private void crossLink() throws DescriptorValidationException {
if (proto.hasExtendee()) {
Expand Down Expand Up @@ -1962,23 +1986,10 @@ private void crossLink() throws DescriptorValidationException {
}
}
}

if (containingType != null
&& containingType.toProto().getOptions().getMessageSetWireFormat()) {
if (isExtension()) {
if (!isOptional() || getType() != Type.MESSAGE) {
throw new DescriptorValidationException(
this, "Extensions of MessageSets must be optional messages.");
}
} else {
throw new DescriptorValidationException(
this, "MessageSets cannot have fields, only extensions.");
}
}
}

/** See {@link FileDescriptor#setProto}. */
private void setProto(final FieldDescriptorProto proto) {
private void setProto(final FieldDescriptorProto proto) throws DescriptorValidationException {
this.proto = proto;
this.options = null;
this.features = resolveFeatures(proto.getOptions().getFeatures());
Expand Down Expand Up @@ -2250,15 +2261,15 @@ private EnumDescriptor(
}

/** See {@link FileDescriptor#resolveAllFeatures}. */
private void resolveAllFeatures() {
private void resolveAllFeatures() throws DescriptorValidationException {
this.features = resolveFeatures(proto.getOptions().getFeatures());
for (EnumValueDescriptor value : values) {
value.resolveAllFeatures();
}
}

/** See {@link FileDescriptor#setProto}. */
private void setProto(final EnumDescriptorProto proto) {
private void setProto(final EnumDescriptorProto proto) throws DescriptorValidationException {
this.proto = proto;
this.options = null;
this.features = resolveFeatures(proto.getOptions().getFeatures());
Expand Down Expand Up @@ -2402,12 +2413,13 @@ private EnumValueDescriptor(final EnumDescriptor parent, final Integer number) {
}

/** See {@link FileDescriptor#resolveAllFeatures}. */
private void resolveAllFeatures() {
private void resolveAllFeatures() throws DescriptorValidationException {
this.features = resolveFeatures(proto.getOptions().getFeatures());
}

/** See {@link FileDescriptor#setProto}. */
private void setProto(final EnumValueDescriptorProto proto) {
private void setProto(final EnumValueDescriptorProto proto)
throws DescriptorValidationException {
this.proto = proto;
this.options = null;
this.features = resolveFeatures(proto.getOptions().getFeatures());
Expand Down Expand Up @@ -2517,7 +2529,7 @@ private ServiceDescriptor(
}

/** See {@link FileDescriptor#resolveAllFeatures}. */
private void resolveAllFeatures() {
private void resolveAllFeatures() throws DescriptorValidationException {
this.features = resolveFeatures(proto.getOptions().getFeatures());

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

/** See {@link FileDescriptor#setProto}. */
private void setProto(final ServiceDescriptorProto proto) {
private void setProto(final ServiceDescriptorProto proto) throws DescriptorValidationException {
this.proto = proto;
this.options = null;
this.features = resolveFeatures(proto.getOptions().getFeatures());
Expand Down Expand Up @@ -2655,7 +2667,7 @@ private MethodDescriptor(
}

/** See {@link FileDescriptor#resolveAllFeatures}. */
private void resolveAllFeatures() {
private void resolveAllFeatures() throws DescriptorValidationException {
this.features = resolveFeatures(proto.getOptions().getFeatures());
}

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

/** See {@link FileDescriptor#setProto}. */
private void setProto(final MethodDescriptorProto proto) {
private void setProto(final MethodDescriptorProto proto) throws DescriptorValidationException {
this.proto = proto;
this.options = null;
this.features = resolveFeatures(proto.getOptions().getFeatures());
Expand Down Expand Up @@ -2723,7 +2735,7 @@ private GenericDescriptor() {}

public abstract FileDescriptor getFile();

FeatureSet resolveFeatures(FeatureSet unresolvedFeatures) {
FeatureSet resolveFeatures(FeatureSet unresolvedFeatures) throws DescriptorValidationException {
if (this.parent != null
&& unresolvedFeatures.equals(FeatureSet.getDefaultInstance())
&& !hasInferredLegacyProtoFeatures()) {
Expand All @@ -2738,6 +2750,7 @@ FeatureSet resolveFeatures(FeatureSet unresolvedFeatures) {
}
features.mergeFrom(inferLegacyProtoFeatures());
features.mergeFrom(unresolvedFeatures);
validateFeatures(features.build());
return internFeatures(features.build());
}

Expand All @@ -2749,6 +2762,9 @@ boolean hasInferredLegacyProtoFeatures() {
return false;
}

void validateFeatures(FeatureSet features) throws DescriptorValidationException {
}

GenericDescriptor parent;
volatile FeatureSet features;
}
Expand Down Expand Up @@ -3214,11 +3230,11 @@ boolean isSynthetic() {
}

/** See {@link FileDescriptor#resolveAllFeatures}. */
private void resolveAllFeatures() {
private void resolveAllFeatures() throws DescriptorValidationException {
this.features = resolveFeatures(proto.getOptions().getFeatures());
}

private void setProto(final OneofDescriptorProto proto) {
private void setProto(final OneofDescriptorProto proto) throws DescriptorValidationException {
this.proto = proto;
this.options = null;
this.features = resolveFeatures(proto.getOptions().getFeatures());
Expand Down

0 comments on commit 4886a9c

Please sign in to comment.