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

fix: correctly handle []any type #110

Merged
merged 3 commits into from
Nov 29, 2023
Merged

fix: correctly handle []any type #110

merged 3 commits into from
Nov 29, 2023

Conversation

rustatian
Copy link
Member

@rustatian rustatian commented Nov 29, 2023

Reason for This PR

closes: roadrunner-server/roadrunner#1793

Description of Changes

  • []any type was missed in the headers handler.

License Acceptance

By submitting this pull request, I confirm that my contribution is made under
the terms of the MIT license.

PR Checklist

[Author TODO: Meet these criteria.]
[Reviewer TODO: Verify that these criteria are met. Request changes if not]

  • All commits in this PR are signed (git commit -s).
  • The reason for this PR is clearly provided (issue no. or explanation).
  • The description of changes is clear and encompassing.
  • Any required documentation changes (code and docs) are included in this PR.
  • Any user-facing changes are mentioned in CHANGELOG.md.
  • All added/changed functionality is tested.

Summary by CodeRabbit

  • Refactor

    • Improved header conversion logic for better handling of various data types.
    • Enhanced support for nested data structures in message headers.
  • Documentation

    • Updated version information in test configurations to reflect the latest release.
  • New Features

    • Introduced logging capabilities within the header conversion process for improved diagnostics and monitoring.

Signed-off-by: Valery Piashchynski <piashchynski.valery@gmail.com>
@rustatian rustatian added the bug Something isn't working label Nov 29, 2023
@rustatian rustatian requested a review from wolfy-j November 29, 2023 16:40
@rustatian rustatian self-assigned this Nov 29, 2023
Copy link

coderabbitai bot commented Nov 29, 2023

Walkthrough

The recent updates focus on enhancing the convHeaders function within the AMQP jobs module. A new helper function, convHeadersAnyType, has been introduced to streamline the conversion of AMQP headers to a string representation, including support for nested slices of primitives. Additionally, the convHeaders function now requires a zap.Logger parameter to log warnings for unknown header types. These changes aim to address a bug related to AMQP headers with list types not being correctly mapped.

Changes

File Path Change Summary
amqpjobs/conv.go Refactored convHeaders to use convHeadersAnyType; added logging; support for nested slices.
tests/jobs_amqp_test.go
amqpjobs/conv_test.go
Imported zap package; updated tests to use zap.Logger.
amqpjobs/item.go Added log parameter to unpack method calls of convHeaders.

Assessment against linked issues

Objective Addressed Explanation
#1793: Jobs AMQP headers with list type not mapped to RR job The refactoring of convHeaders and the addition of convHeadersAnyType address the issue of mapping list type headers.

Poem

In the code where headers nest,
A rabbit fixed a bug with zest.
With logs and types now in their place,
AMQP jobs run a swifter race. 🐇💨


Tips

Chat with CodeRabbit Bot (@coderabbitai)

  • If you reply to a review comment from CodeRabbit, the bot will automatically respond.
  • To engage with CodeRabbit bot directly around the specific lines of code in the PR, mention @coderabbitai in your review comment
  • Note: Review comments are made on code diffs or files, not on the PR overview.
  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

Note: For conversation with the bot, please use the review comments on code diffs or files.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • The JSON schema for the configuration file is available here.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 3

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 2c9967b and ab3fe55.
Files ignored due to filter (1)
  • go.work.sum
Files selected for processing (2)
  • amqpjobs/conv.go (1 hunks)
  • tests/jobs_amqp_test.go (1 hunks)
Additional comments: 2
amqpjobs/conv.go (1)
  • 41-42: The conversion of float32 to string is done by casting to float64 and then formatting. This is necessary because strconv.FormatFloat requires a float64. However, be aware that this cast could potentially lead to precision issues. Ensure that this precision is acceptable for the application's requirements.
tests/jobs_amqp_test.go (1)
  • 693-699: The change in the Version field from "2.9.0" to "2023.3.7" indicates a shift from semantic versioning to date-based versioning. Ensure that this new versioning scheme is consistently applied across the entire codebase and documentation.

amqpjobs/conv.go Outdated Show resolved Hide resolved
amqpjobs/conv.go Outdated Show resolved Hide resolved
amqpjobs/conv.go Outdated Show resolved Hide resolved
Signed-off-by: Valery Piashchynski <piashchynski.valery@gmail.com>
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 2

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between ab3fe55 and b7fbad4.
Files selected for processing (3)
  • amqpjobs/conv.go (1 hunks)
  • amqpjobs/conv_test.go (2 hunks)
  • amqpjobs/item.go (1 hunks)
Additional comments: 7
amqpjobs/conv.go (2)
  • 10-17: The summary states that the return type of convHeaders has been changed, but both the summary and the hunk indicate that the return type was and still is map[string][]string. Please clarify if there was actually a change in the return type.

  • 20-67: The implementation of convHeadersAnyType looks solid and handles a variety of types, including recursive handling for []any. Good use of logging for unknown types.

amqpjobs/conv_test.go (2)
  • 5-9: The import of go.uber.org/zap is correctly added to support the new logging functionality in the TestConv function.

  • 39-39: Ensure that the rest of the test cases are updated if necessary to reflect any changes in the convHeaders function behavior, especially considering the new return type map[string][]string.

amqpjobs/item.go (3)
  • 241-247: The summary states that the unpack method now takes an additional log argument, but the code shows that convHeaders is being called with d.log. The unpack method signature itself has not changed. Please clarify if the summary should be updated to reflect that the change is within the unpack method's body, not its signature.

  • 244-244: The change to pass d.log to convHeaders aligns with the summary and is correctly implemented.

  • 242-247: The unpack function correctly initializes a new Item and sets its headers field using the convHeaders function.

amqpjobs/conv_test.go Outdated Show resolved Hide resolved
amqpjobs/conv_test.go Outdated Show resolved Hide resolved
Signed-off-by: Valery Piashchynski <piashchynski.valery@gmail.com>
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between b7fbad4 and 756a884.
Files selected for processing (1)
  • amqpjobs/conv_test.go (2 hunks)
Additional comments: 4
amqpjobs/conv_test.go (4)
  • 5-9: The import of go.uber.org/zap is correctly added to support the new logging functionality.

  • 36-37: Proper initialization and error handling for the zap.Logger instance.

  • 38-38: The convHeaders function call is correctly updated to include the new log parameter.

  • 40-41: The test assertions for float values are correctly checking the expected string representations.

@rustatian rustatian merged commit 454826f into master Nov 29, 2023
8 checks passed
@rustatian rustatian deleted the fix/any-slice-headers branch November 29, 2023 17:41
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.

[🐛 BUG]: Jobs AMQP headers with list type not mapped to RR job
1 participant