Conversation
Summary of ChangesHello @gnunixon, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces comprehensive support for push notifications within the system. It establishes new data models for managing device installations and their delivery statuses, integrates with Firebase Cloud Messaging (FCM) for sending notifications, and provides API endpoints for users to register and update their device information. This enhancement enables the system to send targeted and dynamic push notifications to users across various platforms. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces push notification capabilities by adding an FCM provider, device installation tracking, and the necessary API endpoints. While the feature is a valuable addition, the current implementation has several critical issues that must be addressed. These include a major security vulnerability in credential storage, incomplete placeholder code that will cause runtime failures, and bugs related to undefined methods and model attributes. Additionally, there are significant opportunities for performance improvements in batch processing and database interactions. I've left detailed comments on these points.
| service_account_json = properties.property( | ||
| types.String(), | ||
| required=True, | ||
| ) |
There was a problem hiding this comment.
Storing the service_account_json as a plain string is a critical security vulnerability. This JSON contains sensitive credentials, including private keys. If the database is compromised, these credentials will be exposed. Credentials should be stored in a secure secret manager (like AWS Secrets Manager, HashiCorp Vault, etc.) and referenced here by an identifier, not stored directly.
| existing.app_version = resource.get("app_version", "") | ||
| existing.os_version = resource.get("os_version", "") | ||
| existing.device_model = resource.get("device_model", "") |
There was a problem hiding this comment.
| def _process_batch_result(self, batch_result): | ||
|
|
||
| for r in batch_result.permanent_failures(): | ||
|
|
||
| inst = Installation.objects.get_one( | ||
| filters={"installation_id": r.installation_id} | ||
| ) | ||
|
|
||
| if inst: | ||
| inst.status = c.AlwaysActiveStatus.INACTIVE.value | ||
| inst.save() | ||
|
|
There was a problem hiding this comment.
This method executes a database query for each permanently failed installation within a loop, which is inefficient (N+1 query problem). This should be optimized to use a single query to fetch all relevant installations and then update them. A bulk update operation would be even more efficient if your ORM supports it.
| def _process_batch_result(self, batch_result): | |
| for r in batch_result.permanent_failures(): | |
| inst = Installation.objects.get_one( | |
| filters={"installation_id": r.installation_id} | |
| ) | |
| if inst: | |
| inst.status = c.AlwaysActiveStatus.INACTIVE.value | |
| inst.save() | |
| def _process_batch_result(self, batch_result): | |
| failures = batch_result.permanent_failures() | |
| if not failures: | |
| return | |
| failed_installation_ids = [r.installation_id for r in failures] | |
| # Assuming the ORM supports an IN filter and bulk updates. | |
| # This is an example of how it could be optimized. | |
| Installation.objects.filter( | |
| installation_id__in=failed_installation_ids | |
| ).update(status=c.AlwaysActiveStatus.INACTIVE.value) |
| data={ | ||
| k: jinja2.Template(str(v)).render(**params) | ||
| for k, v in self.data.items() | ||
| }, |
There was a problem hiding this comment.
The use of str(v) to convert all data values to strings before templating can lead to loss of data types and unexpected behavior, especially for booleans and numbers. It's better to only template values that are already strings and pass other types through as-is.
| data={ | |
| k: jinja2.Template(str(v)).render(**params) | |
| for k, v in self.data.items() | |
| }, | |
| data={ | |
| k: jinja2.Template(v).render(**params) if isinstance(v, str) else v | |
| for k, v in self.data.items() | |
| }, |
No description provided.