Skip to content

Commit

Permalink
Merge pull request #16308 from protocolbuffers/cp-26x-3
Browse files Browse the repository at this point in the history 
Set label to REQUIRED for descriptors with LEGACY_REQUIRED feature.
  • Loading branch information
@zhangskz
zhangskz authored Mar 26, 2024
2 parents b752bc2 + 9bf69ec commit 49253b1
Show file tree
Hide file tree
Showing 2 changed files with 80 additions and 60 deletions.
138 changes: 78 additions & 60 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 All @@ -634,7 +639,7 @@ private void resolveAllFeaturesInternal() {
if (this.features != null) {
return;
}
this.features = resolveFeatures(proto.getOptions().getFeatures());
resolveFeatures(proto.getOptions().getFeatures());

for (Descriptor messageType : messageTypes) {
messageType.resolveAllFeatures();
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 {
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,8 +1111,8 @@ private Descriptor(
}

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

for (Descriptor nestedType : nestedTypes) {
nestedType.resolveAllFeatures();
Expand Down Expand Up @@ -1162,10 +1171,10 @@ 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());
resolveFeatures(proto.getOptions().getFeatures());

for (int i = 0; i < nestedTypes.length; i++) {
nestedTypes[i].setProto(proto.getNestedType(i));
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,8 +1743,8 @@ private FieldDescriptor(
}

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

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

@Override
void validateFeatures() 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,26 +1986,13 @@ 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());
resolveFeatures(proto.getOptions().getFeatures());
}

/** For internal use only. This is to satisfy the FieldDescriptorLite interface. */
Expand Down Expand Up @@ -2250,18 +2261,19 @@ private EnumDescriptor(
}

/** See {@link FileDescriptor#resolveAllFeatures}. */
private void resolveAllFeatures() {
this.features = resolveFeatures(proto.getOptions().getFeatures());
private void resolveAllFeatures() throws DescriptorValidationException {
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());
resolveFeatures(proto.getOptions().getFeatures());

for (int i = 0; i < values.length; i++) {
values[i].setProto(proto.getValue(i));
Expand Down Expand Up @@ -2402,15 +2414,16 @@ private EnumValueDescriptor(final EnumDescriptor parent, final Integer number) {
}

/** See {@link FileDescriptor#resolveAllFeatures}. */
private void resolveAllFeatures() {
this.features = resolveFeatures(proto.getOptions().getFeatures());
private void resolveAllFeatures() throws DescriptorValidationException {
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());
resolveFeatures(proto.getOptions().getFeatures());
}
}

Expand Down Expand Up @@ -2517,8 +2530,8 @@ private ServiceDescriptor(
}

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

for (MethodDescriptor method : methods) {
method.resolveAllFeatures();
Expand All @@ -2532,10 +2545,10 @@ 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());
resolveFeatures(proto.getOptions().getFeatures());

for (int i = 0; i < methods.length; i++) {
methods[i].setProto(proto.getMethod(i));
Expand Down Expand Up @@ -2655,8 +2668,8 @@ private MethodDescriptor(
}

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

private void crossLink() throws DescriptorValidationException {
Expand All @@ -2682,10 +2695,10 @@ 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());
resolveFeatures(proto.getOptions().getFeatures());
}
}

Expand Down Expand Up @@ -2723,11 +2736,13 @@ private GenericDescriptor() {}

public abstract FileDescriptor getFile();

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

FeatureSet inferLegacyProtoFeatures() {
Expand All @@ -2749,6 +2765,8 @@ boolean hasInferredLegacyProtoFeatures() {
return false;
}

void validateFeatures() throws DescriptorValidationException {}

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

/** See {@link FileDescriptor#resolveAllFeatures}. */
private void resolveAllFeatures() {
this.features = resolveFeatures(proto.getOptions().getFeatures());
private void resolveAllFeatures() throws DescriptorValidationException {
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());
resolveFeatures(proto.getOptions().getFeatures());
}

private OneofDescriptor(
Expand Down
2 changes: 2 additions & 0 deletions ruby/src/main/java/com/google/protobuf/jruby/RubyFieldDescriptor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -287,6 +287,8 @@ protected FieldDescriptor getDescriptor() {
private void calculateLabel(ThreadContext context) {
if (descriptor.isRepeated()) {
this.label = context.runtime.newSymbol("repeated");
} else if (descriptor.isRequired()) {
this.label = context.runtime.newSymbol("required");
} else if (descriptor.isOptional()) {
this.label = context.runtime.newSymbol("optional");
} else {
Expand Down

0 comments on commit 49253b1

Please sign in to comment.