Fixes for complex predefined rule types #210
Conversation
ghost
requested review from
| @@ -243,7 +243,7 @@ private Env getRuleEnv( | |||
| Variable.RULES_NAME, | |||
| Decls.newObjectType(constraintMessage.getDescriptorForType().getFullName())), | |||
| Decls.newVar( | |||
| Variable.RULE_NAME, DescriptorMappings.getCELType(constraintFieldDesc, forItems)))); | |||
| Variable.RULE_NAME, DescriptorMappings.getCELType(constraintFieldDesc, false)))); | |||
There was a problem hiding this comment.
Explanation: The forItems in this context is for the field, not the rule. We never act on the nested items of the rule individually, so forItems should always be false for the rule type.
| return Decls.newWrapperType(Decls.String); | ||
| case "google.protobuf.UInt32Value": | ||
| case "google.protobuf.UInt64Value": | ||
| return Decls.newWrapperType(Decls.Uint); |
There was a problem hiding this comment.
Explanation: In most cases, the wrapper type is "unwrapped" by having its constraints processed in processWrapperConstraints; in other cases (e.g. nested in a message), it is handled by cel-java. However, in the case of the rule, we might be dealing with a wrapper or repeated wrapper directly. Therefore,getCELType needs to be aware of wrappers.
| default: | ||
| return Decls.newObjectType(fieldDescriptor.getFullName()); | ||
| return Decls.newObjectType(fqn); |
There was a problem hiding this comment.
Explanation: newObjectType sets the typeName field passed to setMessageType. I think in most cases this field doesn't actually matter (presumably because cel-java is doing protobuf reflection on the runtime value anyways) but having this set wrong makes the error messages unintelligible. Set it to the fqn of the message type instead of the field descriptor full name.
ghost
changed the title
1 participant
Updates the protovalidate version to 0.8.2 and fixes three closely-related bugs:
ruleis inadvertently typed incorrectly when it is a repeated rule field on a repeated fieldgetCELTypeinadvertently returns an incorrectly-named message type when dealing with a message field (using the full name of the field instead of the full name of the type)getCELTypereturns a message type instead of a wrapper type for wrappers likegoogle.protobuf.Int32ValueThese bugs are subtle because they don't occur in normal circumstances, but the new predefined rule tests added in v0.8.2 to close the bugs on protovalidate-go inadvertently revealed several protovalidate-java bugs. This validates that more work on the conformance test can still be quite valuable in finding difficult bugs, even if we don't know exactly what we're looking for.