Skip to content

Decode byte-string fill values in get_json_serializable_value (fixes #61)#108

Open
arpitjain099 wants to merge 1 commit into
nasa:mainfrom
arpitjain099:chore/bytes-fillvalue-json
Open

Decode byte-string fill values in get_json_serializable_value (fixes #61)#108
arpitjain099 wants to merge 1 commit into
nasa:mainfrom
arpitjain099:chore/bytes-fillvalue-json

Conversation

@arpitjain099

@arpitjain099 arpitjain099 commented Jun 27, 2026

Copy link
Copy Markdown

Description

Fixes #61.

As @owenlittlejohns confirmed in the issue thread, the crash comes from a byte value in umm_var_dict (a _FillValue of b'\x00' on a MISR variable). It passes through get_json_serializable_value untouched, and then publish_umm_var's requests.put(..., json=umm_var_dict) blows up with "Object of type bytes is not JSON serializable".

That function already special-cases numpy ints and floats, so I added a bytes branch right alongside them. Byte strings are decoded with errors='replace' so an undecodable fill value can't itself raise. If you'd rather coerce these upstream (e.g. where the FillValues list is built), I'm glad to move it.

Jira Issue ID

N/A (GitHub issue #61)

Local Test Steps

git clone https://github.com/arpitjain099/earthdata-varinfo.git
cd earthdata-varinfo
git checkout chore/bytes-fillvalue-json
make install
make test

Added a byte-string subtest to test_get_json_serializable_value; test_umm_var.py passes (21), ruff and black clean on the changed files.

PR Acceptance Checklist

  • Jira ticket acceptance criteria met.
  • CHANGELOG.md updated to include high level summary of PR changes.
  • VERSION updated if publishing a release.
  • Tests added/updated and passing.
  • Documentation updated (if needed).

Summary by CodeRabbit

  • Bug Fixes

    • Fixed an issue where publishing JSON could fail when certain metadata values were stored as byte strings.
    • Byte-string values are now converted to text so they can be safely serialized.
    • Improved handling of non-JSON-friendly values to reduce crashes during publishing.
  • Tests

    • Added coverage for byte-string conversion to confirm the result can be encoded and decoded as JSON.
  • Chores

    • Updated the release version to 5.1.2 and added release notes.

Byte strings can appear in a variable's _FillValue metadata attribute
and are not JSON serializable. They reached requests.put(json=...) in
publish_umm_var unchanged and crashed with 'Object of type bytes is not
JSON serializable'. Decode them to text so the UMM-Var record serializes.

Fixes nasa#61.

Signed-off-by: Arpit Jain <arpitjain099@gmail.com>
@coderabbitai

coderabbitai Bot commented Jun 27, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

get_json_serializable_value now decodes bytes inputs to text for JSON serialization, the unit test covers the round-trip, and the release files advance to v5.1.2.

Changes

JSON serialization fix

Layer / File(s) Summary
Decode bytes for JSON values
varinfo/umm_var.py, tests/unit/test_umm_var.py, CHANGELOG.md, VERSION
get_json_serializable_value decodes bytes to UTF-8 text with replacement, the unit test checks JSON round-trip behavior, and the changelog and version file advance to v5.1.2.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~4 minutes

Poem

🐇 I nibbled bytes and found them light,
turned fuzzy bits to text just right.
JSON now hops with a merry thump,
v5.1.2 makes the burrow jump! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Linked Issues check ⚠️ Warning The PR fixes the bytes serialization crash, but it does not add the CMR response validation and exception handling requested in #61. Add checks for CMR response type/content before calling .json() and improve exception handling/debugging in the publish path, with tests.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly matches the main change: decoding byte-string fill values to make JSON serialization safe.
Out of Scope Changes check ✅ Passed The changelog, version bump, and tests are all directly related to the byte-string serialization fix.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Description check ✅ Passed The PR description follows the required template and includes the summary, issue reference, test steps, and checklist with only minor omissions.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

publish_umm_var function requires more exception handling of CMR response

1 participant