Skip to content

Fix #123: Add validation for invalid temporal keys like 'end'#125

Merged
flamingbear merged 2 commits into
nasa:mainfrom
sonikaarora:fix-issue-123-temporal-end-key-validation
Feb 16, 2026
Merged

Fix #123: Add validation for invalid temporal keys like 'end'#125
flamingbear merged 2 commits into
nasa:mainfrom
sonikaarora:fix-issue-123-temporal-end-key-validation

Conversation

@sonikaarora

Copy link
Copy Markdown
Contributor

Jira Issue ID

GitHub Issue #123

Description

Request.is_valid() was incorrectly passing when users accidentally used the
key 'end' instead of 'stop' in temporal range dictionaries. This caused
silent failures where:

  • Validation passed erroneously
  • The 'end' key was silently ignored
  • Backend services received incomplete temporal ranges
  • Jobs failed with confusing errors like "Invalid temporal range, both start
    and end required"

Solution

Added validation to the Request class temporal validations that:

  • Checks that temporal dictionary keys are only 'start' or 'stop'
  • Provides a clear, actionable error message when invalid keys are detected
  • Specifically mentions the common mistake of using 'end' instead of 'stop'

Changes

  • harmony/request.py: Added validation check in temporal_validations list
  • tests/test_request.py: Added 3 comprehensive test cases covering:
    • Using 'start' and 'end' together
    • Using only 'end' without 'start'
    • Using multiple invalid keys including 'end

PR Acceptance Checklist

  • Acceptance criteria met
  • Tests added/updated (if needed) and passing
  • Documentation updated (if needed)

@sonikaarora sonikaarora marked this pull request as draft February 16, 2026 02:13
@sonikaarora sonikaarora changed the title [Draft]] Fix #123: Add validation for invalid temporal keys like 'end' Fix #123: Add validation for invalid temporal keys like 'end' Feb 16, 2026
@sonikaarora sonikaarora marked this pull request as ready for review February 16, 2026 02:15
@sonikaarora sonikaarora marked this pull request as draft February 16, 2026 02:22
@sonikaarora sonikaarora marked this pull request as ready for review February 16, 2026 02:24

@chris-durbin chris-durbin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR - just one comment about simplifying the error message, otherwise looks good.

Comment thread tests/test_request.py Outdated
'end': dt.datetime(2019, 12, 30)
},
('Temporal range keys must be either "start" or "stop". '
'Note: Use "stop" instead of "end" (common mistake).')

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you drop the note about "end" which might be confusing some users?

'Temporal range keys must be either "start" or "stop" makes it clear to users what values are allowed.

@flamingbear flamingbear left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, same comments as Chris.

Comment thread harmony/request.py Outdated
Comment on lines +427 to +428
('Temporal range keys must be either "start" or "stop". '
'Note: Use "stop" instead of "end" (common mistake).')),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
('Temporal range keys must be either "start" or "stop". '
'Note: Use "stop" instead of "end" (common mistake).')),
('Temporal range keys must be either "start" or "stop".')),

Like @chris-durbin said, this is less confusing I think.

Comment thread tests/test_request.py Outdated
Comment on lines +214 to +215
('Temporal range keys must be either "start" or "stop". '
'Note: Use "stop" instead of "end" (common mistake).')

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
('Temporal range keys must be either "start" or "stop". '
'Note: Use "stop" instead of "end" (common mistake).')
('Temporal range keys must be either "start" or "stop".')

@sonikaarora

Copy link
Copy Markdown
Contributor Author

@flamingbear @chris-durbin As suggested I have improved the message. let me know if it looks good now. Thank you

@flamingbear flamingbear self-requested a review February 16, 2026 20:37

@flamingbear flamingbear left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update @sonikaarora 👍

@flamingbear flamingbear merged commit 583b125 into nasa:main Feb 16, 2026
6 checks passed
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.

3 participants