Optional support of msgpack.v5 #174
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
oleg-jukovec
force-pushed
the
oleg-jukovec/gh-124-msgpack-v5
branch
6 times, most recently
from
99637ab
to
d5a0c38
Compare
|
Changes:
It is ready for review now. |
oleg-jukovec
force-pushed
the
oleg-jukovec/gh-124-msgpack-v5
branch
4 times, most recently
from
72a16f9
to
a49009b
Compare
vr009
approved these changes
Jun 6, 2022
oleg-jukovec
force-pushed
the
oleg-jukovec/gh-124-msgpack-v5
branch
3 times, most recently
from
901dda2
to
3bb200d
Compare
oleg-jukovec
force-pushed
the
oleg-jukovec/gh-124-msgpack-v5
branch
4 times, most recently
from
ebd9208
to
1e22635
Compare
oleg-jukovec
force-pushed
the
oleg-jukovec/gh-124-msgpack-v5
branch
from
1e22635
to
8c49242
Compare
oleg-jukovec
force-pushed
the
oleg-jukovec/gh-124-msgpack-v5
branch
from
8c49242
to
271a891
Compare
oleg-jukovec
force-pushed
the
oleg-jukovec/gh-124-msgpack-v5
branch
8 times, most recently
from
497d980
to
72eb5b9
Compare
oleg-jukovec
force-pushed
the
oleg-jukovec/gh-124-msgpack-v5
branch
from
72eb5b9
to
827612b
Compare
DifferentialOrange
approved these changes
Aug 3, 2022
oleg-jukovec
force-pushed
the
oleg-jukovec/gh-124-msgpack-v5
branch
from
827612b
to
51a5b60
Compare
DifferentialOrange
approved these changes
Aug 3, 2022
EncodeSliceLen[1] and DecodeSliceLen[2] are marked as deprecated in msgpack v2.9.2. The patch replaces it with EncodeArrayLen and DecodeArrayLen. 1. https://pkg.go.dev/github.com/vmihailenco/msgpack@v2.9.2+incompatible#Encoder.EncodeSliceLen 2. https://pkg.go.dev/github.com/vmihailenco/msgpack@v2.9.2+incompatible#Decoder.DecodeSliceLen Part of #124
We use everywhere EncodeInt instead of EncodeInt64. Also we use everywhere EncodeUint64 instead of EncodeUint. It can be confusing. Although EncodeUint64 and EncodeUint have same logic in msgpack.v2, but different in msgpack.v5. It's good for migration to msgpack.v5 too. Part of #124.
When we decode the schema with msgpack it allows us to handle errors better. For example, incorrect type conversion leads to a runtime error. Now it will be an usual error that we can handle by our code. Also it will help in migration to msgpack.v5, because an interface decoding rules by default have changed in msgpack.v5. Part of #124 Co-authored-by: Oleg Utkin <oleg_utkin@me.com>
The patch replaces the msgpack code by internal wrappers. The msgpack usage have been extracted to msgpack.go file for the code and to msgpack_helper_test.go for tests. It is the same logic for submodules. Part of #124
EncodeUint and EncodeInt have different argument types in msgpack.v2[1][2] and msgpack.v5[3][4]. The wrappers will simplify migration to msgpack.v5. 1. https://pkg.go.dev/github.com/vmihailenco/msgpack@v2.9.2+incompatible#Encoder.EncodeUint 2. https://pkg.go.dev/github.com/vmihailenco/msgpack@v2.9.2+incompatible#Encoder.EncodeInt 3. https://pkg.go.dev/github.com/vmihailenco/msgpack/v5#Encoder.EncodeUint 4. https://pkg.go.dev/github.com/vmihailenco/msgpack/v5#Encoder.EncodeInt Part of #124
The msgpack.v5 code located in msgpack_v5.go and msgpack_v5_helper_test.go for tests. It is the same logic for submodules. An user can use msgpack.v5 with a build tag: go_tarantool_msgpack_v5 The commit also unify typo conversions after decoding in tests. This is necessary because msgpack v2.9.2 decodes all numbers as uint[1] or int[2], but msgpack.v5 decodes numbers as a MessagePack type[3]. There are additional differences when decoding types. 1. https://github.com/vmihailenco/msgpack/blob/23644a15054d8b9f8a9fca3e041f3419504b9c21/decode.go#L283 2. https://github.com/vmihailenco/msgpack/blob/23644a15054d8b9f8a9fca3e041f3419504b9c21/decode.go#L285 3. https://github.com/vmihailenco/msgpack/blob/233c977ae92b215a9aa7eb420bbe67da99f05ab6/decode.go#L410 Part of #124
oleg-jukovec
force-pushed
the
oleg-jukovec/gh-124-msgpack-v5
branch
from
51a5b60
to
67653e9
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I found incompatibilities between msgpack.v2 and msgpack.v5 which affect our code:
EncodeUint64/EncodeInt64 logic:
msgpack.v2
msgpack.v5
EncodeInt/EncodeUint signature:
https://pkg.go.dev/github.com/vmihailenco/msgpack@v2.9.2+incompatible#Encoder.EncodeUint
https://pkg.go.dev/github.com/vmihailenco/msgpack@v2.9.2+incompatible#Encoder.EncodeInt
https://pkg.go.dev/github.com/vmihailenco/msgpack/v5#Encoder.EncodeUint
https://pkg.go.dev/github.com/vmihailenco/msgpack/v5#Encoder.EncodeInt
RegisterExt logic and signature:
msgpack.v2
msgpack.v5
Decode Map logic:
[decode] regression from v4:
invalid code=d3 decoding string/bytes lengthon complex nested structure vmihailenco/msgpack#327https://msgpack.uptrace.dev/guide/#decoding-maps-into-an-interface/
Decode numbers logic:
msgpack.v2
msgpack.v5
Most of the problems can be solved easily and do not affect a
go-tarantoolclients code (1, 2, 3, 4), but it is impossible to get the same decoding logic for all cases (5). The changes will break the client code.The pull request solves problems for the
go-tarantoolcode and providemsgpack.v5as an option for the current version. In the future,msgpack.v5may be used by default.I didn't forget about (remove if it is not applicable):
Related issues:
Closes #124