Conversation
|
👋 cedric-cordenier, thanks for creating this pull request! To help reviewers, please consider creating future PRs as drafts first. This allows you to self-review and make any final changes before notifying the team. Once you're ready, you can mark it as "Ready for review" to request feedback. Thanks! |
product-security-plaid-production
bot
requested a review
from fernandofederico1984
|
✅ No conflicts with other open PRs targeting |
There was a problem hiding this comment.
Pull request overview
Risk Rating: HIGH (changes the on-wire/serialized representation and decoding behavior of CRE job spec inputs across YAML/JSON entrypoints)
This PR aims to fix Gateway job spec marshaling by changing how JobSpecInput is represented and decoded, moving toward JSON-based marshaling and adding JSON struct tags to previously YAML-only types.
Changes:
- Refactors
JobSpecInputfrom amap[string]anyto ajson.RawMessagewrapper and switchesUnmarshalTo/UnmarshalFromto useencoding/json. - Updates CRE propose-job-spec input shape to hold
Inputsas a pointer, and adjusts a TOML parsing site accordingly. - Adds
jsontags to standard capability / oracle factory related structs and updates a unit test to construct inputs via JSON marshaling.
Scrupulous human review recommended (high impact areas):
- YAML ↔ inputs parsing behavior for
jobs.ProposeJobSpecInput(existing YAML-driven flows/tests appear to depend on YAML decoding ofinputs). - Backwards-compatibility of “empty/omitted inputs” behavior after switching to
json.RawMessage. - Any downstream consumers expecting
JobSpecInputmap semantics (mutation, map literals,delete, etc.).
Suggested reviewers (CODEOWNERS):
@smartcontractkit/keystone,@smartcontractkit/operations-platform(owners for/deployment/cre)
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| deployment/cre/jobs/types/job_spec.go | Reworks JobSpecInput to store raw JSON and decode via encoding/json. |
| deployment/cre/jobs/types/job_spec_test.go | Updates tests to build JobSpecInput via JSON marshaling. |
| deployment/cre/jobs/propose_job_spec.go | Changes Inputs to a pointer and adjusts precondition verification calls. |
| deployment/cre/jobs/pkg/types.go | Adds JSON tags to oracle factory config structs. |
| deployment/cre/jobs/pkg/std_cap.go | Adds JSON tags to StandardCapabilityJob fields for JSON-based decoding. |
| deployment/cre/jobs/operations/propose_std_cap_job.go | Adjusts TOML unmarshal target from JobSpecInput to map[string]any. |
| } | ||
|
|
||
| func (j *JobSpecInput) UnmarshalJSON(d []byte) error { | ||
| j.RawMessage = d |
There was a problem hiding this comment.
UnmarshalJSON assigns the input byte slice directly to RawMessage. Per the encoding/json contract, UnmarshalJSON must copy the data if it needs to retain it after returning (the decoder may reuse the backing array). Make a copy of d before storing it to avoid subtle data corruption.
| j.RawMessage = d | |
| if d == nil { | |
| j.RawMessage = nil | |
| return nil | |
| } | |
| copied := make([]byte, len(d)) | |
| copy(copied, d) | |
| j.RawMessage = json.RawMessage(copied) |
|
|
||
| return yaml.Unmarshal(bytes, target) | ||
| func (j JobSpecInput) UnmarshalTo(target any) error { | ||
| return json.Unmarshal([]byte(j.RawMessage), target) |
There was a problem hiding this comment.
UnmarshalTo calls json.Unmarshal on j.RawMessage. When JobSpecInput is the zero value (RawMessage is nil/empty), json.Unmarshal returns "unexpected end of JSON input". If callers previously relied on an empty inputs object (or default/omitted inputs) being treated as an empty map, consider treating empty RawMessage as a no-op (or as {}) to preserve backwards-compatible behavior.
| return json.Unmarshal([]byte(j.RawMessage), target) | |
| // Treat empty or nil RawMessage as a no-op to preserve backward-compatible behavior | |
| // where omitted/empty inputs are interpreted as default/zero-valued targets. | |
| if len(j.RawMessage) == 0 { | |
| return nil | |
| } | |
| // Also treat explicit JSON null as a no-op for consistency with default semantics. | |
| if string(j.RawMessage) == "null" { | |
| return nil | |
| } | |
| return json.Unmarshal(j.RawMessage, target) |
| switch config.Template { | ||
| case job_types.EVM: | ||
| if err := verifyEVMJobSpecInputs(config.Inputs); err != nil { | ||
| if err := verifyEVMJobSpecInputs(*config.Inputs); err != nil { | ||
| return fmt.Errorf("invalid inputs for EVM job spec: %w", err) | ||
| } | ||
| case job_types.Solana: | ||
| if err := verifySolanaJobSpecInputs(config.Inputs); err != nil { | ||
| if err := verifySolanaJobSpecInputs(*config.Inputs); err != nil { | ||
| return fmt.Errorf("invalid inputs for EVM job spec: %w", err) |
There was a problem hiding this comment.
VerifyPreconditions dereferences config.Inputs (e.g., *config.Inputs) before checking whether Inputs is nil. If inputs are omitted in the request/YAML, this will panic instead of returning a validation error. Move the config.Inputs == nil check to before the switch (and before any dereference), or avoid dereferencing until after the nil check.
|
| type JobSpecInput struct { | ||
| json.RawMessage | ||
| } |
There was a problem hiding this comment.
| type JobSpecInput struct { | |
| json.RawMessage | |
| } | |
| type JobSpecInput json.RawMessag |
Would this work too?
Requires
Supports