Remove obsolete JSON format support from Live Data Server#57
Remove obsolete JSON format support from Live Data Server#57darshdinger merged 7 commits intonextfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## next #57 +/- ##
==========================================
- Coverage 89.97% 85.51% -4.46%
==========================================
Files 12 12
Lines 359 283 -76
==========================================
- Hits 323 242 -81
- Misses 36 41 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughThe changes implement a simplification of the plot data management system by removing support for multiple data types and associated features. The system now exclusively supports HTML data storage, eliminating JSON handling and user-data upload workflows. This involves updating the data type constants and removing related model methods, removing URL patterns for JSON and user endpoints, simplifying view functions to eliminate JSON retrieval paths, reducing utility functions to focus on HTML-based operations, and streamlining tests to verify only the remaining HTML-based functionality. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/apps/plots/models.py (1)
59-70: Update stale documentation referencing JSON.The docstring on line 60 and comments on lines 64-66 and 69 still reference JSON data type support, which has been removed. Consider updating these to reflect the HTML-only design.
📝 Suggested documentation update
class PlotData(models.Model): - """Table of plot data. This data can either be json or html""" + """Table of plot data. This data is stored as HTML.""" ## DataRun this run status belongs to data_run = models.ForeignKey(DataRun, on_delete=models.deletion.CASCADE) - ## Data type: - ## type = 100 for live data, 0 static reduced. - ## type += 1 for HTML, 0 for JSON + ## Data type: 1 for HTML (JSON support removed) + ## Legacy: type = 100 for live data, 0 for static reduced data_type = models.IntegerField() - ## JSON/HTML data + ## HTML data data = models.TextField()tests/test_expiration.py (2)
116-127: Extract duplicate_generate_keyto a shared test utility.This function is duplicated verbatim in
tests/test_post_get.py(lines 142-153). Consider extracting it to a shared test utilities module (e.g.,tests/conftest.pyortests/utils.py) to maintain DRY principles.
73-83: Consider adding timeout to test HTTP requests.The
requests.getcalls lack timeout parameters, which could cause tests to hang indefinitely if the server is unresponsive. This is flagged by static analysis (S113).🔧 Add timeout to requests
response1 = requests.get( f"{TEST_URL}/plots/{instrument}/12345/update/html/", headers={"Authorization": _generate_key(instrument, 12345)}, + timeout=30, ) assert response1.status_code == HTTP_OK response2 = requests.get( f"{TEST_URL}/plots/{instrument}/12346/update/html/", headers={"Authorization": _generate_key(instrument, 12346)}, + timeout=30, ) assert response2.status_code == HTTP_OKtests/test_post_get.py (1)
142-153: Duplicate helper function.As noted in
tests/test_expiration.py, this_generate_keyfunction is duplicated. Consider consolidating into a shared test utility.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/apps/plots/models.pysrc/apps/plots/urls.pysrc/apps/plots/view_util.pysrc/apps/plots/views.pytests/test_expiration.pytests/test_post_get.py
💤 Files with no reviewable changes (1)
- src/apps/plots/urls.py
🧰 Additional context used
🧬 Code graph analysis (3)
tests/test_expiration.py (1)
tests/test_post_get.py (1)
_generate_key(142-153)
src/apps/plots/views.py (1)
src/apps/plots/view_util.py (2)
get_plot_data(107-117)store_plot_data(120-143)
src/apps/plots/view_util.py (1)
src/apps/plots/models.py (1)
PlotData(59-87)
🪛 Ruff (0.14.10)
tests/test_expiration.py
73-73: Probable use of requests call without timeout
(S113)
79-79: Probable use of requests call without timeout
(S113)
127-127: Probable use of insecure hash functions in hashlib: sha1
(S324)
tests/test_post_get.py
124-124: Probable use of requests call without timeout
(S113)
🔇 Additional comments (8)
src/apps/plots/models.py (1)
15-16: LGTM on the data type simplification.The consolidation to HTML-only data types aligns with the PR objectives. Retaining the
data_typefield in the model for backward compatibility is a reasonable approach.src/apps/plots/view_util.py (1)
107-117: LGTM on the simplified data retrieval.The removal of
data_typeparameter and per-type validation logic correctly aligns with the HTML-only consolidation. The length check before accessing the first element is appropriate.tests/test_expiration.py (1)
107-113: LGTM on updated purge assertions.The assertion updates from 2 to 1 correctly reflect that only one non-expired run should remain after purging (the run with run_id=12345, since run_id=12346 was created with an expiration date 3 years in the past).
tests/test_post_get.py (1)
122-139: LGTM on simplified test cases.The test simplifications correctly reflect the removal of JSON endpoints and user-data workflows. The unauthorized and session tests now focus on the remaining HTML upload functionality.
src/apps/plots/views.py (4)
10-17: LGTM on updated imports.The import changes correctly reflect the simplified data handling:
JsonResponseremoved,DATA_TYPESimported from models for consistent type handling.
54-63: LGTM on simplified HTML retrieval.The
update_as_htmlview now correctly callsget_plot_datawithout the data_type parameter, aligning with the simplified view_util function.
66-84: LGTM on simplified storage logic.The
_storefunction correctly:
- Uses
DATA_TYPES["html"]for consistent type handling- Maintains expiration date functionality
- Returns 400 for missing file data
The removal of
as_userbranching and JSON-specific logic cleanly simplifies the code.
87-94: LGTM on simplified upload endpoint.The
upload_plot_dataview now directly delegates to_storewithout theas_userparameter, which aligns with the removal of user-data upload functionality.
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
backmari
left a comment
backmari
left a comment
There was a problem hiding this comment.
This PR removes obsolete JSON handling from Live Data Server. Looks good 👌
2 participants
Short description of the changes:
Removed the obsolete JSON data format option for publishing plot data. The server now only accepts HTML format data. The
data_typefield remains in the database model for backward compatibility but is effectively always set to 1 (HTML).Long description of the changes:
This PR addresses issues with the fragile JSON format detection logic (the
<divstring check inget_data_type_from_data()method) that caused problems with REF_M data processing.Changes made:
DATA_TYPESconstant from{"json": 0, "html": 1}to{"html": 1}/update/json/)/upload_user_data/and/list/)get_data_type_from_data()andget_data_type_from_string()helper methods_store()function to always use HTML data typestore_user_data()from view utilitiesThe database schema remains unchanged - the
data_typefield is preserved to avoid migration complexity and maintain backward compatibility with existing data.Check list for the pull request
Check list for the reviewer
Manual test for the reviewer
docker compose up -dpixi shellthenpytestReferences
<divstring detection