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

Return error with invalid length #180

Merged
merged 5 commits into from
Jul 13, 2022
Merged

Conversation

mfdeveloper508
Copy link
Contributor

removed packed check, update composite field

@mfdeveloper508 mfdeveloper508 self-assigned this Jun 27, 2022
@mfdeveloper508 mfdeveloper508 added the bug Something isn't working label Jun 27, 2022
@adamdecaf
Copy link
Member

I deleted an outdated CI config file. If you can rebase CI will get past the error from gitleaks.

@adamdecaf adamdecaf force-pushed the fix-composit-for-varialble-length branch from da300af to f4ae3ee Compare July 1, 2022 18:28
field/composite.go Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Jul 1, 2022

Codecov Report

Merging #180 (e6549fc) into master (94bf9c7) will increase coverage by 0.27%.
The diff coverage is 100.00%.

❗ Current head e6549fc differs from pull request most recent head 4735ea4. Consider uploading reports for the commit 4735ea4 to get more accurate results

@@            Coverage Diff             @@
##           master     #180      +/-   ##
==========================================
+ Coverage   70.14%   70.41%   +0.27%     
==========================================
  Files          37       37              
  Lines        1785     1768      -17     
==========================================
- Hits         1252     1245       -7     
+ Misses        338      334       -4     
+ Partials      195      189       -6     
Impacted Files Coverage Δ
field/binary.go 55.26% <ø> (-1.15%) ⬇️
field/string.go 77.14% <ø> (-0.64%) ⬇️
field/track1.go 69.02% <ø> (+1.20%) ⬆️
field/track2.go 68.22% <ø> (+1.25%) ⬆️
field/track3.go 61.79% <ø> (+1.35%) ⬆️
field/composite.go 81.09% <100.00%> (+0.32%) ⬆️
field/numeric.go 81.57% <0.00%> (+2.63%) ⬆️

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 94bf9c7...4735ea4. Read the comment docs.

@@ -542,6 +539,49 @@ func TestCompositePackingWithTags(t *testing.T) {
require.EqualError(t, err, "failed to encode length: field length: 12 should be fixed: 6")
})

t.Run("Pack returns error when encoded data length is larger than specified variable max length", func(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there was a test for fixed length, I added one for variable length.

// 11 byte limit imposed by the prefix. [011 | 10000100012]
// Therefore, it won't be included in the packed bytes.

packed := []byte("01110000100012")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is proper packed data for the LLL field:

  • length - 011
  • then the fields from 1 to 11 are unpacked
  • fields 12, 13, 14 are not set

require.NoError(t, err)
require.Equal(t, 14, read)

packedBytes, err := composite.Pack()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now we just pack it back. Unset fields will not be included in the packed value. It's just expected behavior.

So, test is removed as we don't test here anything. Or I'm missing something.

@alovak
Copy link
Contributor

alovak commented Jul 11, 2022

@adamdecaf please, take a look.

@mfdeveloper508 did what we had to do: just removed the check. It packs and unpacks according to expectations:

  • If there is no data or more data for the fixed-length field - it will fail
  • If there is more data than max variable length specified for the field - it will fail

I may miss some use-cases @Dakinola892 @cheukwing please review it as well.

I removed some unnecessary code and added some tests.

@alovak
Copy link
Contributor

alovak commented Jul 11, 2022

@louisheath I would be glad if you can check this change as well.

Copy link
Member

@adamdecaf adamdecaf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems fine to me. Did we need to combine changes for zero-length packing and overflow packing?

Copy link
Contributor

@Dakinola892 Dakinola892 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy with these new changes

@alovak alovak merged commit 92f3b34 into master Jul 13, 2022
@alovak alovak deleted the fix-composit-for-varialble-length branch July 13, 2022 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Return error when the lenght of the field value does not match the spec fixed lenght
5 participants