Skip to content

Validate condition router YAML before parsing - #16446

Open
minnnjuuu wants to merge 3 commits into
apache:3.3from
minnnjuuu:fix/validate-condition-router-yaml
Open

Validate condition router YAML before parsing#16446
minnnjuuu wants to merge 3 commits into
apache:3.3from
minnnjuuu:fix/validate-condition-router-yaml

Conversation

@minnnjuuu

@minnnjuuu minnnjuuu commented Sep 4, 2026

Copy link
Copy Markdown

What is the purpose of the change?

ConditionRuleParser.parse() assumes that SnakeYAML always returns a YAML mapping. However, an empty YAML document returns null, while a sequence or scalar document returns a non-map value. This can result in a NullPointerException or ClassCastException instead of a clear validation error.

Some configuration-center adapters filter a Java null or an exactly empty string, but that does not cover every input that produces a null SnakeYAML result. Non-empty inputs such as a comment-only document, ---, or the explicit YAML value null can pass string-level checks and still load as null.

This change validates the top-level YAML value before parsing it and rejects non-mapping documents with a clear IllegalArgumentException instead of relying on incidental NullPointerException or ClassCastException failures.

What does this change do?

  • Parse the YAML document into an Object before casting it.
  • Require the top-level YAML value to be a mapping.
  • Report a clear IllegalArgumentException for empty, comment-only, sequence, and scalar documents.
  • Add regression coverage for null and non-mapping YAML documents.

Checklist

  • Make sure there is a GitHub_issue field for the change.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Write necessary unit-test to verify your logic correction. If the new feature or significant change is committed, please remember to add sample in dubbo samples project.
  • Make sure gitHub actions can pass. Why the workflow is failing and how to fix it?

@codecov-commenter

codecov-commenter commented Sep 4, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 60.89%. Comparing base (dab47b7) to head (4c80352).

Additional details and impacted files
@@             Coverage Diff              @@
##                3.3   #16446      +/-   ##
============================================
- Coverage     60.91%   60.89%   -0.03%     
- Complexity       15    11745   +11730     
============================================
  Files          1953     1953              
  Lines         89271    89274       +3     
  Branches      13473    13474       +1     
============================================
- Hits          54383    54360      -23     
- Misses        29309    29328      +19     
- Partials       5579     5586       +7     
Flag Coverage Δ
integration-tests-java21 32.08% <0.00%> (-0.06%) ⬇️
integration-tests-java8 32.21% <0.00%> (+<0.01%) ⬆️
samples-tests-java21 32.06% <50.00%> (-0.10%) ⬇️
samples-tests-java8 29.82% <50.00%> (-0.02%) ⬇️
unit-tests-java11 59.15% <100.00%> (-0.01%) ⬇️
unit-tests-java17 58.65% <100.00%> (-0.01%) ⬇️
unit-tests-java21 58.63% <100.00%> (-0.02%) ⬇️
unit-tests-java25 58.59% <100.00%> (-0.02%) ⬇️
unit-tests-java8 59.18% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@LI123456mo LI123456mo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Verified the fix — yaml.load() can return null/non-Map for bad input, old code cast blindly (NPE/CCE), new instanceof Map check + clear exception is correct.

Note: testRejectNonMappingRule actually proves the fix (fails on old code). testCommentOnlyRuleUpdatePreservesPreviousRule passes either way — ListenableStateRouter.process() already has a try/catch around the parse & assign, so routerRule was never actually at risk of getting nulled out, worth checking that @minnnjuuu . PR description overstates that part a bit — real value here is clearer error messages, not data-loss prevention. Not a blocker.

LGTM , @zrlw

@minnnjuuu

Copy link
Copy Markdown
Author

Thanks for pointing this out. You're right: the right-hand side of the assignment is evaluated before routerRule is updated, so the existing exception handling already preserves the previous rule.

I've updated the PR description to focus on explicit YAML validation and clearer exception reporting, and removed the redundant state-preservation test.

@LI123456mo

Copy link
Copy Markdown
Contributor

@minnnjuuu , your test classes are well you can keep both for test purposes, the only issue was the description which you have already modified it clearly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants