You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by "koust6u (via GitHub)" <gi...@apache.org> on 2025/02/15 10:50:07 UTC
[PR] Refactor CSRF Filter: Simplify Logic, Improve Logging, and Enhance Code Readability [tomcat]
koust6u opened a new pull request, #827:
URL: https://github.com/apache/tomcat/pull/827
1. **Simplified CSRF Filter Logic**:
- Applied `early return` to eliminate unnecessary `else` statements, improving code readability.
- Simplified conditionals to reduce duplication and streamline logic.
2. **Extracted Logging Logic**:
- Moved CSRF-related logging logic into a separate `CsrfLoggingHelper` class to improve reusability and maintainability.
- Ensured consistent logging levels (debug, trace) are managed within the helper class.
4. **Simplified Return Handling**:
- Streamlined return statements to improve control flow and eliminate unnecessary condition checks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: [PR] Refactor CSRF Filter: Simplify Logic, Improve Logging, and Enhance Code Readability [tomcat]
Posted by "dsoumis (via GitHub)" <gi...@apache.org>.
dsoumis commented on PR #827:
URL: https://github.com/apache/tomcat/pull/827#issuecomment-2661100096
1) I am not in favor of the helper class. Conditions like if (log.isDebugEnabled()) and if (log.isTraceEnabled()) are widely used in the code and I believe it makes it more transparent when the logging occurs.
2) Since early returns (guard clauses) are not applied to all conditions, removing the else blocks makes the code harder to analyze.
3) The filter is being invoked twice
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: [PR] Refactor CSRF Filter: Simplify Logic, Improve Logging, and Enhance Code Readability [tomcat]
Posted by "koust6u (via GitHub)" <gi...@apache.org>.
koust6u commented on PR #827:
URL: https://github.com/apache/tomcat/pull/827#issuecomment-2661166088
Thank you for the thorough review and detailed feedback. I now understand your concerns regarding the logging helper and the importance of explicitly showing when logging occurs. Your point about the removal of else blocks making the code harder to analyze also makes sense, especially given that early returns were not applied consistently. Additionally, I see the issue with the filter being invoked twice.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: [PR] Refactor CSRF Filter: Simplify Logic, Improve Logging, and Enhance Code Readability [tomcat]
Posted by "koust6u (via GitHub)" <gi...@apache.org>.
koust6u closed pull request #827: Refactor CSRF Filter: Simplify Logic, Improve Logging, and Enhance Code Readability
URL: https://github.com/apache/tomcat/pull/827
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org