Fix NaN incorrectly rejected as invalid value for ASCII_String table fields by jordanpadams · Pull Request #1532 · NASA-PDS/validate

jordanpadams · 2026-03-19T22:39:26Z

🗒️ Summary

FieldValueValidator was incorrectly throwing a data type mismatch error when a table field with data type ASCII_String contained the string value "NaN". The check at FieldValueValidator.checkType() was designed to reject NaN/Inf values for non-floating-point numeric types, but it failed to account for string types where any string — including "NaN" — is valid.

Fix: Added ASCII_String to a stringTypes list and updated the NaN/Inf guard to skip the check when the field type is a string type.

🤖 AI Assistance Disclosure

No AI assistance used
AI used for light assistance (e.g., suggestions, refactoring, documentation help, minor edits)
AI used for moderate content generation (AI generated some code or logic, but the developer authored or heavily revised the majority)
AI generated substantial portions of this code

Estimated % of code influenced by AI: 60%

⚙️ Test Data and/or Report

Cucumber test added in src/test/resources/features/4.1.x.feature with tag @4.1.x. Test data (real-world Chandrayaan-1 Mini-RF housekeeping CSV with ASCII_String fields containing NaN values) placed in src/test/resources/github956/.

mvn test -Dtest=\!ReferenceIntegrityTest* -Dcucumber.filter.tags='@4.1.x'

Result: 1 scenario passed.

♻️ Related Issues

Fixes #956

🤓 Reviewer Checklist

Reviewers: Please verify the following before approving this pull request.

Documentation and PR Content

Documentation: README, Wiki, or inline documentation (Sphinx, Javadoc, Docstrings) have been updated to reflect these changes.
Issue Traceability: The PR is linked to a valid GitHub Issue
PR Title: The PR title is "user-friendly" clearly identifying what is being fixed or the new feature being added, that if you saw it in the Release Notes for a tool, you would be able to get the gist of what was done.

Security & Quality

SonarCloud: Confirmed no new High or Critical security findings.
Secrets Detection: Verified that the Secrets Detection scan passed and no sensitive information (keys, tokens, PII) is exposed.
Code Quality: Code follows organization style guidelines and best practices for the specific language (e.g., PEP 8, Google Java Style).

Testing & Validation

Test Accuracy: Verified that test data is accurate, representative of real-world PDS4 scenarios, and sufficient for the logic being tested.
Coverage: Automated tests cover new logic and edge cases.
Local Verification: (If applicable) Successfully built and ran the changes in a local or staging environment.

Maintenance

Backward Compatibility: Confirmed that these changes do not break existing downstream dependencies or API contracts (or that breaking changes are clearly documented).

…fields Closes #956 Co-Authored-By: Claude Sonnet 4.6 <[email protected]>

nutjob4life

Code delta: ✓
Maven tests: ✓
Approval: ✅
Maven details:

293 scenarios (293 passed)
879 steps (879 passed)
11m 51.843s
Mar 19, 2026 6:35:14 PM org.junit.platform.launcher.core.DiscoveryIssueNotifier logIssues
WARNING: TestEngine with ID 'cucumber' encountered a non-critical issue during test discovery:

(1) [WARNING] Discovering tests using the cucumber.features property. Other discovery selectors are ignored!

This is a work around for the limited JUnit 5 support in Maven and Gradle. Please request/upvote/sponsor/ect better support for JUnit 5 discovery selectors. For details see: https://github.com/cucumber/cucumber-jvm/pull/2498

If you are using the JUnit 5 Suite Engine, Platform Launcher API or Console Launcher you should not use this property. Please consult the JUnit 5 documentation on test selection.
[INFO] Tests run: 0, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 711.9 s -- in io.cucumber.junit.platform.engine.CucumberTestEngine
[INFO] 
[INFO] Results:
[INFO] 
[INFO] Tests run: 25, Failures: 0, Errors: 0, Skipped: 23
[INFO] 
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS

Copilot

Pull request overview

Fixes a table-field validation false positive where the literal string "NaN" was rejected for ASCII_String table fields, and adds a regression test dataset to reproduce issue #956.

Changes:

Update FieldValueValidator.checkType() to skip the Inf/NaN guard for string-typed fields.
Add Cucumber coverage and associated github956 test fixtures (PDS4 XML + PDS3 label + CSV).

Reviewed changes

Copilot reviewed 4 out of 5 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
src/main/java/gov/nasa/pds/tools/validate/content/table/FieldValueValidator.java	Adjusts Inf/NaN rejection logic to permit string fields.
src/test/resources/features/4.1.x.feature	Adds a new Scenario Outline example for issue #956.
src/test/resources/github956/fsb_01500_rhk_xib_85s238_v1.xml	Adds a PDS4 label fixture describing an `ASCII_String` delimited table.
src/test/resources/github956/fsb_01500_rhk_xib_85s238_v1.lbl	Adds a PDS3 supplemental label fixture used by the test dataset.
src/test/resources/github956/fsb_01500_rhk_xib_85s238_v1.csv	Adds the CSV fixture containing `"NaN"` values in string fields.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-20T15:27:51Z

src/main/java/gov/nasa/pds/tools/validate/content/table/FieldValueValidator.java

+  // String types that accept any string value, including "NaN" (issue #956)
+  private static List<FieldType> stringTypes = Arrays.asList(
+      FieldType.ASCII_STRING);


stringTypes is a mutable, non-final static collection. Since it’s used as a constant for type classification, make it static final and preferably an immutable Set (e.g., EnumSet) to avoid accidental modification and to make membership checks clearer.

Copilot · 2026-03-20T15:27:52Z

src/test/resources/features/4.1.x.feature

    @4.1.x
    Examples: 
      | issueNumber | subtest | datasrc | args | expectation |
+| 956 | 1 | "github956" | "--skip-context-validation -t {datasrc}/" | |


The Examples table row is not aligned with the header (missing the expected indentation and leading |). As written, Gherkin will not parse this as a table row for the Scenario Outline examples.

Suggested change

| 956 | 1 | "github956" | "--skip-context-validation -t {datasrc}/" | |

| 956 | 1 | "github956" | "--skip-context-validation -t {datasrc}/" | |

Copilot · 2026-03-20T15:27:52Z

src/test/resources/github956/fsb_01500_rhk_xib_85s238_v1.lbl

+RECORD_TYPE                =  STREAM
+RECORD_BYTES               =  85
+FILE_RECORDS               =  116
+^SPREADSHEET               =  "FSB_01500_RHK_XIB_85S238_V1.CSV"


The PDS3 label points ^SPREADSHEET to an uppercase filename ("FSB_01500_RHK_XIB_85S238_V1.CSV"), but the actual CSV added in this test data directory is lowercase. On case-sensitive filesystems this can break any logic that follows this pointer to locate the data file; consider matching the on-disk filename exactly (or aligning both to a consistent case).

Suggested change

^SPREADSHEET = "FSB_01500_RHK_XIB_85S238_V1.CSV"

^SPREADSHEET = "fsb_01500_rhk_xib_85s238_v1.csv"

Copilot · 2026-03-20T15:27:53Z

src/test/resources/github956/fsb_01500_rhk_xib_85s238_v1.csv

+"Mini-RF Housekeeping Parameters File"
+"Characters per entry","85.","counts"
+"Header entries per file","9.","counts"
+"Data entries per record","30.","counts"
+"Records in this file","116.","records"
+"File format version number","1.1000000000","version"
+"File creation time","2010-286T22:50:18.294","UTC"
+"Collection Start Time","2009-071T14:11:25.000","UTC"
+"Collection Stop Time","2009-071T14:14:54.000","UTC"
+"Record Sequence Count","1.","counts"
+"Record Start Time","2009-071T14:11:25.000","UTC"
+"MSR RX FPGA DIE DG-C","NaN","Degrees C"


This test fixture CSV adds ~3.5k lines of data, which increases repo size and can slow test checkout/execution. If the scenario only needs to prove that an ASCII_String field can contain the literal "NaN", consider reducing the CSV to the smallest representative subset that still reproduces the issue.

Copilot · 2026-03-20T15:27:53Z

src/main/java/gov/nasa/pds/tools/validate/content/table/FieldValueValidator.java

    LOG.debug("checkType:value,type:after [{}],[{}]", value, type);

-    if (SpecialConstantChecker.isInfOrNan(value) && !realTypes.contains(type)) {
+    if (SpecialConstantChecker.isInfOrNan(value) && !realTypes.contains(type) && !stringTypes.contains(type)) {


The NaN/Inf guard skips only ASCII_STRING, but this method also supports FieldType.UTF8_STRING later in the same chain. As a result, a UTF8_String field containing "NaN" would still throw "NaN" is not allowed. Consider treating all string field types as exempt from the Inf/NaN check (e.g., include UTF8_STRING here, and ideally centralize string-type membership in a single set).

Suggested change

if (SpecialConstantChecker.isInfOrNan(value) && !realTypes.contains(type) && !stringTypes.contains(type)) {

// Treat all string field types (including UTF8_STRING) as exempt from the Inf/NaN check.

final boolean isStringType =

stringTypes.contains(type)

|| FieldType.UTF8_STRING.getXMLType().equals(type.getXMLType());

if (SpecialConstantChecker.isInfOrNan(value) && !realTypes.contains(type) && !isStringType) {

Fix NaN incorrectly rejected as invalid value for ASCII_String table …

315cf7f

…fields Closes #956 Co-Authored-By: Claude Sonnet 4.6 <[email protected]>

jordanpadams requested a review from a team as a code owner March 19, 2026 22:39

jordanpadams added the bug Something isn't working label Mar 19, 2026

jordanpadams self-assigned this Mar 19, 2026

jordanpadams added the bug Something isn't working label Mar 19, 2026

nutjob4life approved these changes Mar 20, 2026

View reviewed changes

jordanpadams requested review from al-niessner and Copilot March 20, 2026 15:24

Copilot started reviewing on behalf of jordanpadams March 20, 2026 15:25 View session

Copilot AI reviewed Mar 20, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix NaN incorrectly rejected as invalid value for ASCII_String table fields#1532

Fix NaN incorrectly rejected as invalid value for ASCII_String table fields#1532
jordanpadams wants to merge 1 commit intomainfrom
bugfix/956-ascii-string-nan-allowed

jordanpadams commented Mar 19, 2026

Uh oh!

nutjob4life left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 20, 2026

Uh oh!

Copilot AI Mar 20, 2026

Uh oh!

Copilot AI Mar 20, 2026

Uh oh!

Copilot AI Mar 20, 2026

Uh oh!

Copilot AI Mar 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	\| 956 \| 1 \| "github956" \| "--skip-context-validation -t {datasrc}/" \| \|
	\| 956 \| 1 \| "github956" \| "--skip-context-validation -t {datasrc}/" \| \|

	^SPREADSHEET = "FSB_01500_RHK_XIB_85S238_V1.CSV"
	^SPREADSHEET = "fsb_01500_rhk_xib_85s238_v1.csv"

-    if (SpecialConstantChecker.isInfOrNan(value) && !realTypes.contains(type) && !stringTypes.contains(type)) {
+    // Treat all string field types (including UTF8_STRING) as exempt from the Inf/NaN check.
+    final boolean isStringType =
+        stringTypes.contains(type)
+            || FieldType.UTF8_STRING.getXMLType().equals(type.getXMLType());
+    if (SpecialConstantChecker.isInfOrNan(value) && !realTypes.contains(type) && !isStringType) {

Conversation

jordanpadams commented Mar 19, 2026

🗒️ Summary

🤖 AI Assistance Disclosure

⚙️ Test Data and/or Report

♻️ Related Issues

🤓 Reviewer Checklist

Documentation and PR Content

Security & Quality

Testing & Validation

Maintenance

Uh oh!

nutjob4life left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 20, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants