Repository navigation
[java] Fix #7216: GuardLogStatement: accept logstash structured arguments - #7225
Conversation
zbynek
left a comment
zbynek
left a comment
There was a problem hiding this comment.
It's not clear to me how pedantic this rule should be -- calling a constructor for even a lightweight object still creates overhead.
Assuming we're okay with that constructor call (as the linked issue suggests), the rule should still not allow value("user", slowMethodToGetUser()) as an argument.
|
Good catch @zbynek. Updated |
zbynek
left a comment
zbynek
left a comment
There was a problem hiding this comment.
The logic looks good now, thanks. Please resolve the release notes merge conflict.
|
Resolved the release notes conflict against main. |
|
Compared to main: (comment created at 2026-11-08 20:35:49+00:00 for c20c126) |
2fc01cb to
cf5e380
Compare
) log.debug("user={}", value("user", user)) was flagged although its argument cost is the same as plain parameter substitution: the logstash helper call returns a StructuredArgument whose string rendering is deferred to the encoder. Treat arguments of type net.logstash.logback.argument.StructuredArgument as low overhead, covering value, keyValue, entries and user helpers returning the same type. Arbitrary method calls still require a guard. Adds logstash-logback-encoder as a test-scoped dependency so the regression test resolves the argument type.
cf5e380 to
39f72b0
Compare
(excluding indirect recursion)
adangel
changed the title
Describe the PR
log.debug("user={}", value("user", user))was flagged by GuardLogStatement although the logstashStructuredArgumentshelpers only wrap the argument for the structured encoder; the string rendering stays deferred exactly like plain{}parameter substitution, so a level guard buys nothing.The rule now treats any argument of type
net.logstash.logback.argument.StructuredArgumentas low overhead, which coversvalue,keyValue,entries,deferand user factories returning the same type. Arbitrary method calls are still flagged.A test-scoped
logstash-logback-encoderdependency was added so the regression test can resolve the argument type, matching the existing typeresolution test deps.Related issues
value(...)#7216Ready?
GuardLogStatementXML:value/keyValue/entriesunflagged, a plain helper call still flagged)GuardLogStatementTest, red-verified without the rule change: 4 violations instead of 1)./mvnw verifyat the project root (module compile, test, checkstyle and pmd:check run locally for pmd-java)