non utf8 support for asset strings #577

winder · 2021-07-17T13:42:35Z

Summary

If asset strings contain non-printable characters they are stored in a byte array.
Return -b64 asset strings in most endpoints.

Test Plan

New unit test

This reverts commit 131825c.

types/types.go

tsachiherman · 2021-07-19T20:20:10Z

idb/postgres/internal/encoding/encode.go

+	ap.AssetName = strings.ReplaceAll(ap.AssetName, "\000", "")
+	ap.UnitName = strings.ReplaceAll(ap.UnitName, "\000", "")
+	ap.URL = strings.ReplaceAll(ap.URL, "\000", "")


This looks incorrect, but I'm probably not understanding the context :
going from imported AssetParams to persisted AssetParams should base64 the result into the "XXXBytes" variant only when needed. Otherwise, it should remain in the original variable.

This part is for the transaction's JSON format, these fields happen to be unused but I left it in like this in case Anya wants to use it.

idb/postgres/postgres.go

idb/postgres/internal/encoding/encode.go

idb/postgres/postgres.go

api/handlers.go

tsachiherman · 2021-07-20T16:04:54Z

api/handlers.go

+
+		// In case the DB layer filled the name with non-printable utf8
+		if asset.Params.Name != nil {
+			name := util.PrintableUTF8OrEmpty(*asset.Params.Name)


while it's ok to add this, I would suggest you'll add a logging here to indicate that it happened, as it implies there is an issue elsewhere.

I was imagining another DB implemented the API. I don't think it's strictly an issue for the DB layer to ignore this presentation requirement. For Postgres, the tests verify that this case would never be triggered.

codecov-commenter · 2021-07-20T18:11:26Z

Codecov Report

Merging #577 (bcb65e0) into develop (d45623b) will increase coverage by 3.34%.
The diff coverage is 64.21%.

@@             Coverage Diff             @@
##           develop     #577      +/-   ##
===========================================
+ Coverage    43.80%   47.14%   +3.34%     
===========================================
  Files           24       25       +1     
  Lines         3801     3873      +72     
===========================================
+ Hits          1665     1826     +161     
+ Misses        1873     1767     -106     
- Partials       263      280      +17

Impacted Files	Coverage Δ
api/handlers.go	`3.68% <0.00%> (-0.13%)`	⬇️
types/types.go	`0.00% <0.00%> (ø)`
idb/postgres/internal/encoding/decode.go	`58.82% <58.82%> (ø)`
idb/postgres/internal/encoding/encode.go	`96.00% <90.90%> (+2.89%)`	⬆️
api/converter_utils.go	`86.15% <100.00%> (+0.12%)`	⬆️
idb/postgres/postgres.go	`42.49% <100.00%> (+6.12%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d45623b...bcb65e0. Read the comment docs.

tolikzinovyev · 2021-07-20T22:03:21Z

Looks good overall. Left some comments in the code. Could you also add tests in idb/postgres/internal/encoding similar to https://github.com/algorand/indexer/tree/tolik/acct-rewrite4/idb/postgres/internal/encoding?

tolikzinovyev · 2021-07-20T21:44:30Z

idb/postgres/internal/encoding/encode.go

+		URLBytes:       []byte(params.URL),
+	}
+
+	ret.AssetName = util.PrintableUTF8OrEmpty(params.AssetName)


Might be simpler to do:

if params.AssetName is not printable { params.AssetNameBytes = ... params.AssetName = nil }

I copied this function from the go-algorand implementation, it lets us skip 16 if statements which is pretty nice. Some discussion about the different versions here if you're curious: algorand/go-algorand#2555 (comment)

Yes, most of the times it is better. But in this function we set AssetNameBytes and then conditionally unset it, which took me some effort to understand. I suggest IsPrintableUTF8() is factored out of PrintableUTF8OrEmpty() and used here.

idb/postgres/internal/encoding/encode.go

api/handlers.go

types/types.go

tolikzinovyev · 2021-07-21T16:54:51Z

test/common.sh

+      '"reserve": "EQJSQTOITX64CPGA5VRKBKLEJR57YXVNLTW5DKZMESIPTEOLRDNWQIJCGU"' \
+      '"total": 1000000000000' \
+      '"unit-name": "bogo"' \
+      '"unit-name-b64": "Ym9nbw=="'


I think we also need tests where the strings are not utf-8-printable. I think a go test will suffice.

Unfortunately, I don't have a good way to test the HTTP layer. You're right that this is missing the final check for removing name/unit-name/url if they contain non-utf8 characters.

Mock IndexerDb and test the api functions? Ideally, we would test api functions with an implementation (implementations) of IndexerDb, but a bit of refactoring will need to be done beforehand.

For now, I have run some tests manually.

I think it would be good to do some refactoring to the api.Serve, the pattern used by the block generator is pretty nice and allowed me to start/stop the server multiple times for the performance test runner.

tolikzinovyev · 2021-07-22T14:44:44Z

LGTM. I will let @tsachiherman and @brianolson take a look as well.

brianolson

Gave it another read and generally looks good but one quibble over file layout.

brianolson · 2021-07-22T14:55:37Z

idb/postgres/internal/encoding/types.go

+	"github.com/algorand/indexer/types"
+)
+
+type assetParams struct {


I don't like the split of types.go/encode.go/decode.go
I'd rather see one type declaration and its encode and decode in one file (OOP style). Easier to make sure encode and decode match that way. Easier to find the type being referred to.

This split makes a lot more sense in the context of Tolik's rewrite, this pattern is used to wrap many more objects than I've done here.

I would probably opt for a single file, only because it would let you use simple text searches to find everything. But don't have a strong opinion either way. For now, I'd like to match his PR to make the merge slightly easier, maybe we can postpone this discussion for the rewrite PR?

Reference: https://github.com/algorand/indexer/blob/tolik/acct-rewrite4/idb/postgres/internal/encoding/types.go

I won't block over it but I still don't like it. I still want type-encode-decode bundled together in one file. types.go makes the most sense when the only thing happening to them is via reflection (e.g. go-codec loading and storing serialization forms).

tsachiherman · 2021-07-22T17:03:29Z

idb/postgres/internal/encoding/decode.go

 	"github.com/algorand/go-algorand-sdk/encoding/json"
+	"github.com/algorand/indexer/types"
+
+	sdk_types "github.com/algorand/go-algorand-sdk/types"


The grouping here is weird; it should have been like this, right ?

import( "github.com/algorand/go-algorand-sdk/encoding/json" sdk_types "github.com/algorand/go-algorand-sdk/types" "github.com/algorand/indexer/types" )

tsachiherman

looks good to me.

winder added 6 commits July 16, 2021 14:46

Update OpenAPI spec.

0482bfe

Handle non printable UTF8 asset strings.

e7af5a9

Fix tests.

72fb1c1

Fix formatting.

2b1642c

Fix tests.

3ed9563

Fix linter.

996c4a0

winder requested review from brianolson and tolikzinovyev July 17, 2021 13:42

winder added 3 commits July 17, 2021 10:18

Use nightly build in mule.

131825c

Revert "Use nightly build in mule."

4d1d4ad

This reverts commit 131825c.

Test transaction endpoint includes b64 fields.

1e4f135

winder self-assigned this Jul 19, 2021

winder added the Team Carbon-11 label Jul 19, 2021

Merge branch 'develop' into will/asset-b64

6321d4f

tsachiherman reviewed Jul 19, 2021

View reviewed changes

types/types.go Outdated Show resolved Hide resolved

tsachiherman reviewed Jul 19, 2021

View reviewed changes

idb/postgres/postgres.go Outdated Show resolved Hide resolved

brianolson reviewed Jul 19, 2021

View reviewed changes

idb/postgres/internal/encoding/encode.go Outdated Show resolved Hide resolved

idb/postgres/internal/encoding/encode.go Outdated Show resolved Hide resolved

idb/postgres/postgres.go Outdated Show resolved Hide resolved

tsachiherman reviewed Jul 19, 2021

View reviewed changes

idb/postgres/postgres.go Outdated Show resolved Hide resolved

winder added 5 commits July 19, 2021 17:13

Run e2e test with nightly channel. (#580)

430a061

Refactoring pt 1

03a4b64

Finish refactoring.

c157136

Remove baPtrOrDefault

9fffb31

fmt and lint

a30d3bb

This was referenced Jul 20, 2021

Support assets with null characters in strings. #567

Closed

backslash escape bad utf8 and nulls #568

Closed

winder added 2 commits July 20, 2021 11:02

Compute missing

2b1558b

Docker-compose go version should match matrix version.

953fbcd

tsachiherman reviewed Jul 20, 2021

View reviewed changes

api/handlers.go Outdated Show resolved Hide resolved

tsachiherman reviewed Jul 20, 2021

View reviewed changes

winder added 3 commits July 20, 2021 13:08

trigger build

767a8fa

Print image in dockerfile.

c2de3a1

Fix docker-using-travis-go-version script.

d0bef76

winder added 3 commits July 20, 2021 15:26

Use encode/decode conventions in accounting rewrite.

c9d3417

Update transaction encoding to use the same format as assets.

abe67d1

Move AssetRow back to sdk_types.AssetParams

bd67161

winder added 2 commits July 21, 2021 07:43

Add encoding test, fix lint/fmt

58dafb1

Add another test, fix txn encode.

586f679

winder linked an issue Jul 21, 2021 that may be closed by this pull request

non utf8 support for asset strings #566

Closed

winder added 2 commits July 21, 2021 08:34

Add missing asset fields.

1ec9080

oops

9d0ac4c

winder changed the title ~~utf8 support for asset strings~~ non utf8 support for asset strings Jul 21, 2021

tolikzinovyev reviewed Jul 21, 2021

View reviewed changes

More PR feedback.

bcccc1b

tolikzinovyev reviewed Jul 21, 2021

View reviewed changes

More PR feedback.

bcb65e0

winder requested review from brianolson, tolikzinovyev and tsachiherman July 22, 2021 13:54

brianolson reviewed Jul 22, 2021

View reviewed changes

tsachiherman reviewed Jul 22, 2021

View reviewed changes

tsachiherman approved these changes Jul 22, 2021

View reviewed changes

winder added 3 commits July 22, 2021 14:04

Fix import order.

6df1663

Merge branch 'develop' into will/asset-b64

ee71033

Fix merge error.

1c50426

winder merged commit 6e4d737 into develop Jul 22, 2021

winder deleted the will/asset-b64 branch July 22, 2021 18:28

non utf8 support for asset strings #577

non utf8 support for asset strings #577

Uh oh!

Conversation

winder commented Jul 17, 2021

Summary

Test Plan

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Jul 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tolikzinovyev commented Jul 20, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tolikzinovyev commented Jul 22, 2021

Uh oh!

brianolson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tsachiherman left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov-commenter commented Jul 20, 2021 •

edited

Loading