feat(crons): Add CronMonitorDataSourceHandler
#97548
Open
+111
−240
Conversation
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
Adding a new `DataSourceHandler` so that we can treat cron `Monitor` rows as a `DataSource`
Comment on lines
+808
to
+820
| for ds in data_sources: | ||
| try: | ||
| monitor_ids.append(int(ds.source_id)) | ||
| except ValueError: | ||
| logger.exception( | ||
| "Invalid DataSource.source_id fetching Monitor", | ||
| extra={"id": ds.id, "source_id": ds.source_id}, | ||
| ) | ||
|
|
||
| qs_lookup = { | ||
| str(monitor.id): monitor for monitor in Monitor.objects.filter(id__in=monitor_ids) | ||
| } | ||
| return {ds.id: qs_lookup.get(ds.source_id) for ds in data_sources} |
There was a problem hiding this comment.
Potential bug: The `DataSourceSerializer` incorrectly maps data sources when some are invalid, leading to data corruption where users see data from the wrong monitor.
- Description: When
bulk_get_query_objectis called with a list of data sources where some have invalidsource_ids, it returnsNonefor the invalid ones. The consumingDataSourceSerializerfilters out theseNonevalues before zipping the serialized results with the original list of data sources. This causes a misalignment in thezip(ds_query_objs, serialized)operation, leading to serialized data from one monitor being incorrectly associated with a different data source. This results in data corruption in the API response, where users might see information belonging to another monitor. - Suggested fix: Modify the
DataSourceSerializerto handleNonevalues without causing list misalignment. Instead of filteringNones before serialization, iterate through the original list, conditionally serialize items, and maintain a list of the same length as the original to ensure thezipoperation pairs items correctly.
severity: 0.8, confidence: 0.95
Did we get this right? 👍 / 👎 to inform future reviews.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #97548 +/- ##
==========================================
- Coverage 80.62% 80.62% -0.01%
==========================================
Files 8560 8560
Lines 376889 376917 +28
Branches 24538 24538
==========================================
+ Hits 303870 303889 +19
- Misses 72649 72658 +9
Partials 370 370 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Adding a new
DataSourceHandlerso that we can treat cronMonitorrows as aDataSource