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(gateway): don't log IllegalArgumentException as errors #11169

Conversation

skayliu
Copy link
Contributor

@skayliu skayliu commented Dec 4, 2022

Description

IllegalArgumentException get thrown when a user passes variables in their request which cannot be converted to message pack. Currently these are being logged as errors. Since this a fault by the user we should log these as debug.

User input should not lead to reported errors, but user command should be rejected with a helpful message.

Related issues

closes #9832

Definition of Done

Not all items need to be done depending on the issue and the pull request.

Code changes:

  • The changes are backwards compatibility with previous versions
  • If it fixes a bug then PRs are created to backport the fix to the last two minor versions. You can trigger a backport by assigning labels (e.g. backport stable/1.3) to the PR, in case that fails you need to create backports manually.

Testing:

  • There are unit/integration tests that verify all acceptance criterias of the issue
  • New tests are written to ensure backwards compatibility with further versions
  • The behavior is tested manually
  • The change has been verified by a QA run
  • The impact of the changes is verified by a benchmark

Documentation:

  • The documentation is updated (e.g. BPMN reference, configuration, examples, get-started guides, etc.)
  • If the PR changes how BPMN processes are validated (e.g. support new BPMN element) then the Camunda modeling team should be informed to adjust the BPMN linting.

Please refer to our review guidelines.

@skayliu
Copy link
Contributor Author

skayliu commented Dec 5, 2022

Hi, @saig0, @korthout . Please take a moment to review this fix when you have time. Thank you.

@github-actions
Copy link
Contributor

github-actions bot commented Dec 6, 2022

Test Results

   962 files  ±    0     962 suites  ±0   1h 50m 11s ⏱️ - 3m 12s
7 843 tests +174  7 836 ✔️ +174  7 💤 ±0  0 ±0 
8 050 runs  +174  8 041 ✔️ +174  9 💤 ±0  0 ±0 

Results for commit 69ae007. ± Comparison against base commit 659571b.

♻️ This comment has been updated with latest results.

@saig0
Copy link
Member

saig0 commented Dec 6, 2022

@skayliu thank you for your contribution. 🎉 We'll have a look at your PR in the next few days. 👀

Copy link
Contributor

@remcowesterhoud remcowesterhoud left a comment

Choose a reason for hiding this comment

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

Hi @skayliu! Thanks for the changes, I have tested it locally and it all seems to be working fine. I have one question about the changes though, and a small comment.

🔧 Please also add a testcase to the RequestMapperTest

@skayliu
Copy link
Contributor Author

skayliu commented Dec 14, 2022

Please also add a testcase to the RequestMapperTest

@remcowesterhoud, Done, please check this out. Thank you.

Copy link
Contributor

@remcowesterhoud remcowesterhoud left a comment

Choose a reason for hiding this comment

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

Thanks for the fix @skayliu 🙇

bors merge

IllegalArgumentException get thrown when a user passes variables in their request which cannot be converted to message pack. Currently these are being logged as errors. Since this a fault by the user we should log these as debug.
@skayliu skayliu force-pushed the 9832-IllegalArgumentException-error-log branch from 538d938 to 69ae007 Compare December 15, 2022 01:01
@skayliu
Copy link
Contributor Author

skayliu commented Dec 15, 2022

Hi, @remcowesterhoud, It seems to need to rebase to main. Done, Please try again, Thank you.

Copy link
Contributor

@remcowesterhoud remcowesterhoud left a comment

Choose a reason for hiding this comment

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

bors merge

zeebe-bors-camunda bot added a commit that referenced this pull request Dec 15, 2022
11169: fix(gateway): don't log IllegalArgumentException as errors r=remcowesterhoud a=skayliu

## Description

IllegalArgumentException get thrown when a user passes variables in their request which cannot be converted to message pack. Currently these are being logged as errors. Since this a fault by the user we should log these as debug.

User input should not lead to reported errors, but user command should be rejected with a helpful message.

## Related issues

<!-- Which issues are closed by this PR or are related -->

closes #9832



Co-authored-by: skayliu <skay463@163.com>
@zeebe-bors-camunda
Copy link
Contributor

Build failed:

@remcowesterhoud
Copy link
Contributor

bors retry

@zeebe-bors-camunda
Copy link
Contributor

Build succeeded:

@zeebe-bors-camunda zeebe-bors-camunda bot merged commit 066501a into camunda:main Dec 15, 2022
@remcowesterhoud
Copy link
Contributor

/backport

@backport-action
Copy link
Collaborator

Backport failed for stable/8.0, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally.

git fetch origin stable/8.0
git worktree add -d .worktree/backport-11169-to-stable/8.0 origin/stable/8.0
cd .worktree/backport-11169-to-stable/8.0
git checkout -b backport-11169-to-stable/8.0
ancref=$(git merge-base 659571bbc2651a6a5728428745a457aabe411089 69ae007c7961a29395f428244a9d94a1426e6963)
git cherry-pick -x $ancref..69ae007c7961a29395f428244a9d94a1426e6963

@backport-action
Copy link
Collaborator

Successfully created backport PR #11273 for stable/8.1.

@remcowesterhoud remcowesterhoud mentioned this pull request Dec 15, 2022
14 tasks
zeebe-bors-camunda bot added a commit that referenced this pull request Dec 15, 2022
11273: [Backport stable/8.1] fix(gateway): don't log IllegalArgumentException as errors r=remcowesterhoud a=backport-action

# Description
Backport of #11169 to `stable/8.1`.

relates to #9832

Co-authored-by: skayliu <skay463@163.com>
zeebe-bors-camunda bot added a commit that referenced this pull request Dec 15, 2022
11277: Backport 11169 to stable 8.0 r=remcowesterhoud a=remcowesterhoud

## Description

<!-- Please explain the changes you made here. -->
Manual backport of #11169 to stable 8.0 because of a merge conflicts in the `RequestMapperTest`.

## Related issues

<!-- Which issues are closed by this PR or are related -->

closes #



Co-authored-by: skayliu <skay463@163.com>
@skayliu skayliu deleted the 9832-IllegalArgumentException-error-log branch December 16, 2022 13:11
@Zelldon Zelldon added the version:8.1.6 Marks an issue as being completely or in parts released in 8.1.6 label Jan 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
version:8.1.6 Marks an issue as being completely or in parts released in 8.1.6
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MessagePack cannot serialize BigInteger larger than 2^64-1
5 participants