feat: add parsing for new links format #3665
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
Pull Request Checklist
|
Jenkins BuildsClick to see older builds (121)
|
MishkaRogachev
force-pushed
the
feat/generate-and-parse-share-urls
branch
from
6217aed
to
4943f37
Compare
3 tasks
MishkaRogachev
commented
Jun 26, 2023
MishkaRogachev
force-pushed
the
feat/generate-and-parse-share-urls
branch
2 times, most recently
from
ad0d5a2
to
4f4ad50
Compare
Samyoul
reviewed
Jun 28, 2023
|
Ok @MishkaRogachev @borismelnik this actually fixes the functionality and the tests pass. git apply ./fix.patchOr I can push this as a commit myself. diff --git a/protocol/messenger_share_urls_test.go b/protocol/messenger_share_urls_test.go
index 129c6a4f8..54d9a665b 100644
--- a/protocol/messenger_share_urls_test.go
+++ b/protocol/messenger_share_urls_test.go
@@ -112,14 +112,19 @@ func (s *MessengerShareUrlsSuite) createCommunityWithChannel() (*communities.Com
}
func (s *MessengerShareUrlsSuite) TestDecodeEncodeDataURL() {
- testData := []byte("test data 123")
+ ts := [][]byte{
+ []byte("test data 123"),
+ []byte("test data 123test data 123test data 123test data 123test data 123"),
+ }
- encodedData, err := urls.EncodeDataURL(testData)
- s.Require().NoError(err)
+ for i := range ts {
+ encodedData, err := urls.EncodeDataURL(ts[i])
+ s.Require().NoError(err)
- decodedData, err := urls.DecodeDataURL(encodedData)
- s.Require().NoError(err)
- s.Require().Equal(testData, decodedData)
+ decodedData, err := urls.DecodeDataURL(encodedData)
+ s.Require().NoError(err)
+ s.Require().Equal(ts[i], decodedData)
+ }
}
func (s *MessengerShareUrlsSuite) TestSerializePublicKey() {
diff --git a/protocol/urls/utils.go b/protocol/urls/utils.go
index 00ba40942..124ad4e6a 100644
--- a/protocol/urls/utils.go
+++ b/protocol/urls/utils.go
@@ -61,7 +61,11 @@ func encodeDataURL(data []byte) (string, error) {
bb := bytes.NewBuffer([]byte{})
writer := brotli.NewWriter(bb)
_, err := writer.Write(data)
- writer.Close()
+ if err != nil {
+ return "", err
+ }
+
+ err = writer.Close()
if err != nil {
return "", err
}
@@ -71,17 +75,17 @@ func encodeDataURL(data []byte) (string, error) {
func decodeDataURL(data string) ([]byte, error) {
decoded, err := base64.URLEncoding.DecodeString(data)
-
if err != nil {
return nil, err
}
- var output []byte
+
+ output := make([]byte, 4096)
bb := bytes.NewBuffer(decoded)
reader := brotli.NewReader(bb)
- _, err = reader.Read(output)
+ n, err := reader.Read(output)
if err != nil {
return nil, err
}
- return output, nil
+ return output[:n], nil
} |
|
Decoding the url data seems to be working fine for me, but I'm getting the following error:
Detailed output: I've tried to unmarshal this data into the
Parsed using the following code: func (s *MessengerShareUrlsSuite) TestDecodeURLData() {
us := "https://status.app/c/iyKACkQKB0Rvb2RsZXMSJ0NvbG9yaW5nIHRoZSB3b3JsZCB3aXRoIGpveSDigKIg4bSXIOKAohiYohsiByMxMzFEMkYqAwEhMwM=#QnO2hw5tVAE-MD5i8Mq4s_oGWd0rRFmlObgRUNQXtUNwP1ixFw914Y4-BQpF08KOqpaULCh7UCtlWR-O0YP8MwA="
u, err := url.Parse(us)
s.Require().NoError(err)
decodedData, err := urls.DecodeDataURL(u.Path[3:])
s.Require().NoError(err)
spew.Dump(decodedData, base64.StdEncoding.EncodeToString(decodedData))
cu := &protobuf.Community{}
err = proto.Unmarshal(decodedData, cu)
s.Require().NoError(err)
spew.Dump(cu)
}OOOOoor the data is corrupted, which I'm going to presume for the moment that it isn't.
|
|
Hmm, I've noticed something funny: expect(
(
await createCommunityURLWithData(
{
description: community.description.identity!.description,
displayName: community.description.identity!.displayName,
color: community.description.identity!.color,
membersCount: 446_744,
tagIndices: [1, 33, 51],
},
community.privateKey
)
).toString()
).toBe(This does not have the same field order as the protobuf: message Community {
string display_name = 1;
string description = 2;
uint32 members_count = 3;
string color = 4;
repeated uint32 tag_indices = 5;
}status-go/protocol/protobuf/url_data.proto Lines 6 to 12 in a566c91 Which could explain the type error above. Follow up, no it doesn't the decoded protobuf details below: #3665 (comment) |
@Samyoul, this patch works fine! Many thanks! 
|
|
I've ran the following byte output through a generic protobuf decoder and I get the following result:
Which looks fine up until the last field: tagIndices: [1, 33, 51],repeated uint32 tag_indices = 5; |
MishkaRogachev
force-pushed
the
feat/generate-and-parse-share-urls
branch
from
04cce5e
to
6ed874c
Compare
MishkaRogachev
force-pushed
the
feat/generate-and-parse-share-urls
branch
from
6ed874c
to
5b12a63
Compare
MishkaRogachev
force-pushed
the
feat/generate-and-parse-share-urls
branch
from
368b05c
to
78e5377
Compare
MishkaRogachev
force-pushed
the
feat/generate-and-parse-share-urls
branch
from
78e5377
to
e9594e8
Compare
|
For reviewers: the most significant changes located in |
borismelnik
approved these changes
Jun 29, 2023
This was referenced Jun 29, 2023
Samyoul
previously requested changes
Jun 29, 2023
Samyoul
reviewed
Jun 29, 2023
| @@ -0,0 +1,411 @@ | |||
| package protocol | |||
There was a problem hiding this comment.
Is it possible to have some tests that parse expected url strings? This way we can ensure breaking changes to functionality gets addressed.
There was a problem hiding this comment.
This is actually quite close to what I was thinking.
url := fmt.Sprintf("%s/c/%s#%s", baseShareURL, communityData, signature)
Samyoul
reviewed
Jun 29, 2023
Comment on lines
+503
to
+505
| if strings.HasPrefix(urlContents[0], "c/") { | ||
| return m.parseCommunityURLWithData(strings.TrimPrefix(urlContents[0], "c/"), urlContents[1]) | ||
| } |
There was a problem hiding this comment.
Would c/#urlfragment be a valid url that would be mis-parsed?
There was a problem hiding this comment.
Samyoul
reviewed
Jun 29, 2023
Samyoul
dismissed
their stale review
Because I was wrong, there is a whole big file of tests.
Samyoul
approved these changes
Jun 30, 2023
|
@Samyoul @felicio @borismelnik I tried to add some new tests to see if we can parse urls generated by Status Web: And it seems something wrong in our |
MishkaRogachev
force-pushed
the
feat/generate-and-parse-share-urls
branch
from
419e857
to
d8c2f1f
Compare
Opened an issue for that: #3713 |
3 tasks
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.
For status-im/status-desktop#10851
Implement parsing shared urls in a new format
Important changes: