Fix checksum handling in DAP4. #1211
Merged
Conversation
This file contains 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
DennisHeimbigner
added a commit
to DennisHeimbigner/tds
that referenced
this pull request
Jul 8, 2023
## Description of Changes WARNING: The merging of this PR needs to be synchronized with the following netcdf-java PR: Unidata/netcdf-java#1211 So after both PRs are reviewed and approved, both need to be merged one right after the other. This PR make major changes to the DAP4 code. It also makes some small but necessary changes to non-DAP4 code, which will be described below. ### DAP4 Change Overview * Cleanup the D4TS server -- D4TS standing for Dap4 Test Server -- so that it properly operates in the remotetest.unidata.ucar.edu test server. * Remove all DAP4 tests; they all depended on using MockServlet, which basically does not work. The assumption is that we will test dap4 support in the process of testing DAP4 support in netcdf-java and netcdf-c. * The DAP4 service in TDS is left disabled until everything has been successfully merged. * Update gradle files to reflect the flattening of the netcdf-java/dap4 module from PR Unidata/netcdf-java#1133. #### DAP4 Specific Changes * Remove all DSP types and replace with the *CDMWrap* class. * Move the Odometer classes from *netcdf-java* to *tds* since they are only used in that repo. * Remove the attempt to replace netcdf-c JNA system. Now relies only on CDM API. * Remove the synthetic test case generator and all supporting classes. * Remove the dap4/d4tests directory since there are no longer any tests. * Change the gradle "compile" action to the "implementation" to conform to the upcoming gradle version 7 (this change is only partially completed). * Move the various D4TS support files into the WEB-INF directory so the from() actions are no longer needed. * Create a new set of .nc files to be used for testing clients. These files come from and are consistent with those expected by netcdf-c. * Create a new D4TS server web page template (the so-called "FrontPage"). * Created DSR template files to support per-dataset creation of DSRs. * Rebuilt the handling of the *getResourcePath* function to make it dependent on the specific DAP4 Servlet/Controller (i.e. under d4ts vs under thredds). * Added an abstract *getWebContentRoot* function to *DapController* so that each servlet can provide specific locations for the template files. * When a zero-size dimension is encountered, now suppress that dimension and any variable that used it. Ideally this should be an error, but we have encountered too many instances in the wild, so it is better to suppress rather than cause an error. * A number of classes were renamed to more accurately reflect their semantics. Specifically: - *CDMCursor* -> *CDMData* * Convert DSP LRU cache to only cache the NetcdfFile instances. * Remove some now unused classes: - *URLMap* - *URLMapDefault* * *Dap4Controller* was modified to enable DAP4 as a service. * Properly handle checksums in the DMR and DAP. ### Non-DAP4 Specific Changes * Modify CatalogViewContextParser to map a request for file using the DAP4 service: change from *.dmr.xml* to *.dsr.html*. This may change again in the future if we try to emulate the DAP2 .ascii format. * Re-enable the Dap4Controller * Enable the DAP4 service in various test catalogs. * The changes to the intercept code in netcdf-java required changes to some tds tests; specificially *TestFormBuilder* (also converted to JUNIT4 parameterized test format). * Some constants and static utility functions were moved into the CDM class. Currently, this only affects one file outside of DAP4, namely the opendap file NcDAS.java. * Change occurrences of "static protected|public" to "protected|public static". ## PR Checklist <!-- This will become an interactive checklist once the PR is opened --> - [X] Link to any issues that the PR addresses - [ ] Add labels - [X] Open as a [draft PR](https://github.blog/2019-02-14-introducing-draft-pull-requests/) until ready for review - [X] Make sure GitHub tests pass - [X] Mark PR as "Ready for Review"
5 tasks
haileyajohnson
approved these changes
Nov 6, 2023
| @@ -334,7 +334,7 @@ public List<Slice> getConstrainedSlices(DapVariable var) throws DapException { | |||
| Segment seg = findSegment(var); | |||
| if (seg != null) | |||
| slices = seg.slices; | |||
| if (slices == null) | |||
| if (slices == null || slices.size() == 0) | |||
There was a problem hiding this comment.
Hah! I am imprinted with my C idoms. Will change.
The DAP4 specification says how to properly handle checksums, but this was improperly implemented in netcdf-java. This PR makes some changes to handle checksums. WARNING: The merging of this PR needs to be synchronized with the following netcdf-java PR: So after both PRs are reviewed and approved, both need to be merged one right after the other. Note also that their are still unresolved ambiguities about checksums in the DAP4 specification that need resolution. See (here)[OPENDAP/dap4-specification#6] for example. So at the moment, accessing non-thredds servers via DAP4 requires special treatment. ## PR Checklist <!-- This will become an interactive checklist once the PR is opened --> - [X] Link to any issues that the PR addresses - [] Add labels - [X] Open as a [draft PR](https://github.blog/2019-02-14-introducing-draft-pull-requests/) until ready for review - [X] Make sure GitHub tests pass - [X] Make sure Jenkins tests pass - [ ] Mark PR as "Ready for Review" ## DAP4 Changes 1. Properly compute and apply checksums in the DAP and DMR responses. 2. Fix tests that require the _DAP4_Checksum_CRC32 attribute. ## Non-DAP4 Changes The only controversial change is that to ucar.ma2.Index, about line 573. The change is as follows: ```` int prefixrank = (hasvlen ? rank: rank - 1); to int prefixrank = (hasvlen ? rank - 1 : rank); ```` The original line is clearly an error under the assumption that "*" dimensions are always the last dimension for an array. I was worried about this causing follow-on failures, but not surprisingly because vlen is so screwed up, no follow-on failures have appeared.
haileyajohnson
force-pushed
the
checksumfix.dmh
branch
from
23f0a79
to
442b0fb
Compare
|
Fixed, |
1 task
haileyajohnson
pushed a commit
to Unidata/tds
that referenced
this pull request
Jan 22, 2024
* ## Cleanup the DAP4 support in the tds repository ## Description of Changes WARNING: The merging of this PR needs to be synchronized with the following netcdf-java PR: Unidata/netcdf-java#1211 So after both PRs are reviewed and approved, both need to be merged one right after the other. This PR make major changes to the DAP4 code. It also makes some small but necessary changes to non-DAP4 code, which will be described below. ### DAP4 Change Overview * Cleanup the D4TS server -- D4TS standing for Dap4 Test Server -- so that it properly operates in the remotetest.unidata.ucar.edu test server. * Remove all DAP4 tests; they all depended on using MockServlet, which basically does not work. The assumption is that we will test dap4 support in the process of testing DAP4 support in netcdf-java and netcdf-c. * The DAP4 service in TDS is left disabled until everything has been successfully merged. * Update gradle files to reflect the flattening of the netcdf-java/dap4 module from PR Unidata/netcdf-java#1133. #### DAP4 Specific Changes * Remove all DSP types and replace with the *CDMWrap* class. * Move the Odometer classes from *netcdf-java* to *tds* since they are only used in that repo. * Remove the attempt to replace netcdf-c JNA system. Now relies only on CDM API. * Remove the synthetic test case generator and all supporting classes. * Remove the dap4/d4tests directory since there are no longer any tests. * Change the gradle "compile" action to the "implementation" to conform to the upcoming gradle version 7 (this change is only partially completed). * Move the various D4TS support files into the WEB-INF directory so the from() actions are no longer needed. * Create a new set of .nc files to be used for testing clients. These files come from and are consistent with those expected by netcdf-c. * Create a new D4TS server web page template (the so-called "FrontPage"). * Created DSR template files to support per-dataset creation of DSRs. * Rebuilt the handling of the *getResourcePath* function to make it dependent on the specific DAP4 Servlet/Controller (i.e. under d4ts vs under thredds). * Added an abstract *getWebContentRoot* function to *DapController* so that each servlet can provide specific locations for the template files. * When a zero-size dimension is encountered, now suppress that dimension and any variable that used it. Ideally this should be an error, but we have encountered too many instances in the wild, so it is better to suppress rather than cause an error. * A number of classes were renamed to more accurately reflect their semantics. Specifically: - *CDMCursor* -> *CDMData* * Convert DSP LRU cache to only cache the NetcdfFile instances. * Remove some now unused classes: - *URLMap* - *URLMapDefault* * *Dap4Controller* was modified to enable DAP4 as a service. * Properly handle checksums in the DMR and DAP. ### Non-DAP4 Specific Changes * Modify CatalogViewContextParser to map a request for file using the DAP4 service: change from *.dmr.xml* to *.dsr.html*. This may change again in the future if we try to emulate the DAP2 .ascii format. * Re-enable the Dap4Controller * Enable the DAP4 service in various test catalogs. * The changes to the intercept code in netcdf-java required changes to some tds tests; specificially *TestFormBuilder* (also converted to JUNIT4 parameterized test format). * Some constants and static utility functions were moved into the CDM class. Currently, this only affects one file outside of DAP4, namely the opendap file NcDAS.java. * Change occurrences of "static protected|public" to "protected|public static". ## PR Checklist <!-- This will become an interactive checklist once the PR is opened --> - [X] Link to any issues that the PR addresses - [ ] Add labels - [X] Open as a [draft PR](https://github.blog/2019-02-14-introducing-draft-pull-requests/) until ready for review - [X] Make sure GitHub tests pass - [X] Mark PR as "Ready for Review" * Upgrade to latest master --------- Co-authored-by: haileyajohnson <hailey.johnson@ufl.edu>
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 of Changes
The DAP4 specification says how to properly handle checksums, but this was improperly implemented in netcdf-java. This PR makes some changes to handle checksums.
WARNING: The merging of this PR needs to be synchronized with the following netcdf-java PR: XXXX.
So after both PRs are reviewed and approved, both need to be merged one right after the other.
Note also that their are still unresolved ambiguities about checksums in the DAP4 specification that need resolution. See here for example. So at the moment, accessing non-thredds servers via DAP4 requires special treatment.
PR Checklist
DAP4 Changes
Non-DAP4 Changes
The only controversial change is that to ucar.ma2.Index, about line 573. The change is as follows:
The original line is clearly an error under the assumption that "*" dimensions are always the last dimension for an array. I was worried about this causing follow-on failures, but not surprisingly because vlen is so screwed up, no follow-on failures have appeared.