Skip to content

feat: log expression compilation errors and replace single quotes in ExpressionHandler#397

Merged
sagilio merged 5 commits into
masterfrom
copilot/log-expression-handler-errors
Mar 6, 2026
Merged

feat: log expression compilation errors and replace single quotes in ExpressionHandler#397
sagilio merged 5 commits into
masterfrom
copilot/log-expression-handler-errors

Conversation

Copilot AI commented Feb 25, 2026

Copy link
Copy Markdown
Contributor
  • Add Logger property to ExpressionHandler (under #if !NET452) and log warnings when expression compilation fails in TryCompileExpression
  • Replace single quotes with double quotes in expressions to handle known DynamicExpresso issue — moved to EnforceView.CreateWithMatcher so normalization happens once at view creation rather than on every Invoke call
  • Propagate Logger from Enforcer to ExpressionHandler via property setters only (not on every enforce call)
  • Update MockLogger to capture log entries in a Logs list for test assertions
  • Fix MockLogger to pass exception (not null) to formatter so exception details appear in log messages
  • Add tests to verify single-quote replacement and warning logging behavior
  • All 365 tests pass
Original prompt

This section details on the original issue you should resolve

<issue_title>Logging ExpressionHandler exception(s)</issue_title>
<issue_description>1. Multiple times I was stumbled why some policies wouldn't work - until I realized that the expressions had some invalid syntax yet the TryExpressionBuilder was hiding the exception:

https://github.com/casbin/Casbin.NET/blob/6acec871fbabfd6aa3008c27edd57d3a21f3999e/Casbin/Evaluation/ExpressionHandler.cs#L120

IMO we should log the error using the standard logger... I am also not sure why there is no try&catch around CompileExpression() - as there could be errors in there as well.

  1. FYI - the actual Interpretor (DynamicExpresso) has known issues with single quotes - might be useful to change those before the actual evaluation to double quotes, e.g.
expressionString = expressionString.Replace('\'', '"');

Thanks</issue_description>

Comments on the Issue (you are @copilot in this section)


✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@hsluoyz hsluoyz marked this pull request as ready for review February 25, 2026 16:02
…sions

Co-authored-by: hsluoyz <3787410+hsluoyz@users.noreply.github.com>
Copilot AI changed the title [WIP] Log ExpressionHandler exceptions during evaluation Log expression compilation errors and replace single quotes in ExpressionHandler Feb 25, 2026

@sagilio sagilio left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@copilot Maybe it is necessary to set the logger at every enforce call

Comment thread Casbin/Enforcer.Internal.cs Outdated
…Enforce

Co-authored-by: sagilio <42855245+sagilio@users.noreply.github.com>

Copilot AI commented Feb 28, 2026

Copy link
Copy Markdown
Contributor Author

@copilot Maybe it is necessary to set the logger at every enforce call

Fixed in 16f3bc0. The logger is now only propagated when Enforcer.Logger or Enforcer.Model is set (via property setters), not on every enforce call.

@sagilio

sagilio commented Feb 28, 2026

Copy link
Copy Markdown
Member

@copilot Plz fix the PR title for the Semantic PR check

Copilot AI commented Feb 28, 2026

Copy link
Copy Markdown
Contributor Author

@copilot Plz fix the PR title for the Semantic PR check

I'm unable to directly update the PR title through the tools available to me. Could you please rename the PR title to follow Conventional Commits format? The suggested title would be:

feat: log expression compilation errors and replace single quotes in ExpressionHandler

Copilot AI changed the title Log expression compilation errors and replace single quotes in ExpressionHandler feat: log expression compilation errors and replace single quotes in ExpressionHandler Feb 28, 2026
@sagilio

sagilio commented Mar 4, 2026

Copy link
Copy Markdown
Member

@copilot Can you fix this: Semantic / Semantic check (pull_request)

Co-authored-by: sagilio <42855245+sagilio@users.noreply.github.com>

Copilot AI commented Mar 4, 2026

Copy link
Copy Markdown
Contributor Author

@copilot Can you fix this: Semantic / Semantic check (pull_request)

The Semantic check was failing because the PR title lacked a conventional commit prefix. The title was updated to feat: ... and I pushed commit 7a4f4f6 to trigger a new CI run — the Semantic check should now pass.

@sagilio

sagilio commented Mar 6, 2026

Copy link
Copy Markdown
Member

@copilot Plz move expressionString.Replace to create enforcer view

…tcher

Co-authored-by: sagilio <42855245+sagilio@users.noreply.github.com>

Copilot AI commented Mar 6, 2026

Copy link
Copy Markdown
Contributor Author

@copilot Plz move expressionString.Replace to create enforcer view

Done in d8f91fa. The Replace('\'', '"') is now applied at the start of EnforceView.CreateWithMatcher, normalizing the matcher string once at view creation time. It's been removed from ExpressionHandler.Invoke.

@sagilio sagilio self-assigned this Mar 6, 2026

@sagilio sagilio left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@sagilio sagilio merged commit f6fa6e4 into master Mar 6, 2026
7 checks passed
@github-actions

github-actions Bot commented Mar 6, 2026

Copy link
Copy Markdown

🎉 This PR is included in version 2.20.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Logging ExpressionHandler exception(s)

4 participants