Add encode method to convert Sections into GPP String #9

duduchristian · 2023-02-15T10:28:52Z

No description provided.

util/encoding.go

encode.go

hhhjort · 2023-02-27T20:10:05Z

Can you put a note in the README that, in practice, only CMPs should be writing GPP strings as per the IAB documentation? We don't want to encourage people to be generating GPP strings in production when they really should not be.

hhhjort · 2023-03-15T13:46:54Z

Just a friendly ping to see if you have read the comments, and intend to keep working on this PR.

Thanks

duduchristian · 2023-03-27T02:24:29Z

Coming back, still working on this.

Just a friendly ping to see if you have read the comments, and intend to keep working on this PR.

Thanks

Coming back, still working~

encode.go

encode_test.go

util/encoding_test.go

hhhjort · 2023-03-29T19:40:14Z

It may be interesting to write a test where we decode some GPP strings, then re-encode them and compare to see if we got the original string back. It could uncover a subtle error, or potentially that the spec is under specified such that there are multiple ways to encode a set of data into a string.

hhhjort · 2023-04-10T19:49:19Z

Sorry we have been a bit busy and haven't responded in a bit. This PR seems to be in a good state, and we should get the final review in soon.

encode.go

util/encoding.go

encode.go

encode_test.go

util/encoding_test.go

util/encoding.go

util/encoding_test.go

bsardo · 2023-04-27T17:30:04Z

Hi @duduchristian, just checking in to see if you have any time to address my comments. Overall, it looks good. No rush and thanks!

duduchristian · 2023-05-03T05:40:58Z

yep, I get some time next week. Just coming back from my holiday hahah.

bsardo · 2023-05-15T17:13:25Z

Hi @duduchristian, just checking in to see if you have some time to address the outstanding comments. No rush 🙂

bsardo · 2023-06-05T17:18:23Z

Hi @duduchristian. It looks like you made some of the updates but there are a few more comments outstanding. Are you planning to address those as well? Thanks!

duduchristian · 2023-06-19T06:59:45Z

Hi @duduchristian. It looks like you made some of the updates but there are a few more comments outstanding. Are you planning to address those as well? Thanks!

Sorry I was so busy in the previous weeks. It seems I've addressed all the problems mentioned in comments, can you check the code and if there are any problems, make comments just as what you did before. Thank you.

.deepsource.toml

util/encoding.go

hhhjort · 2023-06-22T19:55:24Z

util/encoding.go

+ }
+ temp = make([]byte, final, size)
+ } else {
+ size := (final-expThreshold+step-1)/step*step + expThreshold


Might want to add a parenthesis to make the order of operations clearer for humans.
((final-expThreshold+step-1)/step)*step
It took me a bit to parse this correctly in my head.

util/encoding.go

hhhjort · 2023-06-22T20:08:42Z

util/encoding.go

+func (bs *BitStream) WriteFibonacciInt(num uint16) error {
+ // The num should be [1,6765). Actually once the num is larger than or equal to 987,
+ // the efficiency of Fibonacci Encoding would be no better than 'WriteUint16'.
+ if num <= 0 || num >= fibonacci(fibLen) {


If you are limiting to fibLen, then you can just lookup in the fibLookup[i] array rather than calling the fibonacci() function. The function just adds code to handle the case where the number if greater than fibLen.

hhhjort · 2023-06-26T14:41:50Z

util/encoding.go

+so the indicator is 0, followed by Fibonacci encoded 1, while the next range [3, 4] refers to a range containing
+not only one integer, so the indicator is 1, followed by Fibonacci encoded 3 and 1 (4-3).
+*/
+func (bs *BitStream) WriteIntRange(intRange *IntRange) error {


Shouldn't Fibonacci enter the name someplace as the GPP spec also defines a non-Fibonacci int range. The non-Fibonacci range will be needed once TCFv2 support is added.

Would you like to comment on the difference between the Fibonacci Int range and the legacy Int Range that is used in TCF. We do plan to add TCFv2 code to this library once we get some time to do it. It hasn't been too urgent as an independent TCFv2 library exists.

The current name will be an issue when TCFv2 is brought into the project and the original Int range is implemented. The documentation refers to them as Range (Fibonacci) and Range (Int), so the current name could confuse people looking for the Range (Fibonacci) implementation. This is why the corresponding read function is called ReadFibonacciRange().

bsardo · 2023-07-05T17:45:04Z

Hi @duduchristian, thanks for responding to our comments. We recently left a few more for you to address when you get a chance. If you don't mind, could you please merge with master instead of force pushing? It will save us a lot of time reviewing. Thanks!

util/encoding.go

hhhjort

Looks good to me now. Will get a second pair of eyes to review before we merge.

util/encoding.go

Focusing on the big issue, I forgot there were smaller outstanding comments on Fibonacci.

guscarreon · 2023-08-09T20:48:16Z

encode.go

+ return "", duplicatedSectionErr
+ }
+ if prevID+1 == id {
+ intRange.Range[len(intRange.Range)-1].EndID = uint16(id)


If id equals zero in the first loop of this for statement, we'll be referencing Range at position -1. The following corner case indeed makes Encode(sections []Section) panic:

16 type gppEncodeTestData struct { 17 description string 18 sections []Section 19 expected string 20 } 21 + type zeroSection struct{} + + func (s zeroSection) Encode(b bool) []byte { + return nil + } + + func (s zeroSection) GetID() constants.SectionID { + return constants.SectionID(0) + } + + func (s zeroSection) GetValue() string { + return "string" + } + 22 var testData = []gppEncodeTestData{ + { + description: "Corner case of a section whose GetID returns 0", + sections: []Section{zeroSection{}}, + }, 23 { 24 description: "USPCA GPP string encoding", 25 expected: "DBABh4A~BlgWEYCY.QA~BSFgmiU", 26 sections: []Section{ 27 uspca.USPCA{ 28 CoreSegment: uspca.USPCACoreSegment{ 29 Version: 1, 30 SaleOptOutNotice: 2, 31 SharingOptOutNotice: 1, 32 SensitiveDataLimitUseNotice: 1, 33 *--251 lines: SaleOptOut: 2,------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- 284 }, 285 } 286 287 func TestEncode(t *testing.T) { 288 for _, test := range testData { 289 result, err := Encode(test.sections) encode_test.go

Can we add a check?

bsardo · 2023-10-02T17:27:38Z

Hi @duduchristian. It looks like we're close here. If you could address the remaining comment when you get a chance, that would be great. Thanks!

hhhjort

LGTM

guscarreon

Looks pretty good to me. I just left two very minor comments

guscarreon · 2023-11-08T14:43:44Z

util/encoding.go

+)
+
+func getByteSlice() []byte {
+ // Most string to be encoded are less than 8 bytes.


Nitpick: "Most strings"

guscarreon · 2023-11-10T17:41:02Z

encode_test.go

+ MspaOptOutOptionMode: 1,
+ MspaServiceProviderMode: 1,
+ },
+ SectionID: 9,


Nitpick: can we set to it's constants.SectionUSPVA value instead?

guscarreon

LGTM

hhhjort reviewed Feb 27, 2023

View reviewed changes

util/encoding.go Outdated Show resolved Hide resolved

hhhjort reviewed Feb 27, 2023

View reviewed changes

encode.go Outdated Show resolved Hide resolved

bsardo self-requested a review March 13, 2023 17:19

bsardo self-assigned this Mar 13, 2023

duduchristian force-pushed the feature/encode branch 2 times, most recently from f8059d7 to 08c9814 Compare March 27, 2023 08:15

hhhjort reviewed Mar 28, 2023

View reviewed changes

encode.go Outdated Show resolved Hide resolved

hhhjort reviewed Mar 28, 2023

View reviewed changes

encode_test.go Show resolved Hide resolved

hhhjort reviewed Mar 28, 2023

View reviewed changes

util/encoding_test.go Show resolved Hide resolved

ci: Add .deepsource.toml

014f774

bsardo reviewed Apr 12, 2023

View reviewed changes

duduchristian force-pushed the feature/encode branch from da27435 to aef173f Compare May 4, 2023 07:36

DuduPan added 9 commits June 19, 2023 14:35

Implements base encoding methods

69a7f5c

Encoding method for uspct

11983b3

Encoding method for uspco and uspva

ae35648

Encoding method for uspca

b6aa138

Encoding method for uspnat

eaa27d9

Encoding method for usput

111f86c

Complete the encode method

e5b7e82

Add some comments.

d029401

Optimize the encoding pipeline.

4ca9574

duduchristian force-pushed the feature/encode branch from a392825 to c304daf Compare June 19, 2023 06:37

hhhjort reviewed Jun 22, 2023

View reviewed changes

.deepsource.toml Outdated Show resolved Hide resolved

hhhjort reviewed Jun 22, 2023

View reviewed changes

util/encoding.go Outdated Show resolved Hide resolved

hhhjort reviewed Jun 22, 2023

View reviewed changes

util/encoding.go Show resolved Hide resolved

hhhjort reviewed Jun 22, 2023

View reviewed changes

hhhjort reviewed Jun 26, 2023

View reviewed changes

Refine the code

84fa3f2

hhhjort reviewed Jul 12, 2023

View reviewed changes

util/encoding.go Outdated Show resolved Hide resolved

Fix bug

83c5eb0

hhhjort previously approved these changes Jul 31, 2023

View reviewed changes

hhhjort reviewed Aug 2, 2023

View reviewed changes

util/encoding.go Outdated Show resolved Hide resolved

hhhjort assigned guscarreon and hhhjort and unassigned bsardo Aug 3, 2023

optimize WriteFibonacciInt

97a2ee2

guscarreon reviewed Aug 9, 2023

View reviewed changes

fix the bug of zero section id panic

4f52cf7

hhhjort previously approved these changes Oct 30, 2023

View reviewed changes

guscarreon reviewed Nov 10, 2023

View reviewed changes

duduchristian dismissed hhhjort’s stale review via 88ec25c November 13, 2023 03:01

fix picked nits

b1c7c3f

duduchristian force-pushed the feature/encode branch from 88ec25c to b1c7c3f Compare November 13, 2023 03:04

guscarreon approved these changes Nov 13, 2023

View reviewed changes

hhhjort approved these changes Nov 13, 2023

View reviewed changes

bsardo merged commit 8cca024 into prebid:main Nov 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add encode method to convert Sections into GPP String #9

Add encode method to convert Sections into GPP String #9

duduchristian commented Feb 15, 2023

hhhjort commented Feb 27, 2023

hhhjort commented Mar 15, 2023

duduchristian commented Mar 27, 2023

hhhjort commented Mar 29, 2023

hhhjort commented Apr 10, 2023

bsardo commented Apr 27, 2023

duduchristian commented May 3, 2023

bsardo commented May 15, 2023

bsardo commented Jun 5, 2023

duduchristian commented Jun 19, 2023

hhhjort Jun 22, 2023

hhhjort Jun 22, 2023

hhhjort Jun 26, 2023

hhhjort Jul 26, 2023

hhhjort Aug 9, 2023

bsardo commented Jul 5, 2023

hhhjort left a comment

guscarreon Aug 9, 2023

bsardo commented Oct 2, 2023

hhhjort left a comment

guscarreon left a comment

guscarreon Nov 8, 2023

guscarreon Nov 10, 2023

guscarreon left a comment

Add encode method to convert Sections into GPP String #9

Add encode method to convert Sections into GPP String #9

Conversation

duduchristian commented Feb 15, 2023

hhhjort commented Feb 27, 2023

hhhjort commented Mar 15, 2023

duduchristian commented Mar 27, 2023

hhhjort commented Mar 29, 2023

hhhjort commented Apr 10, 2023

bsardo commented Apr 27, 2023

duduchristian commented May 3, 2023

bsardo commented May 15, 2023

bsardo commented Jun 5, 2023

duduchristian commented Jun 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsardo commented Jul 5, 2023

hhhjort left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsardo commented Oct 2, 2023

hhhjort left a comment

Choose a reason for hiding this comment

guscarreon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guscarreon left a comment

Choose a reason for hiding this comment