Repository navigation
Jhrg/hyrax 2316 segfault ngapapi - #1450
Merged
Merged
Conversation
Drop the -fno-exceptions template note and the human-oriented Plan mode section, and fix "The is" -> "This is" in the project description. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Register test_get_urls_from_granules_umm_json_v1_4_related_url_wo_Type in NgapApiTest's suite; it was written but never run. Delete cmr_json_responses/related_urls_wo_Type.json, a byte-identical copy of no_related_urls.json that nothing referenced. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- get_data_http_url / get_data_s3_url reject a URL only when it ends in .xml or .dmrpp (FR-007, FR-008), not when it contains them anywhere. get_s3credentials_url uses the same ends_with() helper. - Move the defensive .dmrpp trim into NgapApi::strip_dmrpp_suffix(), so each URL is trimmed by its own length; the S3 trim used the https URL's length. - NgapOwnedContainer::dmrpp_read_from_daac_bucket always appends .dmrpp to the https data URL, like the S3 URL (FR-011); otherwise a URL such as granule.dmrpp.nc would be fetched as its own DMR++. - Regression tests with new CMR fixtures; test_dmrpp_is_removed_from_data_url now calls NgapApi instead of a copy of the logic. - Correct the BUG-001 log: the old trim gave s3://some-bucket/lo. The container change has no unit test: that path needs CMR and URL signing over the network. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…der hygiene - Replace the hand-managed curl_easy_escape/unescape + curl_free calls in build_cmr_query_url* with url_escape()/url_unescape() helpers that hold the curl buffer in a unique_ptr and throw BESInternalError on null. The CURL* handle is a unique_ptr with curl_easy_cleanup, checked for null. CMR query strings are unchanged (FR-001..FR-004; resty_path tests pass). - get_cmr_search_endpoint_url() reads the BES keys on every call instead of caching in a function-local static. New test cmr_endpoint_reads_keys_each_call fails on the cached version. - NgapApi.h: drop unused <vector>, <map> and the http::url forward declaration; NgapOwnedContainer.cc and NgapApiTest.cc include <vector>. The null-return branches of the curl helpers are untested; libcurl returns null only on allocation failure. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… crashing get_urls_from_granules_umm_json_v1_4 and its RelatedUrls helpers relied on rapidjson preconditions that are only asserted, and NDEBUG (release) builds don't check them. Check each one first: - parse errors and a non-object document - missing or non-int 'hits' (release builds read it as 0 and reported 'granule not found', a false not-found; FR-006) - missing or empty 'items', items[0] not an object - missing or non-object 'umm' (the unchecked FindMember at the old line 433; HYRAX-2316) - RelatedUrls entries that aren't objects, or whose Type/URL aren't strings (new file-local get_string_member(), which keeps the existing 'does not contain the URL/Type object' messages) 12 regression tests, one per case. Each aborted or segfaulted the test program before the fix. They also pass in an NDEBUG + ASan/UBSan build, where the old code shows UB and the false not-found. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
cmdln/tests/dmrpp/ (added in 763ed1c, HYRAX-1732) was never listed in EXTRA_DIST, so make distcheck failed cmdln/tests 61 because the .bescmd input was missing from the tarball. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…nction throws write_dataset_to_cache wrote the resource-id line, then evaluated the server function. If that threw, the catch block unlocked the file but left it on disk, so every later identical request was a cache hit on an entry with no data and failed with 'The DDX response document parse failed ... Could not read from input stream' instead of the function's own error (cmdln/tests 48 and 51 from their second run on). Unlink the file in the catch block while still holding the exclusive lock. purge_file() isn't used because the partial file's size was never added to the cache info. An unlink failure is logged, not thrown, so the original exception still propagates. New regression test failed_function_leaves_no_cache_entry: call 1 of test() failed with the DDX parse error before the fix. Partial entries already on a server's disk stay until purged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
jgallagher59701
marked this pull request as draft
October 29, 2026 23:32
|
@jgallagher59701 wondering if you intended to have the Claude files ( |
Member
Author
Yeah, that should not be in there. I'll remove it. |
jgallagher59701
marked this pull request as ready for review
October 30, 2026 20:28
The result of two consecutive errors returns the same error message; the second error does not return a mangled message about DDS objects.
rfrm-opendap
approved these changes
Oct 30, 2026
|
|
|
kneumiller
approved these changes
Oct 30, 2026
kneumiller
left a comment
kneumiller
left a comment
Contributor
There was a problem hiding this comment.
Lots of changes here, but looks good
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
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.
Description
HYRAX-2316
In the NgapApi class, CMR is used to fetch JSON information. If the response is missing parts, our code will segfault. This bug was reproduced with a regression test and then fixed. Along the way, other tests wee fixed in the module.
I also found and fixed a test in the functions module that would show up when running the tests several times when the BES was running - a stale cache entry made by the functions code.
TL;DR just focus on the C++ code for the review.
For this review, the various docs in plans, docs, etc., are part of the process I used. The actual changes to code and in modules/dmrpp_module/ngap_container. One change was fixed in the modules/functions code - a test suite bug where the command line tests left a cache entry that could cause subsequent runs of the tests to incorrectly fail.
Tasks