Silent failures When Using 'Class.verify'

[tomergg]

protobuf.js version: 6.6.3

I'm working on upgrading our pbjs from 5.x to 6.x.
The code is a lot easier to use and I was able to remove quite a bit of custom wrapper code.
So far I ran into 2 issues:

1. verify works sometimes. It seems to fail silently, even if I pass "junk" in

const rootNamespace = protobuf.Root.fromJSON(pbJson);
const pbDef = rootNamespace.looup('Pb.MyDef');
const verifyResult = pbDef[pbClassName].verify(123);
if (verifyResult) {
  throw new Error(verifyResult);
}

2. When decoding, I can specify { enums: String } which is great, but when encoding enums can be specified only by integer. Is there a plan to re-introduce support for encoding enum strings?

Thank you @dcodeIO for all the great work.

[dcodeIO]

For reference: verifier

As it is currently, .verify (just like .encode) expects a non-null object as the parameter. It does not check this condition explicitly. This is done to reduce redundant assertions for perf reasons. but I see that this can be misleading and is thus discussable (for .verify).

Regarding { enums: String }: .fromObject, which is the counterpart of .toObject, creates a runtime message from anything, also enums specified as strings. Internally, encoders expect numeric values, again for perf reasons.

[tomergg]

Thank you for the quick response.

1. It makes sense that .verify would expect non-null objects. The issue that I'm seeing is that some times I will get a detailed error for a "bad" object, and sometimes it will just except anything other than null.

2. Thank you for the clarification about the enum conversion. I got it working with the following pattern:

const rootNamespace = protobuf.Root.fromJSON(pbJson);
const pbDef = rootNamespace.looup('MyDef');
const writer = protobuf.Writer.create();
pbDef.encodeDelimited(pbDef.fromObject(myObject), writer);
const buffer = writer.finish();

Is this matching the poet's intention?
Perhaps something similar can be added as @examples to communicate the common usage patterns

[dcodeIO]

Regarding 1.: It only checks whether the object will encode properly. That means it only checks actual field values and no additional properties. For example, if you have a required field on your message, and that's not set, then it will always return an error. If all properties are optional, it won't for anything that can be accessed like an object, like a number.

On 2. Exactly, yes.

[tomergg]

I'm happy to report that I successfully upgraded from v5 to v6 and all our tests are passing. We have a very large code base that relies heavily on protobuf. I've added a gulp task to compile the proto files to a JSON (~500KB). I've also tried static-module, but the file size was over 7MB and I didn't see any benefits.

For future reference, here's my gulp code:

gulp.task('protobuf', (callback) => {
  const pbSrc = path.join(__dirname, '../..', PROTO_SOURCE_PATH);
  const pbDest = path.join(__dirname, '../..', OUTPUT_FILE_PATH);
  const options = ['--target', 'json'];
  const command = [...options, '--path', pbSrc, '--out', pbDest, ...protobufFiles];

  log('generating static JS code from proto files');
  log('pbjs command: ', command.join(' '));

  pbjs.main(
    command,
    (error) => {
      if (error) {
        log('error: ', error);
        callback(error);
        return;
      }

      callback();
    });
});

The issue I'm seeing with the .verify is that in some cases it will accept almost anything, not just additional properties. In the example, in my first comment, I pass in a Number instead of an Object and .verify approves.

Another issue that I've encountered during the upgrade is that v6 is more sensitive to the 'flavor' of base64 strings. I had to modify my code and transform the strings into a non-url-safe flavor. It would be nice if that was covered by the ConversionOptions for .decode, and if the .encode would be more "forgiving". I hope that I'll be able to find some time to submit a PR for it.

[dcodeIO]

This should be fixed with v6.6.4. Feel free to reopen if there are still any issues!

[tomergg]

Thank you