Refine Firebase mock fallback to selectively handle initialization ValueError#133
Merged
Merged
Conversation
The fallback mock database functionality in `firebase_utils.py` (which previously resided in `action_devices.py`) was erroneously catching all exceptions. This masked actual runtime issues such as network connectivity or permission errors by incorrectly returning mock data in production. This commit refactors the `try...except` block to precisely catch `ValueError`, which is uniquely raised by the `firebase-admin` SDK when an app is imported but not initialized (e.g. missing credentials during local development). It also adds comprehensive unit tests to ensure this fallback works correctly without suppressing other errors. Co-authored-by: DaTiC0 <13198638+DaTiC0@users.noreply.github.com>
Contributor
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Contributor
Reviewer's GuideNarrows the Firebase reference() error handling to only treat ValueError as an initialization issue that triggers a MockRef fallback, and adds tests to verify that ValueError leads to the fallback while other exceptions propagate. Flow diagram for refined Firebase reference error handlingflowchart TD
A[call reference user_id] --> B{Firebase db.reference call}
B --> C{ValueError raised?}
C -->|yes| D[log warning Firebase not initialized]
D --> E[return MockRef]
C -->|no| F{Other exception raised?}
F -->|yes| G[exception bubbles up]
F -->|no| H[return real db.reference]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The tests repeatedly manipulate
sys.modulesto force a re-import offirebase_utils; consider extracting this into a small helper or usingimportlib.reloadto reduce duplication and make the module reloading behavior less brittle.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The tests repeatedly manipulate `sys.modules` to force a re-import of `firebase_utils`; consider extracting this into a small helper or using `importlib.reload` to reduce duplication and make the module reloading behavior less brittle.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request refactors the exception handling in firebase_utils.py to catch ValueError instead of a generic Exception when initializing the Firebase reference, and introduces a new test suite to verify this behavior. The reviewer recommends avoiding sys.modules manipulation in the tests to prevent potential side effects and test isolation issues, suggesting a cleaner mocking approach using unittest.mock.patch instead.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Comment on lines
+1
to
+45
| import unittest | ||
| from unittest.mock import patch, MagicMock | ||
|
|
||
| class TestFirebaseUtils(unittest.TestCase): | ||
| def test_reference_value_error_fallback(self): | ||
| # We must clear the module to reload it with our mocked FIREBASE_AVAILABLE state | ||
| import sys | ||
| if 'firebase_utils' in sys.modules: | ||
| del sys.modules['firebase_utils'] | ||
|
|
||
| # Mock firebase_admin and db to simulate an uninitialized state where db.reference raises ValueError | ||
| mock_db = MagicMock() | ||
| mock_db.reference.side_effect = ValueError("The default Firebase app does not exist.") | ||
|
|
||
| with patch.dict(sys.modules, {'firebase_admin': MagicMock(db=mock_db)}): | ||
| import firebase_utils | ||
|
|
||
| # FIREBASE_AVAILABLE should be True because the import succeeds (mocked) | ||
| self.assertTrue(firebase_utils.FIREBASE_AVAILABLE) | ||
|
|
||
| with patch('firebase_utils.logger.warning') as mock_logger: | ||
| ref = firebase_utils.reference() | ||
|
|
||
| # Should fallback to MockRef | ||
| self.assertIsInstance(ref, firebase_utils.MockRef) | ||
| mock_logger.assert_called_once() | ||
| self.assertIn("Firebase not initialized", mock_logger.call_args[0][0]) | ||
|
|
||
| def test_reference_other_exception_bubbles_up(self): | ||
| import sys | ||
| if 'firebase_utils' in sys.modules: | ||
| del sys.modules['firebase_utils'] | ||
|
|
||
| mock_db = MagicMock() | ||
| # Some other error like network permission denied | ||
| mock_db.reference.side_effect = PermissionError("Permission denied.") | ||
|
|
||
| with patch.dict(sys.modules, {'firebase_admin': MagicMock(db=mock_db)}): | ||
| import firebase_utils | ||
|
|
||
| with self.assertRaises(PermissionError): | ||
| firebase_utils.reference() | ||
|
|
||
| if __name__ == '__main__': | ||
| unittest.main() |
Contributor
There was a problem hiding this comment.
Manipulating sys.modules by deleting and reloading modules during tests can lead to unexpected side effects, such as breaking other tests or causing issues with modules that have already imported firebase_utils (e.g., action_devices).
Instead of reloading the module, you can use unittest.mock.patch to directly mock firebase_utils.FIREBASE_AVAILABLE and firebase_utils.db (using create=True to handle cases where db is not defined due to a missing firebase_admin installation). This is much cleaner, more idiomatic, and avoids test isolation issues.
import unittest
from unittest.mock import patch, MagicMock
import firebase_utils
class TestFirebaseUtils(unittest.TestCase):
@patch('firebase_utils.FIREBASE_AVAILABLE', True)
@patch('firebase_utils.db', create=True)
def test_reference_value_error_fallback(self, mock_db):
mock_db.reference.side_effect = ValueError("The default Firebase app does not exist.")
with patch('firebase_utils.logger.warning') as mock_logger:
ref = firebase_utils.reference()
# Should fallback to MockRef
self.assertIsInstance(ref, firebase_utils.MockRef)
mock_logger.assert_called_once()
self.assertIn("Firebase not initialized", mock_logger.call_args[0][0])
@patch('firebase_utils.FIREBASE_AVAILABLE', True)
@patch('firebase_utils.db', create=True)
def test_reference_other_exception_bubbles_up(self, mock_db):
mock_db.reference.side_effect = PermissionError("Permission denied.")
with self.assertRaises(PermissionError):
firebase_utils.reference()
if __name__ == '__main__':
unittest.main()The fallback mock database functionality in `firebase_utils.py` (which previously resided in `action_devices.py`) was erroneously catching all exceptions. This masked actual runtime issues such as network connectivity or permission errors by incorrectly returning mock data in production. This commit refactors the `try...except` block to precisely catch `ValueError`, which is uniquely raised by the `firebase-admin` SDK when an app is imported but not initialized (e.g. missing credentials during local development). It also adds comprehensive unit tests to ensure this fallback works correctly without suppressing other errors. Additionally, this commit updates `tests/test_multi_tenant.py` to fix unit tests that were broken by the refactoring, replacing broad module mocks with precise functional mocks where needed. Co-authored-by: DaTiC0 <13198638+DaTiC0@users.noreply.github.com>
The fallback mock database functionality in `firebase_utils.py` (which previously resided in `action_devices.py`) was erroneously catching all exceptions. This masked actual runtime issues such as network connectivity or permission errors by incorrectly returning mock data in production. This commit refactors the `try...except` block to precisely catch `ValueError`, which is uniquely raised by the `firebase-admin` SDK when an app is imported but not initialized (e.g. missing credentials during local development). It also adds comprehensive unit tests to ensure this fallback works correctly without suppressing other errors. Additionally, this commit updates `tests/test_multi_tenant.py` to fix unit tests that were broken by the refactoring, replacing broad module mocks with precise functional mocks where needed. Co-authored-by: DaTiC0 <13198638+DaTiC0@users.noreply.github.com>
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
1 participant
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.
reference()infirebase_utils.pyto catchValueErrorinstead of a broadException.tests/test_firebase_utils.pyto assert thatValueErrorproperly triggers the mock fallback, while other exceptions correctly bubble up.PR created automatically by Jules for task 16749792168740504321 started by @DaTiC0
Summary by Sourcery
Limit Firebase reference fallback to uninitialized-app ValueError and verify behavior with targeted tests.
Bug Fixes:
Tests: