Skip to content
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

Incorrect reader #191

Closed
blegat opened this issue Dec 5, 2021 · 3 comments
Closed

Incorrect reader #191

blegat opened this issue Dec 5, 2021 · 3 comments

Comments

@blegat
Copy link
Contributor

blegat commented Dec 5, 2021

#188 broke https://github.com/pszufe/OpenStreetMapX.jl.
The issues seems to be that the reader chosen in

return _read_value(io, jtyp)

only depends on jtyp and not on attrib.ptyp.
However, if jtyp is Int64 for instance, ptyp can either be :int64 or :sint64 or :sfixed64.
The line that is called is:
_read_value(io::IO, ::Type{T}) where T<:Number = read_varint(io, T)

so it assumes that ptyp is always :int64 when jtyp is Int64.
Instead, at:
val = read_field(io, obj, attrib, wiretyp, attrib.jtyp)

attrib.jtyp should be replaced by Int64, FixedSizeNumber{Int64} or SignedNumber{Int64} depending on attrib.ptyp.
The same probably also applies for the lines:

ProtoBuf.jl/src/codec.jl

Lines 466 to 469 in bae7008

if fldnum == 1
key_val[1] = read_field(iob, nothing, attrib, wire_type(K), K)
elseif fldnum == 2
key_val[2] = read_field(iob, nothing, attrib, wire_type(V), V)

@blegat
Copy link
Contributor Author

blegat commented Dec 18, 2021

@neveritt what do you think ?

@neveritt
Copy link
Contributor

@blegat, I think you are right. There is another option which I think was my original intention. The jtyp could be set using the ptyp when the protometa is set.

ProtoBuf.jl/src/codec.jl

Lines 287 to 292 in bae7008

function setprotometa!(meta::ProtoMeta, jtype::Type, ordered::Vector{ProtoMetaAttribs}, oneofs::Vector{Int}, oneof_names::Vector{Symbol})
symdict = Dict{Symbol,ProtoMetaAttribs}()
numdict = Dict{Int,ProtoMetaAttribs}()
for attrib in ordered
symdict[attrib.fld] = numdict[attrib.fldnum] = attrib
end

I will create a test case that catches this error and try first the second approach and then your approach to solve this issue.
In the meantime, @tanmaykm feel free to back my changes out and make a new release if you so wish.

@blegat
Copy link
Contributor Author

blegat commented Apr 14, 2022

Fixed by #193

@blegat blegat closed this as completed Apr 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants