Skip to content

fix: Include payload in validation#38

Closed
zfields wants to merge 6 commits intomasterfrom
zak-payload
Closed

fix: Include payload in validation#38
zfields wants to merge 6 commits intomasterfrom
zak-payload

Conversation

@zfields
Copy link
Copy Markdown
Contributor

@zfields zfields commented May 1, 2025

Started as an effort to rebase #14.

I quickly realized that we have taken a deeper dependency on JSONMarshal/JSONUnmarshal (due to card.binary support).

I ensured that the -req would be performed on the underlying CLI parameter, not a parsed request structure.

However, it must be noted, when a payload is offered via -input, it is appended to the original parameter before being passed to the Notecard. When this occurs, it cannot fail silently, which should satisfy the original concern of #14.

@zfields zfields requested review from Bucknalla, Copilot and rayozzie May 1, 2025 18:40
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR aims to improve the request validation process by incorporating payload data from an external file and refining error reporting.

  • Adjusts the validateRequest function signature to accept a verbose flag.
  • Updates main.go to read and integrate a file-based payload into the JSON request.
  • Modifies error handling in validation to optionally output detailed errors and suppress JSON parse errors.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
notecard/validate.go Adds a verbose flag to validateRequest and changes its error handling.
notecard/main.go Incorporates payload file reading and updates the validation call accordingly.
Comments suppressed due to low confidence (1)

notecard/validate.go:187

  • [nitpick] The generic error returned on schema validation failure omits the underlying error details when verbose is false, which could hinder debugging. Consider including critical error details in the returned message while ensuring no sensitive data is exposed.
if err := schema.Validate(reqMap); err != nil { ... }

Comment thread notecard/validate.go Outdated
Copy link
Copy Markdown
Collaborator

@Bucknalla Bucknalla left a comment

Choose a reason for hiding this comment

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

Just the copilot suggestion :)

Comment thread notecard/validate.go Outdated
@zfields zfields force-pushed the zak-payload branch 2 times, most recently from 30f50e6 to 5c4b4ee Compare May 1, 2025 19:36
@zfields zfields force-pushed the zak-payload branch 2 times, most recently from c37cdfd to c3df855 Compare May 2, 2025 03:14
@zfields zfields requested review from Bucknalla and Copilot May 2, 2025 03:18
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request addresses JSON validation improvements by ensuring that payload data provided via –input is included in the validation process and by refining schema caching. Key changes include:

  • Refactoring the validateRequest function to accept a map and add verbose logging.
  • Updating main() to append file-based payloads to the request and re-marshal the updated JSON.
  • Adding a new configuration flag (toggle-json-validation) and related field to control JSON schema validation.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
notecard/validate.go Updated validation functionality and schema cache path to support payload updates.
notecard/main.go Revised payload handling, error validation, and enhanced logging during validation.
lib/config.go Introduced a new configuration flag/field for JSON validation and updated flag docs.

Comment thread notecard/main.go Outdated
Comment thread lib/config.go Outdated
Copy link
Copy Markdown
Collaborator

@Bucknalla Bucknalla left a comment

Choose a reason for hiding this comment

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

Looks good, some small changes

Comment thread notecard/main.go Outdated
Comment thread lib/config.go Outdated
Comment thread notecard/main.go Outdated
Comment thread lib/config.go Outdated
Comment thread lib/config.go
Comment thread lib/config.go Outdated
Comment thread lib/config.go Outdated
Comment thread notecard/main.go Outdated
Comment thread notecard/validate.go Outdated
Comment thread notecard/validate.go Outdated
@zfields zfields requested review from Bucknalla, Copilot and rayozzie May 2, 2025 17:50
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR aims to fix payload validation by including the payload in the validation process and updating the JSON schema control flow. Key changes include:

  • Refactoring schema validation logic in notecard/validate.go, including adding a helper (resolveSchemaError) to report schema errors.
  • Adjusting CLI behavior in notecard/main.go to append payload input and control JSON schema settings via environment variables.
  • Removing the now-unused json-schema-url flag and its related cache clearing logic in lib/config.go.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
notecard/validate.go Refactored schema validation logic and error resolution approach.
notecard/main.go Updated CLI payload handling and JSON schema validation controls.
lib/config.go Removed json-schema-url flag and updated related comments.

Comment thread notecard/validate.go Outdated
Comment thread notecard/main.go
@zfields zfields requested a review from Copilot May 2, 2025 18:06
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR addresses validation improvements by including payloads in the validation process and refactoring schema-related logic.

  • Refactors the validateRequest function to use a new resolveSchemaError helper and adjusts schema URL construction.
  • Enhances main.go to append payloads from a file and control JSON schema settings via environment variables.
  • Updates and removes unused configuration flags in lib/config.go.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
notecard/validate.go Refactors schema validation by splitting error handling and updating schema URL composition.
notecard/main.go Adds payload injection from file and improves JSON schema env var handling.
lib/config.go Removes the json-schema-url flag and adjusts comments for consistency.

Comment thread notecard/validate.go Outdated
Comment thread notecard/main.go
Copy link
Copy Markdown
Collaborator

@Bucknalla Bucknalla left a comment

Choose a reason for hiding this comment

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

Some important changes but I'll unblock the PR 👍🏻

Comment thread notecard/validate.go
Comment thread notecard/validate.go Outdated
Comment thread notecard/validate.go Outdated
@zfields
Copy link
Copy Markdown
Contributor Author

zfields commented May 4, 2025

Closed in favor of #39

@zfields zfields closed this May 4, 2025
@zfields zfields deleted the zak-payload branch May 5, 2025 14:15
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.

4 participants