Fix for issue 96. #100

LLITCHEV · 2023-09-08T04:23:42Z

Added tests for all the types.

Related issue(s):

Fixes #96
Fixes #73

github-actions · 2023-09-08T04:25:54Z

JUnit Test Report

    46 files +  1     46 suites +1 4m 45s ⏱️ + 1m 29s
  758 tests +21   757 ✔️ +21   1 💤 ±0 0 ❌ ±0
5 808 runs +21 5 790 ✔️ +21 18 💤 ±0 0 ❌ ±0

Results for commit 958c8c4. ± Comparison against base commit 5ea94d1.

This pull request removes 2 and adds 22 tests. Note that renamed tests count towards both.

, 1
com.hedera.pbj.runtime.Utf8ToolsTest ‑ [4]

com.hedera.pbj.runtime.ProtoParserToolsTest ‑ [1] 0
com.hedera.pbj.runtime.ProtoParserToolsTest ‑ [1] 1
com.hedera.pbj.runtime.ProtoParserToolsTest ‑ [2] 1
com.hedera.pbj.runtime.ProtoParserToolsTest ‑ [2] 2
com.hedera.pbj.runtime.ProtoParserToolsTest ‑ [3] 3
com.hedera.pbj.runtime.ProtoParserToolsTest ‑ testReadBytes()
com.hedera.pbj.runtime.ProtoParserToolsTest ‑ testReadBytes_incomplete()
com.hedera.pbj.runtime.ProtoParserToolsTest ‑ testReadDouble()
com.hedera.pbj.runtime.ProtoParserToolsTest ‑ testReadFixed64()
com.hedera.pbj.runtime.ProtoParserToolsTest ‑ testReadFixedInt32()
…

♻️ This comment has been updated with latest results.

github-actions · 2023-09-08T04:26:04Z

Integration Test Report

    215 files +    2     215 suites +2 5m 36s ⏱️ + 1m 22s
86 997 tests +  34 86 997 ✔️ +  34 0 💤 ±0 0 ❌ ±0
87 263 runs +300 87 263 ✔️ +300 0 💤 ±0 0 ❌ ±0

Results for commit 958c8c4. ± Comparison against base commit 5ea94d1.

♻️ This comment has been updated with latest results.

pbj-core/pbj-runtime/src/main/java/com/hedera/pbj/runtime/ProtoParserTools.java

pbj-integration-tests/src/test/java/com/hedera/pbj/intergration/test/Issue96Test.java

jasperpotts

Think we still need to check at the end of parsing a sub-object that all the expected bytes were read. Anywhere there is length encoded data with a length, we should enforce we never read more or less bytes than specified in length.

LLITCHEV · 2023-09-15T18:53:56Z

@jasperpotts I aded the extra check. I added the check to make sure we have enough limit (which is a fast fail - without having the actual read). It does make sense to do the check after the read as well, so added code there as well. Thanks!

pbj-integration-tests/src/test/java/com/hedera/pbj/intergration/test/TruncatedDataTests.java

pbj-core/pbj-runtime/src/main/java/com/hedera/pbj/runtime/ProtoParserTools.java

jasperpotts · 2023-11-28T00:10:57Z

We also need to check that all cases are covered. In protobuf there are a few core data types, fixed length, variant and length encoded. All length encoded data we need to make sure all of the specified length bytes are read or throw exception. Otherwise we can start processing the remaining data as a different field and get corrupt data. Length encoded includes, strings, bytes and sub-messages as maybe others.

jasperpotts · 2023-11-28T00:12:23Z

We should also check anywhere we do a bulk read of bytes with int getBytes(byteArray) style method that we check the returned number of bytes read matches expected. We have choice to loop and keep reading till all bytes are read or throw exception.

Added tests for all the types. Signed-off-by: Lubomir Litchev <[email protected]>

Signed-off-by: Lubomir Litchev <[email protected]>

If the buffer is truncated on the boundary of the subObject, we still need to throw an exception. Signed-off-by: Lubomir Litchev <[email protected]>

While strictParsing, we should make sure the buffer is not empty when parsing strictly a type from buffer. This can happen if the buffer is truncated to an empty one. Signed-off-by: Lubomir Litchev <[email protected]>

Signed-off-by: Lubomir Litchev <[email protected]>

Signed-off-by: Ivan Malygin <[email protected]>

codacy-production · 2023-12-04T17:04:00Z

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation	Diff coverage
✅ +1.67% (target: -1.00%)	75.00%

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (`53391bd`)	3766	1013	26.90%
Head commit (`958c8c4`)	3780 (+14)	1080 (+67)	28.57% (+1.67%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#100)	12	9	75.00%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings Change summary preferences

Signed-off-by: Ivan Malygin <[email protected]>

pbj-core/pbj-runtime/src/main/java/com/hedera/pbj/runtime/ProtoParserTools.java

… Cleaned up redundand `throw IOException` Signed-off-by: Ivan Malygin <[email protected]>

LLITCHEV requested a review from jasperpotts September 8, 2023 04:23

LLITCHEV self-assigned this Sep 8, 2023

jasperpotts requested changes Sep 8, 2023

View reviewed changes

jasperpotts requested changes Sep 13, 2023

View reviewed changes

LLITCHEV requested a review from jasperpotts September 15, 2023 19:04

codacy-production bot reviewed Nov 13, 2023

View reviewed changes

pbj-integration-tests/src/test/java/com/hedera/pbj/intergration/test/TruncatedDataTests.java Show resolved Hide resolved

codacy-production bot reviewed Nov 13, 2023

View reviewed changes

pbj-integration-tests/src/test/java/com/hedera/pbj/intergration/test/TruncatedDataTests.java Show resolved Hide resolved

codacy-production bot reviewed Nov 13, 2023

View reviewed changes

pbj-integration-tests/src/test/java/com/hedera/pbj/intergration/test/TruncatedDataTests.java Show resolved Hide resolved

jasperpotts reviewed Nov 28, 2023

View reviewed changes

pbj-core/pbj-runtime/src/main/java/com/hedera/pbj/runtime/ProtoParserTools.java Outdated Show resolved Hide resolved

Lubomir Litchev added 8 commits December 1, 2023 13:50

Fix for issue 96.

8da1f1e

Added tests for all the types. Signed-off-by: Lubomir Litchev <[email protected]>

Move the check before reading the bytes.

38faab4

Signed-off-by: Lubomir Litchev <[email protected]>

Addressed some CR.

76b1d26

Signed-off-by: Lubomir Litchev <[email protected]>

Add code to throw BufferUnderflowException()

c74483f

If the buffer is truncated on the boundary of the subObject, we still need to throw an exception. Signed-off-by: Lubomir Litchev <[email protected]>

Added code to make sure the input is not empty.

68bbf2f

While strictParsing, we should make sure the buffer is not empty when parsing strictly a type from buffer. This can happen if the buffer is truncated to an empty one. Signed-off-by: Lubomir Litchev <[email protected]>

Added test to parse and strictParse empty buffers.

bb4e9dd

Signed-off-by: Lubomir Litchev <[email protected]>

Add some more checks.

bc7ad90

Signed-off-by: Lubomir Litchev <[email protected]>

dded an extra check for read bytes per CR.

2a64a37

Signed-off-by: Lubomir Litchev <[email protected]>

imalygin force-pushed the lubo-issue-96 branch 2 times, most recently from 5f7a65a to 522e667 Compare December 4, 2023 16:54

Addressed Jasper's comments. Added more tests.

983a4a4

Signed-off-by: Ivan Malygin <[email protected]>

imalygin force-pushed the lubo-issue-96 branch from 522e667 to 983a4a4 Compare December 4, 2023 16:58

poulok assigned imalygin and unassigned LLITCHEV Dec 4, 2023

Added more checks and tests.

57bed6c

Signed-off-by: Ivan Malygin <[email protected]>

artemananiev previously approved these changes Dec 4, 2023

View reviewed changes

imalygin dismissed artemananiev’s stale review via 3db7895 December 4, 2023 23:32

Removed redundant IOException clause

e6ca170

Signed-off-by: Ivan Malygin <[email protected]>

imalygin force-pushed the lubo-issue-96 branch from 3db7895 to e6ca170 Compare December 4, 2023 23:34

jasperpotts requested changes Dec 5, 2023

View reviewed changes

pbj-core/pbj-runtime/src/main/java/com/hedera/pbj/runtime/ProtoParserTools.java Outdated Show resolved Hide resolved

Inlined validateInput method as there were only a few usages of it.…

958c8c4

… Cleaned up redundand `throw IOException` Signed-off-by: Ivan Malygin <[email protected]>

imalygin force-pushed the lubo-issue-96 branch from c48f1b0 to 958c8c4 Compare December 5, 2023 17:28

jasperpotts approved these changes Dec 5, 2023

View reviewed changes

artemananiev approved these changes Dec 5, 2023

View reviewed changes

imalygin merged commit f2933b2 into main Dec 5, 2023

imalygin deleted the lubo-issue-96 branch December 5, 2023 18:30

imalygin mentioned this pull request Dec 11, 2023

Upgrade pbj to 0.7.8 hiero-ledger/hiero-consensus-node#10425

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for issue 96. #100

Fix for issue 96. #100

LLITCHEV commented Sep 8, 2023 •

edited by poulok

Loading

github-actions bot commented Sep 8, 2023 •

edited

Loading

github-actions bot commented Sep 8, 2023 •

edited

Loading

jasperpotts left a comment

LLITCHEV commented Sep 15, 2023

jasperpotts commented Nov 28, 2023

jasperpotts commented Nov 28, 2023

codacy-production bot commented Dec 4, 2023 •

edited

Loading

Fix for issue 96. #100

Fix for issue 96. #100

Conversation

LLITCHEV commented Sep 8, 2023 • edited by poulok Loading

github-actions bot commented Sep 8, 2023 • edited Loading

JUnit Test Report

github-actions bot commented Sep 8, 2023 • edited Loading

Integration Test Report

jasperpotts left a comment

Choose a reason for hiding this comment

LLITCHEV commented Sep 15, 2023

jasperpotts commented Nov 28, 2023

jasperpotts commented Nov 28, 2023

codacy-production bot commented Dec 4, 2023 • edited Loading

Coverage summary from Codacy

See diff coverage on Codacy

See your quality gate settings Change summary preferences

LLITCHEV commented Sep 8, 2023 •

edited by poulok

Loading

github-actions bot commented Sep 8, 2023 •

edited

Loading

github-actions bot commented Sep 8, 2023 •

edited

Loading

codacy-production bot commented Dec 4, 2023 •

edited

Loading