fix(rocketmq): log warning when FLAG or DELAY header is not numeric#4330
Open
daguimu wants to merge 1 commit into
Open
fix(rocketmq): log warning when FLAG or DELAY header is not numeric#4330daguimu wants to merge 1 commit into
daguimu wants to merge 1 commit into
Conversation
RocketMQMessageConverterSupport#getAndWrapMessage swallowed any
Integer.parseInt failure on the MQ_FLAG and DELAY_TIME_LEVEL message
headers with catch (Exception ignored) {}, silently falling back to
flag=0 and delayLevel=0. A producer that set DELAY to a misconfigured
non-numeric value (e.g. via a SpEL expression that produced a String
instead of an int) would have its delayed delivery silently turn into
an immediate send with no log line, no exception, and no way for the
caller to know the header was malformed.
Add an SLF4J logger to the support class and warn with both header
names and their actual values when parsing fails. Behaviour is
otherwise unchanged: flag and delayLevel still fall back to 0, the
broad catch (Exception) is kept, and the DELAY_TIME_LEVEL header lookup
remains inside the try block so any exception path that the original
code happened to absorb is still absorbed. Only the variable
declaration for delayLevelObj is hoisted out of the try block so the
catch clause can include its raw value in the warning.
Adds three regression tests that lock in the fallback behaviour:
non-numeric DELAY_TIME_LEVEL stays at 0, non-numeric FLAG stays at 0,
and conversion does not propagate a parse exception. These tests pass
both before and after the fix; their purpose is to prevent a future
refactor from regressing the silent-fallback semantics that downstream
producers may rely on.
6f45a40 to
ac70625
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Describe what this PR does / why we need it
RocketMQMessageConverterSupport#getAndWrapMessageparses theMQ_FLAGandDELAY_TIME_LEVELSpring Messaging headers intointviaInteger.parseInt. The previous implementation wrapped both parses intry { ... } catch (Exception ignored) {}, so any malformed header (e.g. aStringvalue injected via a SpEL expression, a typo, or a non-numeric class) was silently absorbed and the producer fell back toflag=0anddelayLevel=0with no exception, no log line, and no signal to the caller.The most user-visible consequence is on the delayed-delivery path: a misconfigured
DELAY_TIME_LEVEL(e.g."abc"from a SpEL that returned a String instead of an int) turns a delayed message into an immediate send and the application has no way of knowing.Does this pull request fix one issue?
NONE
Describe how you did it
RocketMQMessageConverterSupport.catch (Exception ignored) {}withcatch (Exception e) { log.warn(...) }, including both header names (MQ_FLAG,DELAY_TIME_LEVEL) and their raw values in the warning so the misconfiguration is diagnosable.DELAY_TIME_LEVELheader lookup out of thetryblock so thecatchclause can reference the raw value; both lookups are side-effect-free, so this is a no-op on the happy path. Parse order is unchanged (delayLevelfirst, thenflag).flag=0,delayLevel=0) and the broadcatch (Exception)to avoid any behaviour change for existing producers — only observability is added.Describe how to verify it
mvn -pl spring-cloud-alibaba-starters/spring-cloud-starter-stream-rocketmq -am test(all 13 module tests pass locally, including 3 new ones).New tests in
RocketMQMessageConverterSupportTest:nonNumericDelayTimeLevelHeaderFallsBackToZero— setsDELAY = "not-a-number", assertsrkmqMsg.getDelayTimeLevel() == 0.nonNumericFlagHeaderFallsBackToZero— setsMQ_FLAG = "not-a-number", assertsrkmqMsg.getFlag() == 0.invalidNumericHeaderDoesNotPropagateException— sets both headers to non-numeric values and asserts no exception escapesconvertMessage2MQ.These tests pass both before and after the fix; their role is regression lock-in for the silent-fallback semantics. The warning itself is verified manually by running with
--logging.level.com.alibaba.cloud.stream.binder.rocketmq=WARNand sending a message with a malformedDELAYheader.Special notes for reviews
log.warnis the only externally visible difference.log.warnbecause the fix does not change the message-conversion output. I left them in as regression guards rather than introducing a new log-capture testing pattern (e.g.OutputCaptureExtension) that the module doesn't currently use elsewhere.tryblock's parse order is unchanged —delayLevelparses first, thenflag. This matters in the unlikely case where one is numeric and the other isn't, and I wanted to keep that observable behaviour identical.