nested packed enums not handled properly

[tyrasd]

I'm trying to read a pbf with a packed repeated enum field (as defined here. The compiled pbf parser seems odd for that field:

Relation.read = function (pbf, end) {
    return pbf.readFields(Relation._readField, {id: 0, keys: [], vals: [], info: null, roles_sid: [], memids: [], types: 0}, end);
};
Relation._readField = function (tag, obj, pbf) {
    […]
    else if (tag === 10) obj.types.push(pbf.readVarint());
};

Shouldn't the default value for the types field be an empty array [] (instead of 0), and the data be read via something like pbf.readPackedVarint(obj.types); (instead of obj.types.push(pbf.readVarint());)?

[kjvalencik]

The issue is here. https://github.com/mapbox/pbf/blob/master/compile.js#L270
That line should be

if (enumValues && !field.repeated)

I have a fix and test on a fork that I forgot about. I'll submit a PR tomorrow.

[mourner]

@kjvalencik thanks for such a quick fix!
@tyrasd pushed as v3.0.3, can you confirm that it fixes the issue?

[tyrasd]

@tyrasd pushed as v3.0.3, can you confirm that it fixes the issue?

Not entirely. The default value is fine now, but the data is still read via an obj.types.push(pbf.readVarint()) (as if the field were not defined as [packed = true]), which produces wrong results (and eventually crashes the parsing). By manually changing that to pbf.readPackedVarint(obj.types); (which is to be expected from a packed repeated field) everything works fine.

One cause seems to be the delete field.options.packed; in https://github.com/mapbox/pbf/blob/master/compile.js#L257 (introduced for #57), which overrides the previously present packed flag of the repeated enum field. Another odd line is https://github.com/mapbox/pbf/blob/master/compile.js#L112 (originating from 7929f12), which should be more similar to how that's implemented in compileRepeatedWrite a couple of lines below (and, at first glance L111 seems a bit out of place as well).

Here's a very rough patch that works for me and passes all tests (no guarantees, though): tyrasd@4591228

[kjvalencik]

Thanks @tyrasd. Can you give this branch a try? https://github.com/mapbox/pbf/tree/kv/fix-repeated-enums

[tyrasd]

Yes, the solution in #71 works for me. Thanks!

[mourner]

Released as 3.0.4.