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.

Sign up for GitHub

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

Jump to bottom

BUG: The length of the IP address was not being set. #91

Merged
merged 8 commits into from
Mar 20, 2025
Merged

BUG: The length of the IP address was not being set. #91

merged 8 commits into from
Mar 20, 2025

Conversation

jwrosewell
Copy link
Contributor

@jwrosewell jwrosewell commented Mar 5, 2025 •
edited
Loading

Tests need to be added to verify the presence of the length member and that its value is correct.

@jwrosewell jwrosewell requested a review from justadreamer March 5, 2025 14:53
@justadreamer
Copy link
Contributor

justadreamer commented Mar 6, 2025 •
edited
Loading

added 2 asserts to verify IP address length, 2 concerns:

  • big-endianness of the parsed IP address, i.e. the IP address "::FFFF:FFFF:FFFF:FFFF" will parse into this big-endian representation of bytes { 0, 0, 0, 0, 0, 0, 0, 0, 255, 255, 255, 255, 255, 255, 255, 255 } and the length will be set to 8
  • we have a commented out file of legacy tests: IpHeaderParserTests - should it be deleted or resurrected? Some parser tests now live in IpParserTests

@justadreamer
Copy link
Contributor

justadreamer commented Mar 6, 2025 •
edited
Loading

The comment on the IpHeaderParserTests is addressed here: #76 (comment) - they are not needed for now.

I now think on what's the semantics of this length field and is it really needed. The comment says that it's the length of the byte array and that's how Max has changed the implementation and tests, but then it is redundant, because you can figure out whether it is 4 or 16 bytes from the type of the IP address.

size_t length; /**< Length of the byte array. */

If it is the number of bytes written - then the comment needs to be changed, and also the question on whether this field is needed at all for clients to consume.

@jwrosewell
Copy link
Contributor Author

@drasmart we agreed to remove the length property from common-cxx. Therefore this PR needs to change to do that.

@justadreamer
Copy link
Contributor

justadreamer commented Mar 11, 2025 •
edited
Loading

I actually attempted to do that and removed all length-checking asserts from the tests, but then I discovered that .type field gets overwritten during parsing. This test fails:

IpParserTests.cpp:234: Failure
Expected equality of these values:
  result->type
    Which is: '\0'
  FIFTYONE_DEGREES_IP_TYPE_IPV6
    Which is: 6
Expected type to be IPv6

it looks like the type is set here, but then overwritten by further parsing code...

common-cxx/ip.c

Lines 308 to 322 in 1e435a2

address->type = type;
IpAddressBuildState buildState = {
address,
address->value,
byteCount,
};
// Add the bytes from the source value and get the type of address.
iterateIpAddress(
start,
end,
&buildState,
&springCount,
type,
callbackIpAddressBuild);
return true;

previously when the length field was part of the structure - I guess it was overwritten... so it suggests we have some issue in the parsing code, I can push my commit with this change and test failing..

@jwrosewell
Copy link
Contributor Author

@drasmart to include as part of today's PR and then review with @justadreamer at that time.

@jwrosewell
Copy link
Contributor Author

@drasmart will make in parallel with #93.

@drasmart
Copy link
Contributor

Test run for last commit:

@jwrosewell
Copy link
Contributor Author

@drasmart can be merged.

@github-actions GitHub Actions
Copy link

Unit Tests - Ubuntu_x86_Debug

592 tests  ±0   592 ✅ ±0   15s ⏱️ -1s
  1 suites ±0     0 💤 ±0 
  1 files   ±0     0 ❌ ±0 

Results for commit 5d90afd. ± Comparison against base commit 1e435a2.

@github-actions GitHub Actions
Copy link

Unit Tests - Ubuntu_x86_Release

592 tests  ±0   592 ✅ ±0   9s ⏱️ -1s
  1 suites ±0     0 💤 ±0 
  1 files   ±0     0 ❌ ±0 

Results for commit 5d90afd. ± Comparison against base commit 1e435a2.

@github-actions GitHub Actions
Copy link

Integration Tests - Ubuntu_x86_Debug

0 tests  ±0   0 ✅ ±0   0s ⏱️ ±0s
1 suites ±0   0 💤 ±0 
1 files   ±0   0 ❌ ±0 

Results for commit 5d90afd. ± Comparison against base commit 1e435a2.

@github-actions GitHub Actions
Copy link

Integration Tests - Ubuntu_x86_Release

0 tests  ±0   0 ✅ ±0   0s ⏱️ ±0s
1 suites ±0   0 💤 ±0 
1 files   ±0   0 ❌ ±0 

Results for commit 5d90afd. ± Comparison against base commit 1e435a2.

@github-actions GitHub Actions
Copy link

Unit Tests - Ubuntu_x64_Release

592 tests  ±0   592 ✅ ±0   10s ⏱️ -1s
  1 suites ±0     0 💤 ±0 
  1 files   ±0     0 ❌ ±0 

Results for commit 5d90afd. ± Comparison against base commit 1e435a2.

@github-actions GitHub Actions
Copy link

Unit Tests - Mac_x64_Debug

590 tests  ±0   590 ✅ ±0   40s ⏱️ +2s
  1 suites ±0     0 💤 ±0 
  1 files   ±0     0 ❌ ±0 

Results for commit 5d90afd. ± Comparison against base commit 1e435a2.

@github-actions GitHub Actions
Copy link

Performance Tests - Ubuntu_x86_Release

0 tests  ±0   0 ✅ ±0   0s ⏱️ ±0s
1 suites ±0   0 💤 ±0 
1 files   ±0   0 ❌ ±0 

Results for commit 5d90afd. ± Comparison against base commit 1e435a2.

@github-actions GitHub Actions
Copy link

Unit Tests - Ubuntu_x64_Debug

592 tests  ±0   592 ✅ ±0   14s ⏱️ -1s
  1 suites ±0     0 💤 ±0 
  1 files   ±0     0 ❌ ±0 

Results for commit 5d90afd. ± Comparison against base commit 1e435a2.

@github-actions GitHub Actions
Copy link

Integration Tests - Ubuntu_x64_Release

0 tests  ±0   0 ✅ ±0   0s ⏱️ ±0s
1 suites ±0   0 💤 ±0 
1 files   ±0   0 ❌ ±0 

Results for commit 5d90afd. ± Comparison against base commit 1e435a2.

@github-actions GitHub Actions
Copy link

Integration Tests - Mac_x64_Debug

0 tests  ±0   0 ✅ ±0   0s ⏱️ ±0s
1 suites ±0   0 💤 ±0 
1 files   ±0   0 ❌ ±0 

Results for commit 5d90afd. ± Comparison against base commit 1e435a2.

@github-actions GitHub Actions
Copy link

Integration Tests - Ubuntu_x64_Debug

0 tests  ±0   0 ✅ ±0   0s ⏱️ ±0s
1 suites ±0   0 💤 ±0 
1 files   ±0   0 ❌ ±0 

Results for commit 5d90afd. ± Comparison against base commit 1e435a2.

@github-actions GitHub Actions
Copy link

Performance Tests - Ubuntu_x64_Release

0 tests  ±0   0 ✅ ±0   0s ⏱️ ±0s
1 suites ±0   0 💤 ±0 
1 files   ±0   0 ❌ ±0 

Results for commit 5d90afd. ± Comparison against base commit 1e435a2.

@github-actions GitHub Actions
Copy link

Unit Tests - Windows_x64_Debug

592 tests  ±0   592 ✅ ±0   19s ⏱️ +3s
  1 suites ±0     0 💤 ±0 
  1 files   ±0     0 ❌ ±0 

Results for commit 5d90afd. ± Comparison against base commit 1e435a2.

@github-actions GitHub Actions
Copy link

Unit Tests - Windows_x86_Debug

592 tests  ±0   592 ✅ ±0   16s ⏱️ ±0s
  1 suites ±0     0 💤 ±0 
  1 files   ±0     0 ❌ ±0 

Results for commit 5d90afd. ± Comparison against base commit 1e435a2.

@github-actions GitHub Actions
Copy link

Integration Tests - Windows_x64_Debug

0 tests  ±0   0 ✅ ±0   0s ⏱️ ±0s
1 suites ±0   0 💤 ±0 
1 files   ±0   0 ❌ ±0 

Results for commit 5d90afd. ± Comparison against base commit 1e435a2.

@github-actions GitHub Actions
Copy link

Unit Tests - Windows_VS_x64_Release

592 tests  ±0   592 ✅ ±0   5s ⏱️ ±0s
 75 suites ±0     0 💤 ±0 
  1 files   ±0     0 ❌ ±0 

Results for commit 5d90afd. ± Comparison against base commit 1e435a2.

@github-actions GitHub Actions
Copy link

Unit Tests - Windows_VS_x86_Release

592 tests  ±0   592 ✅ ±0   5s ⏱️ ±0s
 75 suites ±0     0 💤 ±0 
  1 files   ±0     0 ❌ ±0 

Results for commit 5d90afd. ± Comparison against base commit 1e435a2.

@github-actions GitHub Actions
Copy link

Unit Tests - Windows_VS_x64_Debug

592 tests  ±0   592 ✅ ±0   6s ⏱️ ±0s
 75 suites ±0     0 💤 ±0 
  1 files   ±0     0 ❌ ±0 

Results for commit 5d90afd. ± Comparison against base commit 1e435a2.

@github-actions GitHub Actions
Copy link

Integration Tests - Windows_x86_Debug

0 tests  ±0   0 ✅ ±0   0s ⏱️ ±0s
1 suites ±0   0 💤 ±0 
1 files   ±0   0 ❌ ±0 

Results for commit 5d90afd. ± Comparison against base commit 1e435a2.

@github-actions GitHub Actions
Copy link

Integration Tests - Windows_VS_x64_Release

0 tests  ±0   0 ✅ ±0   0s ⏱️ ±0s
0 suites ±0   0 💤 ±0 
0 files   ±0   0 ❌ ±0 

Results for commit 5d90afd. ± Comparison against base commit 1e435a2.

@github-actions GitHub Actions
Copy link

Integration Tests - Windows_VS_x86_Release

0 tests  ±0   0 ✅ ±0   0s ⏱️ ±0s
0 suites ±0   0 💤 ±0 
0 files   ±0   0 ❌ ±0 

Results for commit 5d90afd. ± Comparison against base commit 1e435a2.

@github-actions GitHub Actions
Copy link

Integration Tests - Windows_VS_x64_Debug

0 tests  ±0   0 ✅ ±0   0s ⏱️ ±0s
0 suites ±0   0 💤 ±0 
0 files   ±0   0 ❌ ±0 

Results for commit 5d90afd. ± Comparison against base commit 1e435a2.

@github-actions GitHub Actions
Copy link

Unit Tests - Windows_VS_x86_Debug

592 tests  ±0   592 ✅ ±0   6s ⏱️ ±0s
 75 suites ±0     0 💤 ±0 
  1 files   ±0     0 ❌ ±0 

Results for commit 5d90afd. ± Comparison against base commit 1e435a2.

@github-actions GitHub Actions
Copy link

Unit Tests - Windows_x64_Release

592 tests  ±0   592 ✅ ±0   8s ⏱️ ±0s
  1 suites ±0     0 💤 ±0 
  1 files   ±0     0 ❌ ±0 

Results for commit 5d90afd. ± Comparison against base commit 1e435a2.

@github-actions GitHub Actions
Copy link

Unit Tests - Windows_x86_Release

592 tests  ±0   592 ✅ ±0   8s ⏱️ ±0s
  1 suites ±0     0 💤 ±0 
  1 files   ±0     0 ❌ ±0 

Results for commit 5d90afd. ± Comparison against base commit 1e435a2.

@github-actions GitHub Actions
Copy link

Integration Tests - Windows_VS_x86_Debug

0 tests  ±0   0 ✅ ±0   0s ⏱️ ±0s
0 suites ±0   0 💤 ±0 
0 files   ±0   0 ❌ ±0 

Results for commit 5d90afd. ± Comparison against base commit 1e435a2.

@github-actions GitHub Actions
Copy link

Integration Tests - Windows_x64_Release

0 tests  ±0   0 ✅ ±0   0s ⏱️ ±0s
1 suites ±0   0 💤 ±0 
1 files   ±0   0 ❌ ±0 

Results for commit 5d90afd. ± Comparison against base commit 1e435a2.

@github-actions GitHub Actions
Copy link

Integration Tests - Windows_x86_Release

0 tests  ±0   0 ✅ ±0   0s ⏱️ ±0s
1 suites ±0   0 💤 ±0 
1 files   ±0   0 ❌ ±0 

Results for commit 5d90afd. ± Comparison against base commit 1e435a2.

@github-actions GitHub Actions
Copy link

Performance Tests - Windows_x64_Release

0 tests  ±0   0 ✅ ±0   0s ⏱️ ±0s
1 suites ±0   0 💤 ±0 
1 files   ±0   0 ❌ ±0 

Results for commit 5d90afd. ± Comparison against base commit 1e435a2.

@github-actions GitHub Actions
Copy link

Performance Tests - Windows_x86_Release

0 tests  ±0   0 ✅ ±0   0s ⏱️ ±0s
1 suites ±0   0 💤 ±0 
1 files   ±0   0 ❌ ±0 

Results for commit 5d90afd. ± Comparison against base commit 1e435a2.

@Automation51D Automation51D merged commit 591332e into version/4.5 Mar 20, 2025
30 checks passed
@Automation51D Automation51D deleted the feature/bug-ip-length branch March 20, 2025 11:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@justadreamer justadreamer justadreamer approved these changes

Assignees

@drasmart drasmart

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jwrosewell @justadreamer @drasmart @Automation51D