Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overall this is a clean, focused improvement. The two changes are logically independent and both add value.
connector_type in log context — straightforward and useful for observability. No issues.
Domain validation message improvements — the refactoring is well-motivated: separating the human-readable guidance from the exception message makes the error_details structured log field cleaner (see connection_exception_details), and the new message labels (enforcement mode - monitor / enforcement mode - enabled) are more informative than the old monitor mode. A few minor points in the inline comments:
- f-string vs loguru lazy evaluation — nit-level, but the previous code used loguru's preferred
{}placeholder style. - Double-logging — the explicit
logger.error()beforeraisemeans the event will appear twice at error level. The PR description already calls this out; the inline comment offers one alternative approach if the team wants to eliminate the duplication. get_log_context()ordering — safe as written, minor note for future readers.
No blocking issues. The exception message content (violation_msg) is unchanged, so existing tests that match on "not in the list of allowed values" and "The value 'evil.com' for 'domain'" continue to pass without modification.
| logger.info( | ||
| "To add support for this domain, contact Ethyca support." | ||
| ) | ||
| raise DomainValidationError(violation_msg) |
There was a problem hiding this comment.
The explicit logger.error(...) on line 91 followed by raise DomainValidationError(...) here will produce two error-level log entries for the same event: the one logged explicitly, and the one logged when the upstream handler catches the exception (e.g. connection_exception_details + whatever logs it). You've flagged this in the PR description, which is appreciated.
One alternative worth considering: drop the explicit logger.error() here and instead emit the guidance lines at the catch-site, right before the upstream handler re-logs the exception. That way the violation appears once, immediately followed by the guidance. It does require touching authenticated_client.py (or wherever DomainValidationError is caught), so if keeping the change self-contained is more important than the duplicate log line, the current approach is acceptable.
There was a problem hiding this comment.
This is probably not a super common error that folks are going to hit, so I'm ok with duplicated error lines (unless a reviewer feels otherwise).
As written, it'll produce logs like this:
ERROR Domain validation violation (enforcement mode - enabled): The value 'evil.attacker.com' for 'host' is not in the list of allowed values: [api.stripe.com, *.stripe.com].
INFO To change domain validation behavior, set FIDES__SECURITY__DOMAIN_VALIDATION_MODE to 'monitor' or 'disabled'.
INFO To add support for this domain, contact Ethyca support.
ERROR Connector request failed. [error_group=domain_blocked, error_details="The value 'evil.attacker.com' for 'host' is not in the list of allowed values: [...]", status_code=None]
There was a problem hiding this comment.
would this also mean duplicated error notifications on 'save' on the integration setup tab? i think it might be annoying enough
There was a problem hiding this comment.
what i mean is i dont mind them being repeated in the logs but two errors popping up in the ui does seem kinda bad
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7820 +/- ##
========================================
Coverage 85.01% 85.02%
========================================
Files 614 622 +8
Lines 40094 40427 +333
Branches 4671 4708 +37
========================================
+ Hits 34087 34373 +286
- Misses 4965 4998 +33
- Partials 1042 1056 +14 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2 participants
Description Of Changes
Improve domain validation violation messages by standardizing the warn/error as:
Domain validation violation (enforcement mode - <mode>): <violation message>".Unfortunate caveat here is that this means that we'll have two error-level log lines with more or less the same contents when the enforcement is enabled. There's the initial
logger.error()and then it'll get logged again when theDomainValidationErrorgets caught ("Connector request failed" and then the error in as structured fields). I'm alright with that, but looking for other suggestions/feedback.Additionally, I'm adding the saas integration type to the log context for better connector observability.
Code Changes
Steps to Confirm
Pre-Merge Checklist
CHANGELOG.mdupdatedmaindowngrade()migration is correct and works