New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Silent failures When Using 'Class.verify' #685

Closed
tomergg opened this Issue Feb 22, 2017 · 6 comments

Comments

Projects
None yet
2 participants
@tomergg

tomergg commented Feb 22, 2017

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);
}
  1. 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

This comment has been minimized.

Owner

dcodeIO commented Feb 22, 2017

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

This comment has been minimized.

tomergg commented Feb 22, 2017

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

This comment has been minimized.

Owner

dcodeIO commented Feb 22, 2017

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

This comment has been minimized.

tomergg commented Feb 22, 2017

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 added a commit that referenced this issue Feb 23, 2017

@dcodeIO dcodeIO added the enhancement label Feb 23, 2017

@dcodeIO

This comment has been minimized.

Owner

dcodeIO commented Feb 28, 2017

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

@dcodeIO dcodeIO closed this Feb 28, 2017

@tomergg

This comment has been minimized.

tomergg commented Feb 28, 2017

Thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment